Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(emptyState): Support primary and icon as div elements #2957

Merged
merged 1 commit into from Sep 25, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Sep 19, 2019

What:

closes #2929

I wanted to add the demos (loading state table and empty state table) but I couldn't because:

  1. I don't have the spinner component in react-core
  2. They're relayed on the height-auto modifier which isn't in master yet: #2932
  3. I am waiting on patternfly core to be upgraded to 2.31.9 (please ignore the second commit).
Copy link
Contributor

karelhala left a comment

Is there any reason why you changed single quotes to double quotes? Other than that, it looks good!

@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Sep 19, 2019

@boaz0 the patternfly core package has been updated.

@tlabaj tlabaj requested a review from jschuler Sep 19, 2019
@tlabaj tlabaj requested review from mcarrano and mcoker Sep 19, 2019
@boaz0 boaz0 force-pushed the boaz0:closes_2929 branch 2 times, most recently from dda1afc to c00ab9d Sep 20, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 20, 2019

@karelhala 😱 for some reason my vim-prettier got crazy.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 20, 2019

PatternFly-React preview: https://patternfly-react-pr-2957.surge.sh

@boaz0 boaz0 force-pushed the boaz0:closes_2929 branch 2 times, most recently from b0ae031 to 9ff2893 Sep 22, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 23, 2019

@mcoker ok I updated the PR, I also temporarily (just for the sake of this review) added the loading state and the no-match state to the examples.

Also, I am assuming that the component that will be wrapped inside the div will get the same props: ('size', 'color' and 'title') as the icon component. Can I assume that or we're not sure how it's going to look like.

At the worst case, we may create a new component that will support both wrapping general components whether it's a general React component or an Patternfly Icon component

Copy link
Contributor

karelhala left a comment

Looks like vim-prettier got crazy again and changed single quotes to double quotes. Other than that still looking fine.

@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 23, 2019

Thanks @karelhala I guess vim-prettier doesn't love me 😭 - I am going to turn it off. May it rest in peace 🙏

@boaz0 boaz0 force-pushed the boaz0:closes_2929 branch from 9ff2893 to 74875c1 Sep 23, 2019
@mcoker

This comment has been minimized.

Copy link
Contributor

mcoker commented Sep 23, 2019

@boaz0 thanks! My expectation is that if pf-c-empty-state__icon is used as a container, you can put whatever you want in it and color, size, title, etc no longer apply since it's now a <div> and not an <svg>. I would expect it to work similarly to pf-c-empty-state__primary.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:closes_2929 branch from 74875c1 to cf5e675 Sep 24, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Sep 24, 2019

@mcoker thanks.
to make this backward compatible I set a new prop that is only used in the container variant.

We should figure out a better API (probably will introduce a breaking change) to support one simple prop for both icon and component under the empty state icon.

@karelhala what do you think? have ideas?

Copy link
Member

mcarrano left a comment

Looks good to me!

@mcarrano mcarrano added ux approved and removed ux review labels Sep 24, 2019
@mcoker
mcoker approved these changes Sep 24, 2019
Copy link
Contributor

mcoker left a comment

lgtm, thanks @boaz0!!

@tlabaj
tlabaj approved these changes Sep 25, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 9bb3962 into patternfly:master Sep 25, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 25, 2019

Your changes have been released in:

  • @patternfly/react-core@3.110.0
  • @patternfly/react-docs@4.13.24
  • @patternfly/react-inline-edit-extension@2.11.60
  • demo-app-ts@3.6.1
  • @patternfly/react-table@2.22.9
  • @patternfly/react-topology@2.8.56
  • @patternfly/react-virtualized-extension@1.2.46

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.