-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Created a new package containing all icons #9648
Conversation
This will help with reusability and maintainability.
Nice! Notably, there are currently some icons missing from the embed block. (The generic ones for brands without a unique icon and the brand-specific ones.) Should brand icons be included in this package or in a separate package? |
I was thinking on placing those at the bottom as well. Separated with a comment indicating they are used by the embed block. |
Thanks for the PR @caxco93! I get the theory and reasoning here, but I'm not sure this is the best solution. This would effectively be forking and maintaining our own copy of Material Design Icons (albeit a small subset), which seems potentially complicated. If the ultimate goal is that the icons inside block placeholders reflect the block icons, perhaps there's a way that blocks could access their own properties — including the icon — from inside the block? That would allow you to reference something like |
@chrisvanpatten I did something like that in #9646. |
This package would let other users reuse these icons, just like they are already used to reusing Dashicons. Also it would be very easy to, in the future, add a custom SVG icon and have it be reusable everywhere else in Gutenberg. I'd like more opinions on this |
@@ -5,6 +5,7 @@ import { __ } from '@wordpress/i18n'; | |||
import { RichText } from '@wordpress/editor'; | |||
import { createBlock } from '@wordpress/blocks'; | |||
import { createBlobURL } from '@wordpress/blob'; | |||
import { video as videoIcon } from '@wordpress/icons'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably do this a bit differently:
import { video as icon } from '@wordpress/icons';
and then later in the file be able to use icon,
instead of icon: videoIcon
.
@caxco93 just as some background, the switch to raw SVGs was deliberate. The approach with Dashicons is considered a fallback at this point. There's some more info about the change in #8916. To be clear I'm not the final arbiter on whether this is a good or bad approach, just want to provide some context on why raw SVGs were chosen over an icon library approach. I also think that if this type of approach is preferred (pulling in the SVG component from a library), it may make long term maintenance easier to use an existing Material Design component library (like this one or this one) rather than maintaining our own fork of the icons. That said it's not up to me, and I'm very interested to hear other feedback :) |
Related discussions:
|
Discussed in this week's #core-js meeting (link requires registration): https://wordpress.slack.com/archives/C5UNMSU4R/p1549981547059600 |
As mentioned by @paaljoachim, @senadir is working on the same task in #17055. It looks like he got to the more advanced stage but there is still a lot of concerns in his proposal. I suggest we focus on the more up to date proposal and close this PR to ensure we have one place to continue the discussion. |
Description
[IN PROGRESS] [NEEDS FEEDBACK]
Creates a new package called
@wordpress/icons
containing all new svg icons for reusability and maintainability.This will help close #9642 and others in the future more easily
Fixes #9647
How has this been tested?
Check if the blocks show the correct icon
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: