Components: Add next Icon component #28978
Conversation
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
sarayourfriend
added
[Feature] Component System
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
| // Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
|
||
| exports[`props should render correctly 1`] = ` | ||
| .emotion-0.emotion-0.emotion-0.emotion-0.emotion-0.emotion-0.emotion-0 { |
There was a problem hiding this comment.
Is this something that is applied for every component or it's only shared and can be removed from snapshots? I see it everywhere.
There was a problem hiding this comment.
I think these come from the base View component.
Any styles applied to the component will end up in emotion-0 as that's the deserialized name for the styles.
For example, if I had a snapshot test like this:
const { container } = render( <View css={ { width: '200px' } } /> );
expect( container.firstChild ).toMatchSnapshot();
The snapshot would include width: 200px in emotion-0.
Some of this will be cleaned up if we remove automatic prefix generation as suggested in #21745
|
@gziolo We should probably make sure this works with Dashicons some how before merging it 😀 |
|
@gziolo HaiiI!~ @sarayourfriend and I discussed this yesterday when we were pairing (🍐 ❤️ ). (Will try to talk through the scenario - it's a little tricky) The new ui/icon component is really meant to replace the one from From my understanding, the current The tricky thing is, we can't add the new Ideas?
Those are the best ideas I have for now? Would love your thoughts :) |
|
@youknowriad should have the best answer since he authored I don't know what role All icons should use gutenberg/packages/primitives/src/svg/index.js Lines 85 to 98 in 1d27652 |
| * @param {import('@wp-g2/create-styles').ViewOwnProps<Props, 'div'>} props | ||
| * @param {import('react').Ref<any>} forwardedRef | ||
| */ | ||
| function Icon( props, forwardedRef ) { |
There was a problem hiding this comment.
The "official" icon component now, is actually in @wordpress/icons and the idea was to deprecate the one in "components" at some point, this feels like it goes in the opposite direction, so we should give it some thoughts.
There was a problem hiding this comment.
in other words, why do we need an "Icon" component in "components" and why the one in @wordpress/icons is not sufficient?
There was a problem hiding this comment.
...why the one in @wordpress/icons is not sufficient?
@youknowriad Good question! The short answer is custom styling (or rather refinements).
From my experience, SVG-based icons usually require adjustments based on width, height, display (block or inline), and color (fill or path).
The current @wordpress/icon supports width and height, but are missing supprt for display + color.
To support this, custom styles have been added for the (Icon) svg selector, and is scattered throughout the .scss codebase:
https://github.com/WordPress/gutenberg/search?l=SCSS&p=1&q=svg
(Note: Most are color based)
The one from this PR addresses display, color, and is rendered with a wrapping <View />.
This wrapper helps control the layout rendering of the svg element, and offers improved style customization + stability (with the new styling system).
Hope this helps!
There was a problem hiding this comment.
The Icon component in the @wordpress/icons doesn't need to support color or display because the color is just the one that is actually just the current-color in CSS and display is more for its parent than the icon itself.
So instead of adding a 4th icon block and continue the fragmentation, I personally don't think it's worth it and we should just continue with the one we have.
There was a problem hiding this comment.
the color is just the one that is actually just the current-color in CSS
The tricky thing is the current @wordpress/icons doesn't support this, so fill: currentColor CSS is added in places that need it. We can enhance @wordpress/icons to have this style be default - although I'd be uncertain how it should be done (either with a new .scss file or inline styles).
display is more for its parent than the icon itself
That's true in some sense. But I think it's a common enough setup that it would be useful if supported by the component.
Taking a step back, the purpose of the component is to provide a nice "wrapper" for the svg icon.
Something that takes care of the stuff I had mentioned.
The best parallel example I can think of is how Github handles icons.
Octicons is their SVG icon library (https://primer.style/octicons/).
For their React component system, they have StyledOcticon, which is their "wrapper".
They do have a dedicated Icon renderer component within Octicon
https://github.com/primer/octicons/blob/74b249595db151ec361bcacfdca18215ae8cc020/docs/src/components/icon.js
(which interestingly has some layout opinions hard coded, like display: inline-block and vertical-align)
We could have a similar setup where we leave @wordpress/icon as is, and we create a styled wrapper.
However, that touches upon the concern you mentioned...
So instead of adding a 4th icon block
TLDR:
We need a reliable way to render icons within the new Component System.
The out-of-the-box <Icon /> component from @wordpress/icon is missing a couple of things.
We can create a styled wrapper - something that's used internally, and only at the framework level.
There was a problem hiding this comment.
What we need a StyledIcon instead of just View?
There was a problem hiding this comment.
@youknowriad The icon wrapper would take care of display, color, and user-select out of the box for the inner Icon. View is capable of doing this, but it doesn't these styles by default.
The custom styles can be found here:
https://github.com/ItsJonQ/g2/blob/main/packages/components/src/Icon/Icon.styles.js
There was a problem hiding this comment.
I guess I don't really understand why not just move these styles to Icon component in @wordpress/icons if they are mandatory.
I don't want to be a blocker, so feel free to move on if you think otherwise but for me there's a lot of unnecessary duplication in terms of dependencies and useless props/behaviors in these new components and my concern is making sure we catch these.
There was a problem hiding this comment.
I guess I don't really understand why not just move these styles to Icon component in @wordpress/icons if they are mandatory.
I think this is what we should do, ultimately replacing the existing Icon component in @wordpress/icons with the new one.
@ItsJonQ We'll need to brainstorm some way of doing this given the dependency on G2. We cannot do it just with inline styles, unfortunately, which might have simplified things.
For context: I was confused about the status of the Icon component in @wordpress/components which is why the adapter behaves the way it does and why the new Icon component lives where it does now.
I do not think we should go backwards but I think we should enhance the @wordpress/icons component with the new features that Q introduced in the CSS-in-JS one.
There was a problem hiding this comment.
@youknowriad + @sarayourfriend Thank you for your thoughts 🙏
We'll need to brainstorm some way of doing this given the dependency on G2. We cannot do it just with inline styles, unfortunately, which might have simplified things.
A part of me wishes we could do it with inline styles 😛 .
Ultimately, we want to provide Icon with some styles to make it easier to work with and more reliable to render (style wise).
The 3 choices we have are:
- Use inline styles
- Use CSS (via
.scss) - Use CSS-in-JS (Emotion or the new Style System)
Inline styles is the only option that does not require a workflow adjustment.
The trade-off is that customizing styles may be quite difficult.
CSS would require the workflow adjustment of stylesheet compile/export/import.
Not the worst thing in the world, as this is how we're currently setup.
CSS-in-JS would require adding a dependency to compile the styles.
This is the direction we're heading with the new Component System.
If we go wit the Style System CSS-in-JS solution, it would be good if we could create the packages. That way, the style system could be shared between @wordpress/icons and @wordpress/components.
Bringing over the style system was something briefly touch upon in this issue under "Backfilling"
#28399
For now, we may be able to work around this for the component integration work. However, this is something that we'll need to solve.
Description
Adds the next version of the
Iconcomponent. It's significantly simpler than the existing icon, but could be expanded slightly to allow for the sameDashiconstring interface that we have now, might be worth considering @ItsJonQ.Unfortunately the adapter is basically copied and pasted from the existing component. There's no way around this I fear as the new component only accepts a
ReactElementto pass tocloneElementas theiconargument. You can see in the story that I included this works fine for SVGs. I have not figured out how to get it work work forDashicons.How has this been tested?
Storybook
Types of changes
New
Iconcomponent inuiChecklist: