patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

feat(Icon): add Icon component

Open kmcfaul opened this issue 3 years ago • 4 comments

What: Closes #7779

Adds the Icon component, examples and some unit tests.

kmcfaul avatar Sep 23 '22 17:09 kmcfaul

The inline example does not currently fully match core and is simpler. Let me know if the core example is preferable and I can update it.

There is also a slight spacing differences between core and react that I can't really determine the source of. There isn't any extra padding in core that I can see but the core items have a separation between the flex icon containers.

kmcfaul avatar Sep 23 '22 17:09 kmcfaul

Preview: https://patternfly-react-pr-8050.surge.sh

A11y report: https://patternfly-react-pr-8050-a11y.surge.sh

patternfly-build avatar Sep 23 '22 18:09 patternfly-build

I will defer to @mcarrano on whether to match the inline example. I feel like why not just match if it's relatively painless to do, just based on the idea that the more they match the better I guess? I thought that was something we were trying to do? But don't feel super strongly. Looks good otherwise!!

mmenestr avatar Sep 23 '22 20:09 mmenestr

It can be added to org at any point, since the core examples have already been released. I believe I had raised my hand to volunteer adding summaries to new components at the same point in the release process that I generate screenshots for full page examples. I am happy to do that. But also, here is a PR of me taking a stab at adding the summary right now.

nicolethoen avatar Sep 24 '22 00:09 nicolethoen

@tlabaj I am a little confused about the combination of the isInProgress and progressIcon properties as well. The way it should be working is that when the isInProgress modifier is present, the progressIcon replaces the child icon. The example you're referencing actually is two different Icon components, one with this modifier and one without it.

kmcfaul avatar Sep 29 '22 18:09 kmcfaul

@tlabaj I am a little confused about the combination of the isInProgress and progressIcon properties as well. The way it should be working is that when the isInProgress modifier is present, the progressIcon replaces the child icon. The example you're referencing actually is two different Icon components, one with this modifier and one without it.

Ok, so now I see what is going on. I think what you want to do is only have one Icon component in your example and have a way to toggle inProgress. I may have some suggestions for API, but I need more info.

@mcoker @srambach I have a couple of questions:

  1. Is there a reason the spinner and the icon have to render at the same time Link in the core example? is it to maintain space when the icon is swapped dynamically with the spinner?

  2. Could the progress be something other than the spinner?

tlabaj avatar Sep 29 '22 19:09 tlabaj

@tlabaj

Is there a reason the spinner and the icon have to render at the same time Link in the core example? is it to maintain space when the icon is swapped dynamically with the spinner?

Yep exactly, the spinner and icon may be different sizes and the goal is to have the spinner and icon swap without shifting anything around it if the size changes. When something is in progress, we visually hide the icon used, but allow it to maintain its layout/space in the page, then we absolutely position the spinner in its place (absolutely positioned things do not take up layout/space in the page).

Could the progress be something other than the spinner?

Yep, we aren't very opinionated about what goes in there. The only opinion we have about the spinner in the icon's CSS is when the spinner is in an inline icon - then we set the spinner's diameter to 1em. But we can do away with that CSS when the inline variation of the spinner is added to the react component.

mcoker avatar Sep 30 '22 18:09 mcoker

Your changes have been released in:

Thanks for your contribution! :tada:

patternfly-build avatar Oct 04 '22 15:10 patternfly-build