FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(emptyState): Support primary and icon as div elements by boaz0 · Pull Request #2957 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

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

Merged
tlabaj merged 1 commit into
patternfly:masterfrom
boaz0:closes_2929
Sep 25, 2019
ConversationCommits (1)ChecksFiles changed
Merged

feat(emptyState): Support primary and icon as div elements#2957
tlabaj merged 1 commit into
patternfly:masterfrom
boaz0:closes_2929

Conversation

boaz0 commented Sep 19, 2019

Copy link
Copy Markdown
Member

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: feat(tables): support height auto modifier in tr #2932
  3. I am waiting on patternfly core to be upgraded to 2.31.9 (please ignore the second commit).

karelhala reviewed Sep 19, 2019
View reviewed changes

karelhala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tlabaj commented Sep 19, 2019

Copy link
Copy Markdown
Contributor

@boaz0 the patternfly core package has been updated.

tlabaj requested a review from jschuler September 19, 2019 18:20
tlabaj requested review from mcarrano and mcoker September 19, 2019 20:58

boaz0 commented Sep 20, 2019

Copy link
Copy Markdown
Member Author

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

patternfly-build commented Sep 20, 2019

Copy link
Copy Markdown
Collaborator

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

karelhala previously approved these changes Sep 20, 2019
View reviewed changes

boaz0 commented Sep 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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

karelhala suggested changes Sep 23, 2019
View reviewed changes

karelhala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

boaz0 commented Sep 23, 2019

Copy link
Copy Markdown
Member Author

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

karelhala previously approved these changes Sep 23, 2019
View reviewed changes

mcoker commented Sep 23, 2019

Copy link
Copy Markdown
Contributor

@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.

feat(emptyState): support primary and icon as div elements
cf5e675 
Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

boaz0 commented Sep 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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?

mcarrano approved these changes Sep 24, 2019
View reviewed changes

mcarrano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good to me!

mcoker approved these changes Sep 24, 2019
View reviewed changes

mcoker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm, thanks @boaz0!!

tlabaj approved these changes Sep 25, 2019
View reviewed changes

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

tlabaj merged commit 9bb3962 into patternfly:master Sep 25, 2019

patternfly-build commented Sep 25, 2019

Copy link
Copy Markdown
Collaborator

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! 🎉

boaz0 deleted the closes_2929 branch April 24, 2020 06:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcoker mcoker approved these changes
kmcfaul kmcfaul approved these changes
jschuler Awaiting requested review from jschuler
+3 more reviewers
karelhala karelhala approved these changes
mcarrano mcarrano approved these changes
tlabaj tlabaj approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

jschuler

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Empty state icon and primary button updates

8 participants

Back | FazBrowse Home | New Git URL