New Block: Icon Block #71227
New Block: Icon Block #71227
Conversation
ryanwelcher
added
New Block
|
The PR is very much a draft that is essentially the source code from @ndiego plugin with changes to work as a core block, pass linting, and display the icons mentioned here. |
|
Thanks for doing the heavy lifting here @ryanwelcher. Excited to see this come to Gutenberg |
|
Great work, @ryanwelcher and @ndiego! Excited to dig into this. Happy to provide any resources, guidance, or context I can to help get this across the finish line. |
ryanwelcher
removed
[Type] Enhancement
… block implementation.
|
One thing that I have been thinking about how to handle SVG uploads. To be clear, I don't think we should enable SVG uploads in core - that's another conversation entirely and beyond the scope of this work. That being said, the existing code from the Icon Block plugin does a mime type check to see if SVG uploads have been enabled and provides the UI to upload them. IMO, this is a big win for extenders and I would love to see it stay in the block. |
|
Size Change: +34.1 kB (+1.75%) Total Size: 1.98 MB
ℹ️ View Unchanged
|
github-project-automation
bot
moved this to 🔎 Needs Review
in WordPress 6.9 Editor Tasks
|
Thanks for the PR! My biggest concern is that the block is very fragile because the save function relies on the
I don't have any good ideas right now, but I think you need to make it more robust, at least when it comes to the save function. Another concern is that we might have too many features. These features might be useful in third-party plugins, but we're not sure if they're all necessary for core. Personally, I prefer to ship something small and robust first. After shipping, we can evaluate each feature and continue enhancing it. |
|
I agree, that this does look a bit too fully featured for a first iteration. Would it be possible to slim it down to a more basic version? Also I think once the icon is saved, we should break the connection to the library, so that if the library changes the save function doesn't throw an error. |
I looked into this a bit. The ErrorModifying icon files in The reason this is a concern is that it's possible that icons in the package will receive design updates (tweaks) in future. This would cause errors as it stands meaning we need to address this prior to merge. How Block Validation WorksWhen the editor loads, Gutenberg's block validation system:
Root CauseThe icon block's printedIcon = namedIcon[ 0 ]?.icon; // Gets current icon from @wordpress/iconsThe timeline:
FixWe could try:
|
There was a problem hiding this comment.
If I were to slim this down I'd be looking at creating a version that just allows uploading custom SVG markup directly.
This can then be built upon to add the more advanced functionality in the current version.
@ryanwelcher Do you think you'd be open to doing that? Once the simple version is merged we can raise a followup in which we can look to resolve the issue with the icon picker.
|
Thanks everyone for your feedback! This is a pretty full-featured block and I agree that scaling it down might be the best solution - especially if we are aiming for it to be in 6.9.
My only concern with this is that is requires the user to have access to a list of SVG markup making this block not very useful to those that don't or don't know what SVG markup is. It seems to me that this block needs to provide some default icons.
I think that the most basic version of this a predefined list of icons that the user can choose from (along with some filters for extenders to introduce more) and the ability to upload markup. It seems like the best path forward (this is strong opinion, not strongly held) is to fix the issue with changes to the existing icons as providing those icons would be the most MVP that we can get. I am 1000% open to suggestions/comments or people just not liking this approach :) |
|
Digging into this a little further and it might make sense for an MVP to follow the prior art of the Social Link block. It's a dynamic block that stores the SVGs in PHP as well as in JavaScript. This does introduce the burden of SVG duplication but as we're starting with a subset of the Proposed Changes:
|
|
Thanks for picking this up again.
Yes it does. I wonder if we could update the I suspect that "icon packs" will need the same thing in future (i.e. exposing as JS and PHP). Do we know who is responsible for maintaining
I think this is a must have so as not to have these icons get outdated over time. |
… it in the edit state as well.
|
@getdave I was able to sever the connection between the icons and the library after the block is saved. This means that the icon is saved as is and any changes to the associated icon will not trigger a block validation error and display the icon that was saved in the edit. I think we can move forward with keeping the block static. |
|
I’m not sure if this is relevant for the MVP of this block, but when I used Nick’s block in the past and extended it with filters to use the Phosphoricons (an icon pack with around 1500 icons), I experienced performance issues in the editor because all icons were loaded through the large JS file and are all loaded at once. Would it be possible to load the icons via an endpoint instead, so that not all icons are loaded at once? This would also make it easier to extend the list of icons later. Does that make sense? |
This makes sense to me. Furthermore, and this is just a thought, I wonder if the design approach used in block patterns can be applied to icons as well.
|
|
I also noticed that the outermost prefix is still frequently used, e.g., as ClassName “wp-block-outermost-icon-block.” Should it be called “wp-block-icon-block” instead? |
|
What do you think to add a way to change the tagName ( |
This is a great outline on how to approach managing the icons but I would say that it's most likely not attainable for the MVP hoping to be shipped in 6.9. I would advocate for getting the block to a place where it can be included and then starting on how to manage the icons. To that end, @t-hamano, @scruffian, and @getdave have all mentioned that the block is too full-featured for an MVP. I would like to understand what we think needs to be removed before this can considered ready? |
|
To ensure the Icon Block seamlessly integrates with future infrastructure, here are my personal thoughts. Please let me know what you think!
|
This does mean that we'll need to convert the block from static to dynamic. I was able to disconnect the save from the list of icons but perhaps the dynamic approach is more scalable?
Great ideas overall, I can start implementing this today. |
If we consider icon SVG data as content, it would be best to store it in the database, i.e. as a static block. However, to my knowledge, there is currently no way to build it as a static block without destroying the block 🤔 |
I was able to get this working as a static block by disconnecting the icon from the save. I'll work on the other items and we can take another look at this after. |
What?
Closes #16484
This pull request adds a an icon block based on @ndiego Icon Block.
To be completed:
html-react-parserlibrary. Is this something we want to add to the project?