Fixes #24475 - Add EmptyState to React components #5850

boaz0 · 2018-07-18T16:11:46Z

This is part of #5755 - Adding Table React components to Hardware Models.
EmptyState is used in the Table component and since it didn't have tests or storybook in Katello I decided to push it as its own PR.

This PR introduces two EmptyState components - DefaultEmptyState which is what we are using in Foreman and EmptyStatePattern which allows to customize the empty state component (actually DefaultEmptyState is based on top of it).

If you would like to play with this a little bit the storybook can be found here

PRs that are related: #5755 and #5024.

PatternFly React

I saw that @sharvit asked to push EmptyStatePattern to pf-react, which is similar to this PR (read more here). Between those two PRs there are nuances but the major difference is that in this PR the primary action and the secondary actions can be rendered in any way you would like (it doesn't have to be a button). This is to allow other people to set their own action-buttons/actions part.

Katello

Katello is using a very similar EmptyState component. The only big difference that I see is that the action buttons are wrapped inside LinkContainer in order to interact with react-router.

Therefore, in order to use this component in Katello use EmptyStatePattern and pass a function that renders the buttons from title and url. Something like this:

const katelloAction = ({title, url}) => (
  <LinkContainer to={action.url}>
    <Button href={action.url} bsStyle="primary" bsSize="large">
      {action.title}
    </Button>
  </LinkContainer>
)

const {action ..} = props
return <EmptyStatePattern
  ...
  action={katelloAction(action)}
  secondaryAction={secondaryActions.map(action => katelloAction(action))}
/>

//cc @ohadlevy @sharvit @amirfefer @tstrachota @waldenraines

houndci-bot · 2018-07-18T16:13:31Z

webpack/assets/javascripts/react_app/components/common/emptyState/emptyState.test.js

@@ -0,0 +1,67 @@
+import React from 'react';
+import DefaultEmptyState, { EmptyStatePattern } from './index';
+import { testComponentSnapshotsWithFixtures } from '../../../common/testHelpers';


'toJson' is defined but never used no-unused-vars

houndci-bot · 2018-07-18T16:13:32Z

webpack/assets/javascripts/react_app/components/common/emptyState/emptyState.test.js

@@ -0,0 +1,67 @@
+import React from 'react';
+import DefaultEmptyState, { EmptyStatePattern } from './index';


'shallow' is defined but never used no-unused-vars

ohadlevy · 2018-07-18T19:14:30Z

webpack/assets/javascripts/react_app/components/common/emptyState/index.js

+  </Button>
+);
+
+const documentationBlock = ({


see https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/components/common/DocumentationLink/index.js, maybe worth refactoring / extracting

Thanks @ohadlevy.
I uploaded here the storybook with EmptyState and DocumentationLink.

What bothers me is that DocumentationLink has the bullet in the beginning of the link. Is that supposed to be there?

it is used today in the bookmarks dropdown, so I assume that needs to be extracted from the component and wrapped above it in bookmarks...?

Yeah, I think if I move it from the <MenuItem> it will look good. Let me check it out

Ping @ohadlevy - here is the storybook with what you suggested. I will upload the changes if it looks fine to you

ohadlevy · 2018-07-19T08:11:34Z

webpack/assets/javascripts/react_app/components/common/emptyState/index.js

+import PropTypes from 'prop-types';
+import { EmptyState as PfEmptyState, Button } from 'patternfly-react';
+
+const secondaryActionButtons = (actions = []) =>


does it make sense to create a component for each one of those? it can in theory make the snapshots smaller?

Do you mean something like:

const ActionButton = ({action: {title, url}, ...props}) => <Button href={url}>{title}</Button>; const PrimaryActionButton = (action) => <ActionButton action={action} bsSize="large" bsStyle="primary" /> const SecondaryActionButtons = (actions = []) => actions.map(action => <ActionButton action={action} />)

boaz0 · 2018-07-25T12:27:09Z

@ohadlevy I updated the code and the storybook:

Default Empty State is using DocumentLinkContent
DocumentationLink is using DocumentLinkContent too
You can import DocumentLinkContent and use it inside a component such as button

boaz0 · 2018-07-25T16:16:36Z

@ohadlevy I have no idea why but when I am using the __ function outside a React component (f.e. in a defaultProps) tests fail:

ReferenceError: Can't find variable: __
ReferenceError: Can't find variable: __
    at http://127.0.0.1:36453/webpack/bundle-bbfc6206aec26c54f74a.js:1 in ./webpack/assets/javascripts/react_app/components/common/DocumentationLink/index.js
    at http://127.0.0.1:36453/webpack/vendor-255a8802f7a0349e7379.js:1 in a
    at http://127.0.0.1:36453/webpack/bundle-bbfc6206aec26c54f74a.js:1 in ./webpack/assets/javascripts/react_app/components/bookmarks/index.js
    at http://127.0.0.1:36453/webpack/vendor-255a8802f7a0349e7379.js:1 in a
    at http://127.0.0.1:36453/webpack/bundle-bbfc6206aec26c54f74a.js:1 in ./webpack/assets/javascripts/react_app/components/componentRegistry.js
    at http://127.0.0.1:36453/webpack/vendor-255a8802f7a0349e7379.js:1 in a
    at http://127.0.0.1:36453/webpack/bundle-bbfc6206aec26c54f74a.js:1 in ./webpack/assets/javascripts/react_app/common/MountingService.js
    at http://127.0.0.1:36453/webpack/vendor-255a8802f7a0349e7379.js:1 in a
    at http://127.0.0.1:36453/webpack/bundle-bbfc6206aec26c54f74a.js:1 in ./webpack/assets/javascripts/bundle.js
    at http://127.0.0.1:36453/webpack/vendor-255a8802f7a0349e7379.js:1 in a
    at http://127.0.0.1:36453/webpack/vendor-255a8802f7a0349e7379.js:1 in webpackJsonp
ReferenceError: Can't find variable: tfm
ReferenceError: Can't find variable: tfm
    at http://127.0.0.1:36453/assets/application-6496916c7a22421384ad3711aebc5b3026d4e153555adc51af5d7700547476d9.js:21485 in global code
ReferenceError: Can't find variable: tfm
ReferenceError: Can't find variable: tfm
    at http://127.0.0.1:36453/about:104 in global code
ReferenceError: Can't find variable: tfm
ReferenceError: Can't find variable: tfm
    at http://127.0.0.1:36453/about:307 in global code
ReferenceError: Can't find variable: tfm
ReferenceError: Can't find variable: tfm
    at http://127.0.0.1:36453/about:318 in global code
    test/integration/about_test.rb:31:in `block in <class:AboutIntegrationTest>'

ohadlevy · 2018-07-26T07:12:32Z

@ripcurld does window.__ makes any difference? is it during tests (I guess not as i see webpack in your output) also, error about tfm indicate it failed to compile it?

ohadlevy · 2018-07-26T07:12:49Z

@amirfefer can you review please? I remember you did something similar in the past?

boaz0 · 2018-07-26T10:42:13Z

@ohadlevy nope window.__ didn't help. It's not only during tests but when going to any page (Hardware Models, Operating Systems, Hosts) bookmarks isn't displayed and a similar error message is displayed in the console log.

boaz0 · 2018-07-26T11:52:01Z

I found moved jed(i18n) to webpack. I will try to see if that helps here too.

Thanks @ohadlevy & @amirfefer

boaz0 · 2018-07-30T08:54:18Z

Update: @amirfefer's PR moved jed(i18n) to webpack does fix that problem.

amirfefer

Looks good @ripcurld !
few minor questions

amirfefer · 2018-07-31T08:50:21Z

webpack/assets/javascripts/react_app/components/common/DocumentationLink/index.js

+};
+
+DocumentationLink.defaultProps = {
+  id: 'documentationLink',


shouldn't it be hard coded? for instance if we have some css rules on this id, the consumer might break that.

Oops you're right. 😱

@amirfefer actually to my surprise BookmarkContainer is passing id to DocumentationLink.
I only noticed it when I ran tests on my computer.

and should the consumer be able to change the id?

Decided not to expose the "id" props as it's used for CSS/jQuery any where in the code base.

amirfefer · 2018-07-31T08:55:56Z

webpack/assets/javascripts/react_app/components/common/emptyState/index.js

+import { DocumentLinkContent } from '../DocumentationLink';
+
+const ActionButton = ({ url, children, ...props }) => (
+  <Button href={url} {...props}>


Looks like it only wraps a button component with no extra logic or extension, why not just use Button directly in secondaryActionButtons and primaryActionButton

amirfefer · 2018-07-31T08:59:03Z

...ets/javascripts/react_app/components/common/emptyState/__snapshots__/emptyState.test.js.snap

+  }
+  description="Printers print a file from the computer"
+  documentation={
+    <Unknown>


It's because I am using React.Fragment. Let me what I can do about it.

I noticed that this fixed in jest 23. For more information read this PR.

We currently are using jest v20.0.4 so I probably have to update the package, right? or is there a better way?

I tried to update to 23.0.0 and all tests were failing:

SecurityError: localStorage is not available for opaque origins at Window.get localStorage [as localStorage] (node_modules/jest-environment- jsdom/node_modules/jsdom/lib/jsdom/browser/Window.js:257:15) at Array.forEach (<anonymous>)

n/m need to add "testURL" to jest section in package.json in order to fix that

update and yet didn't help. I got many tests failing (some on snapshot and others because turbolink's visit is not called)... I will figure a different way. 😪

boaz0 · 2018-07-31T14:10:39Z

/ping @ohadlevy @amirfefer - ready for next iteration of reviews.

Thanks 😀

amirfefer · 2018-08-01T12:18:00Z

webpack/assets/javascripts/react_app/components/common/DocumentationLink/index.js

+};
+
+DocumentationLink.defaultProps = {
+  id: 'documentationLink',


and should the consumer be able to change the id?

boaz0 · 2018-08-01T13:34:26Z

webpack/assets/javascripts/react_app/components/common/emptyState/emptyState.test.js

+describe('Default Empty State', () => {
+  // React.Fragment is rendered as <Unknown>:
+  // https://github.com/facebook/jest/pull/5816
+  // To fix this, I am using TestRenderer.create.


BTW, I tried to mock React.Fragment using jest (mock and setMock) none was working for me.
In addition, I found TestRenderer much easier to follow and to test.

If someone is capable to mock React.Fragment please let me know. 🍡

amirfefer · 2018-08-01T15:34:14Z

webpack/assets/javascripts/react_app/components/bookmarks/__snapshots__/bookmarks.test.js.snap

                            </a>
                          </SafeAnchor>
                        </li>
                      </MenuItem>
-                    </Component>
+                    </DocumentationLink>
                    <MenuItem
                      bsClass="dropdown"
                      disabled={false}


nitpick: this is too large snapshot, is there a way to reduce its size? (divide it ?)

Will have a solution for that hopefully in 2 hours 👍

sharvit

Nice work @ripcurld, I just got one small comment.

sharvit · 2018-08-02T14:32:28Z

webpack/assets/javascripts/react_app/components/common/emptyState/index.js

@@ -0,0 +1,116 @@
+import React from 'react';


Could you change this file so it will contain single component per file?
e.g.

common/EmptyState/EmptyState.js

common/EmptyState/EmptyStatePattern.js

common/EmptyState/EmptyStatePropTypes.js

common/EmptyState/EmptyStateSecondaryActionButtons.js

common/EmptyState/EmptyStatePrimaryActionButton.js

And you can namely expose the relevant component from the common/EmptyState/index.js

No problem - you got it!

amirfefer

LGTM 👍

boaz0 · 2018-08-03T14:14:31Z

[test foreman]

sharvit

LGTM! Thanks @ripcurld 👍

boaz0 · 2018-08-06T10:04:24Z

@ohadlevy @sharvit @amirfefer I rebased (there was a conflict with #5872)

boaz0 · 2018-08-07T08:39:12Z

Rebased (due to #5921) ☘️

ohadlevy · 2018-08-07T08:48:29Z

webpack/assets/javascripts/react_app/components/common/EmptyState/DefaultEmptyState.js

+  label = __('For more information please see'),
+  buttonLabel = __('Documentation'),
+}) =>
+  (url ? (


nitpick: why not url && <...> instead ? e.g.

url && (<React.Fragment> {label}{' '} <a href={url} target="_blank"> <DocumentLinkContent>{buttonLabel}</DocumentLinkContent> </a> </React.Fragment>)

ohadlevy · 2018-08-07T08:49:45Z

looks good, one nitpick :)

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

boaz0 · 2018-08-07T12:31:56Z

[test upgrade]

ohadlevy · 2018-08-09T10:48:11Z

@sharvit & @amirfefer mind giving final ack? thanks

amirfefer

LGTM 👍
Thanks @ripcurld

sharvit

LGTM! Thanks @ripcurld

boaz0 · 2018-08-09T20:28:55Z

🎉 SWEET 🎉

houndci-bot reviewed Jul 18, 2018

View reviewed changes

ohadlevy reviewed Jul 18, 2018

View reviewed changes

ohadlevy reviewed Jul 19, 2018

View reviewed changes

boaz0 changed the title ~~[WIP] Fixes #0000 - Add EmptyState to React components~~ [WIP] Fixes #24475 - Add EmptyState to React components Jul 30, 2018

boaz0 changed the title ~~[WIP] Fixes #24475 - Add EmptyState to React components~~ Fixes #24475 - Add EmptyState to React components Jul 30, 2018

amirfefer suggested changes Jul 31, 2018

View reviewed changes

theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Waiting on contributor labels Jul 31, 2018

amirfefer suggested changes Aug 1, 2018

View reviewed changes

theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Needs re-review Waiting on contributor labels Aug 1, 2018

boaz0 commented Aug 1, 2018

View reviewed changes

amirfefer reviewed Aug 1, 2018

View reviewed changes

sharvit suggested changes Aug 2, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Aug 2, 2018

amirfefer previously approved these changes Aug 2, 2018

View reviewed changes

boaz0 dismissed amirfefer’s stale review via 7b95d49 August 3, 2018 09:25

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Aug 3, 2018

sharvit previously approved these changes Aug 6, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Aug 6, 2018

boaz0 dismissed sharvit’s stale review via acced1c August 6, 2018 09:58

ohadlevy reviewed Aug 7, 2018

View reviewed changes

Fixes #24475 - Add EmptyState to React components

67abfe5

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

ohadlevy approved these changes Aug 9, 2018

View reviewed changes

amirfefer approved these changes Aug 9, 2018

View reviewed changes

sharvit approved these changes Aug 9, 2018

View reviewed changes

ohadlevy merged commit 285fd58 into theforeman:develop Aug 9, 2018

boaz0 deleted the empty_state branch September 2, 2018 10:52

		@@ -0,0 +1,67 @@
		import React from 'react';
		import DefaultEmptyState, { EmptyStatePattern } from './index';

Fixes #24475 - Add EmptyState to React components #5850

Fixes #24475 - Add EmptyState to React components #5850

Conversation

boaz0 commented Jul 18, 2018 • edited Loading

PatternFly React

Katello

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

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

Choose a reason for hiding this comment

boaz0 commented Jul 25, 2018

boaz0 commented Jul 25, 2018

ohadlevy commented Jul 26, 2018

ohadlevy commented Jul 26, 2018

boaz0 commented Jul 26, 2018

boaz0 commented Jul 26, 2018

boaz0 commented Jul 30, 2018

amirfefer left a comment

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

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

Choose a reason for hiding this comment

boaz0 Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaz0 Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

boaz0 commented Jul 31, 2018

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

sharvit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer left a comment

Choose a reason for hiding this comment

boaz0 commented Aug 3, 2018

sharvit left a comment

Choose a reason for hiding this comment

boaz0 commented Aug 6, 2018

boaz0 commented Aug 7, 2018

Choose a reason for hiding this comment

ohadlevy commented Aug 7, 2018

boaz0 commented Aug 7, 2018

ohadlevy commented Aug 9, 2018

amirfefer left a comment

Choose a reason for hiding this comment

sharvit left a comment

Choose a reason for hiding this comment

boaz0 commented Aug 9, 2018

boaz0 commented Jul 18, 2018 •

edited

Loading

boaz0 Jul 31, 2018 •

edited

Loading

boaz0 Jul 31, 2018 •

edited

Loading