Add Empty state components and use them #285

ZitaNemeckova · 2021-01-04T14:31:31Z

How it looks so far:

TODO:

List of all pages:

Empty state - No data yet:

Collections
Namespaces
My namespaces
Users (it's there but it shouldn't happen)
Groups
Groups - select a group - click on Users tab
Repo management - Remote tab (it's there but it shouldn't happen)
Approval

Empty state - No data for filter:

Collections
Namespaces
My namespaces
Users
Groups
Groups - select a group - click on User tab
Approval

Empty state - Unauthorised:

Groups (view and add/edit)
Users (view and add/edit)
Approval

Custom "errror" state:

Collections - select a collection - click documentation tab (it's there but it shouldn't happen)

ZitaNemeckova · 2021-01-26T15:07:31Z

@sbuenafe-rh @fionayhlin does it look to be heading the right way?

sbuenafe-rh · 2021-01-26T15:24:33Z

@ZitaNemeckova for the No groups empty state, can you swap out the plus icon outline with the solid plus icon? The PF icon name is: fa-plus-circle or if using React name: PlusCircleIcon

For the Not found message. If the Readme file or any other file that isn't available then it should have the PF icon: fa-exclamation-circle with the message: "The file is not available in this version of ___". If the user is searching/filtering for a particular file and it cannot be found, then I would use the magnifying glass icon. Lemme know if you have any questions or need examples/mocks. :)

sbuenafe-rh · 2021-01-26T15:27:45Z

@ZitaNemeckova for the No users added to this group, you can shorten that title to No users

fionayhlin · 2021-01-26T17:50:13Z

@ZitaNemeckova @sbuenafe-rh Should that be No users yet to match the other titles?

sbuenafe-rh · 2021-01-26T17:59:05Z

@fionayhlin @ZitaNemeckova yes! good catch.

ZitaNemeckova · 2021-02-03T11:56:45Z

Container Registry:

Groups:

Collection documentation:

Group detail - users:

@sbuenafe-rh @fionayhlin here's new Container Registry empty pages and fixed ones. WDYT?

ZitaNemeckova · 2021-02-03T15:13:42Z

@himdel please have a look code-wise, thanks :)

himdel · 2021-02-04T13:53:39Z

Sure, thanks for the help getting it working :), some testing...

Repo Management > Remote tab - always shows empty state, on master there are 2 items
My namespaces - Create button doesn't work, nothing happens (the empty state one; the filter one works, but shouldn't be there)
Namespace & My namespaces - filter still there on empty state

(I have yet to go through the code, next comment :))

(I also noticed the UI doesn't stop me from removing the current user, which is probably a bug too, though unrelated :))

And do we have a list of all the screens with an EmptyState?

himdel · 2021-02-04T14:03:16Z

src/components/index.ts

@@ -48,3 +48,7 @@ export { RemoteRepositoryTable } from './repositories/remote-repository-table';
 export { LocalRepositoryTable } from './repositories/local-repository-table';
 export { StatusIndicator } from './status/status-indicator';
 export { HelperText } from './helper-text/helper-text';
+export { EmptyStateFilter } from './empty-state/empty-state-filter';
+export { EmptyStateUnauthorised } from './empty-state/empty-state-unauthorised';


this should probably be spelled Unauthorized (s -> z)? (ngrams)

src/components/repositories/remote-repository-table.tsx

chouseknecht · 2021-02-04T15:08:06Z

@himdel Thanks for reviewing our code! Appreciate the comments. I think @alikins is working on the issue regarding users deleting their own account. View the issue here.

ZitaNemeckova · 2021-02-08T12:58:19Z

@sbuenafe-rh @fionayhlin some more Empty State pages. WDYT?

fionayhlin · 2021-02-08T20:24:59Z

@ZitaNemeckova @sbuenafe-rh I believe the no results found from search keyword ones should have a "Clear all filters" link button in the empty state under the text, but otherwise looks good!

ZitaNemeckova · 2021-02-11T12:12:50Z

@fionayhlin We agreed with @sbuenafe-rh that filter handling should stay in filter component only for this case
I assume it included all searches. WDYT?

- remove Primary button

ZitaNemeckova · 2021-02-15T12:50:35Z

@himdel It's ready for another round of review. Thanks :)

himdel · 2021-02-17T00:35:07Z

src/components/repositories/remote-repository-table.tsx

@@ -69,30 +55,6 @@ export class RemoteRepositoryTable extends React.Component<IProps> {

  render() {
    const { remotes } = this.props;
-    if (remotes.length == 0) {


👍 handled by repository-list now, the only user of RemoteRepositoryTable

himdel · 2021-02-17T00:35:10Z

I think EmptyStateUnauthorised is currently missing a screenshot, and unless it's supposed to be different..

diff --git a/src/components/empty-state/empty-state-unauthorised.tsx b/src/components/empty-state/empty-state-unauthorised.tsx
index a205ca6..bef7e2e 100644
--- a/src/components/empty-state/empty-state-unauthorised.tsx
+++ b/src/components/empty-state/empty-state-unauthorised.tsx
@@ -15,9 +15,9 @@ interface IProps {}
 export class EmptyStateUnauthorised extends React.Component<IProps> {
   render() {
     return (
-      <EmptyState variant={EmptyStateVariant.xs}>
+      <EmptyState variant={EmptyStateVariant.small}>
         <EmptyStateIcon icon={LockIcon} />
-        <Title headingLevel='h4'>
+        <Title headingLevel='h4' size='lg'>
           You do not have have access to Automation Hub
         </Title>
         <EmptyStateBody>

(And I still think Unauthorized is the proper spelling, unless Ansible is using non-US spellings?)

Would it make sense to DRY it up by implementing the other empty states on top of EmptyStateCustom?

If we move the {this.props.button && (... logic there, all the other empty states could just be built on top of it, like..

export const EmptyStateFilter = ({}) => (<EmptyStateCustom
  title="No resuls found"
  description="No results match the filter criteria. Remove all filters or clear all filters to show results."
  icon={SearchIcon}
/>);

export const EmptyStateNoData = ({ title, description, button }) => (<EmptyStateCustom
  title={title}
  description={description}
  icon={button ? PlusCircleIcon : SearchIcon}
  button={button}
/>);

export const EmptyStateUnauthorized = ({}) => (<EmptyStateCustom
  title="You do not have have access to Automation Hub"
  description="Contact you organization administrator for more information."
  icon={LockIcon}
/>);

(+- typescript and the component syntax anyway)

I'm seeing some icon changes, InfoIcon -> SearchIcon, WarningTriangleIcon -> SearchIcon, but those are all intentional, right? :)

And I'll continue tomorrow :)

himdel · 2021-02-17T00:56:30Z

src/utilities/filter-is-set.ts

@@ -0,0 +1,12 @@
+// Checks that at least one filter is set
+export function filterIsSet(params, filters) {


This could be _.some(filters, (filter) => filter in params));, if lodash is preferred.

ZitaNemeckova · 2021-02-17T10:46:38Z

@himdel Icon changes are intentional based on UX requirements. s/z is fixed. I'll do the DRY you suggested as it's really cool!

Unathorised:

ZitaNemeckova · 2021-02-17T13:53:08Z

@himdel last review addressed. Ready for another round. Thanks :)

himdel · 2021-02-18T00:29:50Z

src/containers/namespace-list/namespace-list.tsx

-            />
-            <div>
-              <Pagination
+          {filterIsSet(this.state.params, ['keywords']) ||


consider using a noData var as well? (also on line 166, and possibly 209)

Added noData var. Leaving 209 as is.

himdel · 2021-02-18T00:34:51Z

src/containers/repositories/repository-list.tsx

+              <RemoteRepositoryTable
+                remotes={content}
+                updateParams={this.updateParams}
+                editRemote={(remote: RemoteType) => {


this.selectRemoteToEdit becomes unused (and does the same thing except on a copy of remote?)

I cannot remember why I did that. Fixing back at this.selectRemoteToEdit

himdel · 2021-02-18T00:36:20Z

src/containers/user-management/user-create.tsx

-      return <Redirect to={Paths.notFound}></Redirect>;
-    }
-    return (
+    const notAutthorised =


typo, double T (and s/z for consistency (sorry :)))

himdel · 2021-02-18T00:40:38Z

Thanks, looks great :)

Is redirect ever set from outside the component, or can I assume setState({ redirect: ... only happens in the component itself?

If it's local only, would it make sense to split redirect from 404 and use a separate state param? It looks like some components (edit-namespace) still need to Redirect after update, so you're catching Paths.notFound as a special case and dealing with it differently, while others (certification-dashboard) only used Paths.notFound so the Redirect is removed and redirect is used to just mean "not there".

I'm not seeing any logic errors in the conversion 👍 , but if it's safe to split, I'd keep redirect just for Redirect and use something like setState({ notFound: true }) instead for the unauthorized empty state.

EDIT: or unauthorized like in user-edit

I do notice that filterIsSet needs to know the list of params that could be filters while AppliedFilters seems to need a list of params to ignore.. sounds like the 2 lists could get out of sync fairly quickly, new params would get ignored by filterIsSet but not by AppliedFilters, we may want to fix that by having a per screen list of filters, used by both.

And perhaps the meta params could be app global, enforcing page_size, id etc. can never be treated as a filter, regardles of whether the screen is using it or not (and we could drop the ignoredParams list).

But that's probably a separate refactoring as that's not really related to this change?

I can't seem to be able to rebuild now (pkg_resources.VersionConflict: (pulp-container 2.1.0 (/venv/lib/python3.6/site-packages), Requirement.parse('pulp-container==2.3.1')), possibly related to ansible/galaxy_ng#650 ?), so I have not tested the new changes yet.

But except for comments above, LGTM 👍 :)

ZitaNemeckova · 2021-02-18T09:38:22Z

@himdel

redirect shouldn't be set from outside. I'll fix it as suggested.

Params refactoring sounds like good idea but it's out of scope for this PR.

How to rebuild: galaxy_ng must be on latest master, LOCK_REQUIREMENTS=1 added in .compose.env then build and run migration (to be safe).

…ed instead.

himdel · 2021-02-19T00:09:51Z

src/containers/namespace-list/namespace-list.tsx

@@ -109,6 +104,8 @@ export class NamespaceList extends React.Component<IProps, IState> {
    const { namespaces, params, itemCount } = this.state;
    const { title, filterOwner } = this.props;
    const { user } = this.context;
+    const noData =
+      !filterIsSet(this.state.params, ['keywords']) && namespaces.length === 0;


namespaces can be undefined, we can't check length if it is (maybe just move this after the if (!namespaces)?)

himdel · 2021-02-19T00:17:11Z

Ah, managed to rebuild, docker system prune -a --volumes helped, seems like the build doesn't notice changes to requirements :).

Trying to load /ui/namespaces or /ui/my-namespaces (with a fresh db):

 Cannot read property 'length' of undefined
    at NamespaceList../src/containers/namespace-list/namespace-list.tsx.NamespaceList.render (App.js:12364)

(#285 (comment))
(in other places, groups is always an array, so is collections and items; itemCount doesn't do .length; so this is the only place)

src/containers/certification-dashboard/certification-dashboard.tsx is still using the redirect: Paths.notFound pattern to display EmptyStateUnauthorized

src/containers/namespace-detail/namespace-detail.tsx is still redirecting (actual redirect) to Paths.notFound, should it be doing that?

ZitaNemeckova · 2021-02-19T11:41:16Z

@himdel

src/containers/namespace-detail/namespace-detail.tsx is still redirecting (actual redirect) to Paths.notFound, should it be doing that?

Yes, it gets all http://localhost:8002/ui/<ramdom stuff> calls. This my be worth refactoring later.

himdel

LGTM, not seeing any issues in code or when testing 👍

ZitaNemeckova force-pushed the empty_states branch 2 times, most recently from edb06b4 to 33d53d5 Compare January 11, 2021 09:12

ZitaNemeckova force-pushed the empty_states branch from 33d53d5 to d9e9c58 Compare January 22, 2021 14:00

ZitaNemeckova closed this Jan 22, 2021

ZitaNemeckova reopened this Jan 22, 2021

ZitaNemeckova force-pushed the empty_states branch 2 times, most recently from a89b11b to 08bc0c2 Compare January 26, 2021 08:46

ZitaNemeckova force-pushed the empty_states branch 2 times, most recently from 0e52bee to eefef55 Compare February 3, 2021 11:43

ZitaNemeckova changed the title ~~[WIP] Add Empty state components and use them~~ Add Empty state components and use them Feb 3, 2021

himdel reviewed Feb 4, 2021

View reviewed changes

src/components/repositories/remote-repository-table.tsx Outdated Show resolved Hide resolved

ZitaNemeckova added 6 commits February 12, 2021 15:11

Add Empty state components and use them

f59688e

Use Unauthorised instead of Redirect

d406740

- remove Primary button

Add check for filter/no data

1666331

Use EmptyStateNoData in collection documentation

e946bfa

Add title, description and button to instances of EmptyStateNoData

4d4350d

Remove Clear all filters from EmptyStateFilter component

4fa7fc4

himdel reviewed Feb 17, 2021

View reviewed changes

Change Unauthorised to Unauthorized

5478a45

ZitaNemeckova added 2 commits February 17, 2021 14:02

DRY empty state components

be4a5ec

Don't invent a wheel use lodash :)

5217795

himdel reviewed Feb 18, 2021

View reviewed changes

Fix typo notAutthorised -> notAuthorised

dd37ad1

ZitaNemeckova added 3 commits February 18, 2021 12:12

Use noData var to make coode more readable

8362c01

Use function

1485a82

Don't use redirect for two different things. Add extra var unauthoriz…

a6c29e3

…ed instead.

ZitaNemeckova force-pushed the empty_states branch from 1b9bdc8 to a6c29e3 Compare February 18, 2021 16:08

himdel reviewed Feb 19, 2021

View reviewed changes

Fix My namespaces error that appeared on load

ba2fe28

Use unauthorized in certification-dashboard

9d78450

himdel approved these changes Feb 19, 2021

View reviewed changes

ZitaNemeckova merged commit a0bd5fd into ansible:master Feb 19, 2021

ZitaNemeckova deleted the empty_states branch February 19, 2021 14:02

ZitaNemeckova added the backport-4.2 This PR should be backported to stable-4.2 (1.2) label Feb 19, 2021

ZitaNemeckova removed the backport-4.2 This PR should be backported to stable-4.2 (1.2) label Apr 9, 2021

ekmixon mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities ekmixon/ansible-hub-ui#45

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Empty state components and use them #285

Add Empty state components and use them #285

ZitaNemeckova commented Jan 4, 2021 •

edited

Loading

ZitaNemeckova commented Jan 26, 2021

sbuenafe-rh commented Jan 26, 2021

sbuenafe-rh commented Jan 26, 2021

fionayhlin commented Jan 26, 2021

sbuenafe-rh commented Jan 26, 2021

ZitaNemeckova commented Feb 3, 2021

ZitaNemeckova commented Feb 3, 2021

himdel commented Feb 4, 2021 •

edited

Loading

himdel Feb 4, 2021 •

edited

Loading

chouseknecht commented Feb 4, 2021

ZitaNemeckova commented Feb 8, 2021

fionayhlin commented Feb 8, 2021

ZitaNemeckova commented Feb 11, 2021

ZitaNemeckova commented Feb 15, 2021

himdel Feb 17, 2021

himdel commented Feb 17, 2021

himdel Feb 17, 2021

ZitaNemeckova Feb 17, 2021

ZitaNemeckova commented Feb 17, 2021

ZitaNemeckova commented Feb 17, 2021

himdel Feb 18, 2021 •

edited

Loading

ZitaNemeckova Feb 18, 2021

himdel Feb 18, 2021

ZitaNemeckova Feb 18, 2021

himdel Feb 18, 2021 •

edited

Loading

ZitaNemeckova Feb 18, 2021

himdel commented Feb 18, 2021 •

edited

Loading

ZitaNemeckova commented Feb 18, 2021

himdel Feb 19, 2021 •

edited

Loading

ZitaNemeckova Feb 19, 2021

himdel commented Feb 19, 2021 •

edited

Loading

ZitaNemeckova commented Feb 19, 2021

himdel left a comment

		@@ -0,0 +1,12 @@
		// Checks that at least one filter is set
		export function filterIsSet(params, filters) {

Add Empty state components and use them #285

Add Empty state components and use them #285

Conversation

ZitaNemeckova commented Jan 4, 2021 • edited Loading

ZitaNemeckova commented Jan 26, 2021

sbuenafe-rh commented Jan 26, 2021

sbuenafe-rh commented Jan 26, 2021

fionayhlin commented Jan 26, 2021

sbuenafe-rh commented Jan 26, 2021

ZitaNemeckova commented Feb 3, 2021

ZitaNemeckova commented Feb 3, 2021

himdel commented Feb 4, 2021 • edited Loading

himdel Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

chouseknecht commented Feb 4, 2021

ZitaNemeckova commented Feb 8, 2021

fionayhlin commented Feb 8, 2021

ZitaNemeckova commented Feb 11, 2021

ZitaNemeckova commented Feb 15, 2021

Choose a reason for hiding this comment

himdel commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZitaNemeckova commented Feb 17, 2021

ZitaNemeckova commented Feb 17, 2021

himdel Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Feb 18, 2021 • edited Loading

ZitaNemeckova commented Feb 18, 2021

himdel Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented Feb 19, 2021 • edited Loading

ZitaNemeckova commented Feb 19, 2021

himdel left a comment

Choose a reason for hiding this comment

ZitaNemeckova commented Jan 4, 2021 •

edited

Loading

himdel commented Feb 4, 2021 •

edited

Loading

himdel Feb 4, 2021 •

edited

Loading

himdel Feb 18, 2021 •

edited

Loading

himdel Feb 18, 2021 •

edited

Loading

himdel commented Feb 18, 2021 •

edited

Loading

himdel Feb 19, 2021 •

edited

Loading

himdel commented Feb 19, 2021 •

edited

Loading