Icons: Redraw 35 prominent icons to strokes, and maintain absolute stroke-width#78808
Icons: Redraw 35 prominent icons to strokes, and maintain absolute stroke-width#78808jasmussen wants to merge 25 commits into
Conversation
jasmussen
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
A note that I forgot to mention: as I carefully hand-redraw these, some curves and vectors become more precisely positioned, clearly mistakes from previous iteration. In 95% of the cases in this batch, the end result when seen at 24x24 is invisible. However I want to point out a couple of icons were very slightly visually refreshed as part of this, in case that invites feedback. IMO, it's not enough to warrant a changelog entry beyond what this PR can benefit from regardless, but for your awareness:
|
|
Flaky tests detected in 932fcc0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27810898468
|
|
Wonderful! Since this involves redrawing icons, would it make sense to address some of the low-hanging fruit in #65786 as well? For instance applying endcap-roundness consistently, pixel grid alignment, and footprint consistency? |
|
Yes it does indeed! And notably the pixel grid alignment I've been fixing here. There's been a lot discovered. I've also unified a couple of circles that were off-size. I'm hesitant to do too much, but would be happy to follow up after we land a good new stroke based baseline. One really potent improvement as I do this: the new icons are subtantially easier to edit and work with, because what we used to call the "icon source", the stroke based version we would go ahead and convert to fills, that is now the icon that we ship, one and the same, so the Figma to GitHub flow is now very much improved. The short is, once this is done, it should be both easier to contribute, to edit, to consolidate, and all that. |
|
Yes, appreciate it's a balance; we want to avoid scope creep. I'm just aware that it might be easier to tweak these things in one go rather than sequentially, with no meaningful trade-offs that I can think of. One other thing that springs to mind from a previous exploration around converting the icons to be stroke-based... there is quite a bit of css in Gutenberg that targets icons with |
github-actions
Bot
added
[Package] Components
jasmussen
force-pushed
the
update/redraw-34-icons-to-stroke-based
branch
from
d336873 to
9c97f83
Compare
|
It's funny you should mention that, because in testing other things, I found issues with exactly that: 
This is now fixed, but I will write some additional detail on the fix in context of the code. 
|
|
|
||
| svg, | ||
| path { | ||
| svg { |
There was a problem hiding this comment.
This change was necessary because without it, everything gets a fill, including what is meant to just be a stroke based circle (with a hole in the middle).
I prefer to keep PRs minimal, but this one was necessary in order for this PR to be possible to land. I have some additional changes I want to push—a normalisation script and some more resilience, but this was a minimal change I was able to do. I wasn't able to find any ill consequences from this change, but CC'ing some component experts for a gut check: @mirka @ciampo
There was a problem hiding this comment.
I trust @ciampo's judgment here but just wanted to confirm that this has been tested and verified to not cause any unexpected consequences.
There was a problem hiding this comment.
To expand a bit:
All in-repo usage shouldn't be affected, because @wordpress/icons icons are either:
- Single filled path (with no associated
fillrule), where the childpathwill nherit thefillrule from the parentsvganyway; - Stroke-based with explicit
fill: noneas an inline style, which takes precedence
I scanned the codebase for all usages of ItemGroup Item + SVGs and all first-party usage should be good (still better to run smoke tests, though).
The risk for visual regressions exists but is limited to third-party usage, in case the third-party icon relied on that path { fill: currentColor } rule that is being removed here. In particular, I believe the main sources where this regression could manifest are:
- The block list in the Global Styles screen (for third party blocks);
- The sidebar navigation of the recently added
bootpackage, for consumer-registered icons
I think we should be good with a CHANGELOG entry. The regression in this case will be purely visual and won't prevent users from interacting with the Gutenberg app.
| <HStack justify="flex-start"> | ||
| { icon && ( | ||
| <Icon | ||
| style={ { fill: 'currentcolor' } } |
There was a problem hiding this comment.
This rule overrode the SVG that was inserted, and forced a fill rule on it, making it impossible to use stroke-based icons.
| return `${ camelKey }: '${ value }'`; | ||
| } ) | ||
| .join( ', ' ); | ||
| return ` style={ { ${ declarations } } }`; |
There was a problem hiding this comment.
This change was required so the inline style="fill: none" on stroke icons gets translated to React's object form during build. Without it, React ignores the style and the icons render filled.
Claude Code helped me with this, btw.
|
Size Change: +1.91 kB (+0.02%) Total Size: 8.6 MB 📦 View Changed
|
| @@ -1,3 +1,3 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"> | |||
| <path d="M18.5 5.5V8H20V5.5h2.5V4H20V1.5h-1.5V4H16v1.5h2.5zM12 4H6a2 2 0 00-2 2v12a2 2 0 002 2h12a2 2 0 002-2v-6h-1.5v6a.5.5 0 01-.5.5H6a.5.5 0 01-.5-.5V6a.5.5 0 01.5-.5h6V4z" /> | |||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" style="fill: none" stroke="currentColor" stroke-width="1.5"> | |||
There was a problem hiding this comment.
We're now adding a lot of explicit currentColor in the icons themselves. This is going to result in inconsistent behavior across icons.
Here's the icon library in a color: red context:
| Before | After |
|---|---|
 |
 |
Unlike Icon in either wp-components or wp-icons, the new Icon from wp-ui applies a fill="currentColor" by default. Here's the icon library modified to use the wp-ui version:
If we're going with these stroke-based icons, I think we're probably forced to add explicit currentColor in the icons themselves, for all the icons, not just the ones that have been redrawn here.
There was a problem hiding this comment.
If we're going with these stroke-based icons, I think we're probably forced to add explicit currentColor in the icons themselves, for all the icons, not just the ones that have been redrawn here.
I volunteer to do that 🙋♂️
Before I do, I want to just check with you if you're okay with this? Is it functionally a good choice? IMO yes as this is the implied behavior of the icon set and it's worth making explicit. I also expect to follow up with a PR that adds a script to do this on every build of the set.
There was a problem hiding this comment.
I think it's a good choice. It just has the potential to introduce unexpected visual glitches for consumers receiving this update, but it's probably for the best in the long term and it alings the current behaviour to the new behaviour that @wordpress/ui's Icon component already applies
There was a problem hiding this comment.
It will also expose a cleaner way to change the icon color: ie. via CSS color, rather than guessing if fill or stroke should be used
There was a problem hiding this comment.
I agree. I think it's ultimately a tech enhancement that opens up new opportunities.
In that vein, what are some action items for this PR? Do you think we can land this one as-is, or do I need to include all the icons in one go?
There was a problem hiding this comment.
Probably best not to increase the scope of this PR further, but we should definitely aim to have a coherent behavior for all icons within the same WordPress release (and possible even Gutenberg release)
There was a problem hiding this comment.
The icons are easy to redraw, I can do it in days. The problem is this first PR, I'm reluctant to redraw ALL the icons in this PR, if there's a risk this PR doesn't land.
A note, I will be AFK for the summer vacation starting Tuesday, so if something has to happen for 7.1, it has to be this week. I don't think it has to be a 7.1 thing, but sharing that context for clarity regardless.
There was a problem hiding this comment.
I'd say that "adding explicit currentColor to all icons" is a good thing to do, regardless of this redrawing a subset of the icons to strokes.
Still, if we want to keep PRs as small as possible, I think the order needs to be:
- Add
currentColorto all the icons. - Redraw a subset of the icons to strokes.
Otherwise, landing the stroke redraw first will get the @wordpress/icons package in a state where some icons get currentColor and some do not. Since the @wordpress/icons is a bundled package and not tied to WordPress/Gutenberg releases, the only thing that matters is the package release. We'll want to avoid any situation where there could be a release of the icons package in a mixed color state.
jasmussen
force-pushed
the
update/redraw-34-icons-to-stroke-based
branch
2 times, most recently
from
3568808 to
5d10653
Compare
Even with `style` in the kses allowlist, wp_kses filters the attribute value through safecss_filter_attr(), which drops CSS properties not on its allowlist. SVG presentation properties like `fill` are not allowed, so `style="fill: none"` was emptied and removed during sanitization, leaving stroke-based icons with a solid default fill. Temporarily register a safe_style_css filter while sanitizing to allow the SVG presentation properties (fill, stroke, stroke-width, stroke-linecap, stroke-linejoin, stroke-miterlimit) in both the compat base class and the Gutenberg subclass override. Additionally, the icon block overwrote the SVG's intrinsic style attribute when applying block styles. Merge the existing style with the generated CSS so `fill: none` survives, with block styles last to win on conflicts. Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Weston Ruter <westonruter@git.wordpress.org>
Co-authored-by: Weston Ruter <westonruter@git.wordpress.org>
Co-authored-by: Weston Ruter <westonruter@git.wordpress.org>
A previous suggestion application landed the JSDoc body without its opening /** delimiter, which left the file with orphaned * lines that break Node's parser. Restore the opener so the build runs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Rename splitStyleDeclarations to parseStyleDeclarations and have it split each declaration on the first un-quoted, un-parenthesised colon as part of the same tokenisation pass. Returns Array<[key, value]> instead of string[], so the caller no longer needs a second indexOf parse to extract the value. Per Weston's review feedback. Adds tests for the colon-handling edge cases (colons inside parens, quotes, and `data:` URIs). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Block icon's render: guard against an empty existing style when merging block styles, avoiding a leading semicolon when the intrinsic style is absent or whitespace-only. - Icon component: move the explicit style merge before ...restProps in the svg branch to match the generic element branch. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jasmussen
force-pushed
the
update/redraw-34-icons-to-stroke-based
branch
from
695eed4 to
2b921c0
Compare
The override declared stricter types than WP_Icons_Registry::sanitize_icon_content() in the parent class, triggering a PHP signature-compatibility fatal: Declaration of WP_Icons_Registry_Gutenberg::sanitize_icon_content(string $icon_content): string must be compatible with WP_Icons_Registry::sanitize_icon_content($icon_content) Match the parent's untyped signature. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Addressed to the best of my ability the feedback, and rebased this branch. To note, the sanitization-side feedback (DRY, polygon attributes, sanitization tests) is being more comprehensively addressed in @t-hamano's #75550, which is a substantial rework of sanitize_icon_content with a broader allowlist (including polygon with full stroke support) and dedicated tests. Doing intermediate refactors here would create a conflict with that PR. How would you feel about letting 75550 be the single source of truth for the sanitizer? As for the icon block render tests, here, happy to do a followup. The empty-style edge case should be guarded in code, so perhaps the test could be a followup. As far as next steps, I wanted to note I'm heading AFK for my summer vacation starting Tuesday next week. I'd love to be able to land this before then, as I believe it's a strong step forward for the icon tech, and it embodies changes so far that are purely invisible, all under-the-hood stuff that we can take advantage of later, notably when the rest of the set is updated to be stroke-based. But if we don't land it before then, I just wanted to make clear that others should feel free to commandeer and push/edit this PR, as well as merge, if they would like. Finally, I want to flag again that I think it would be useful to land #79116 as a safeguard for the Icon block. It ensures strokes scale like they do today, even as some icons become stroke-based. It would be a temporary safeguard because once the whole set is updated, we would change this to become a toggle instead, so you could choose your appearance, capitalising on the change. |
github-project-automation
Bot
moved this to 🔎 Needs Review
in WordPress 7.1 Editor Tasks
|
I think I would classify the |
|
Thanks for the clarity @mirka. I'd like to propose a way to address that consistency concern, I have it working locally but it's unpushed, since it will involve adding an additional 295 changes to this commit, I'd love to share the brief and pitch with you first, before I commit. The pitch is that we add a small source-normalization step to This script I've run locally, and validated that it works. What it does is add a new function to the existing build script. It's wired into main() so it runs on every When I re-run it on already-normalised source files, it produces no changes. In testing locally, it changed 295 icons, left 35 unchanged (the ones redrawn in this PR), coming to 330 total. It ignored the 35 as they all have style="fill: none" on the outer element. For the icons changed, it looks like this: If you approve, I'd make two commits:
I propose to do it in this PR, but it could also be a separte PR, or a "branch on the branch". I'd like to do it in this PR as I will be AFK from Tuesday next week, back July 8th. But I an also put it in a separate branch, so long as we remember to land both in short succession. E.g. I already think #79116 would be good to pair with this. Also to be clear: I do welcome commits from others, and to merge or even revert if something happens, in my absence. Let me know! |
|
The color normalization is a completely separate concern to this PR, and warrants dedicated review and documentation of our reasoning. So let's do it in a separate PR. If you can push the relevant changes that you made, I can drive it forward while you're out 👍 In terms of specifics, I have a feeling we should just normalize the existing SVGs with your script now and commit the changes permanently, rather than keeping the script as part of the build script. If we find that new icons get added without the necessary colors, we can maybe add a lint rule later. |
|
Solid feedback, and thanks so much for all the help. I've now pushed an entirely separate PR, #79320, which does as you propose. First it updates all the icons. The second is the optional helper script, but no longer tied to npm run build. We can keep or delete that second commit. But honestly it works pretty well even as a test: if you run it and it doesn't change a thing, that means all the icons are as they should. Let me know if this works for you! Thanks @mirka. |
…ns-to-stroke-based
The global safe_style_css filter added in #79172 (lib/compat/wordpress-7.1/kses.php) now allowlists SVG presentation properties, so the per-call filter that temporarily allowed fill/stroke properties during wp_kses() is redundant. Co-Authored-By: Claude <noreply@anthropic.com>
Same cleanup as the previous commit, applied to the WP 7.0 compat base class. The global safe_style_css filter from #79172 covers the SVG presentation properties, making the per-call filter redundant here too. Co-Authored-By: Claude <noreply@anthropic.com>
Remove the explanatory comment about the global safe_style_css filter from the WP 7.0 base class for a cleaner sanitize_icon_content(). Co-Authored-By: Claude <noreply@anthropic.com>
t-hamano
force-pushed
the
update/redraw-34-icons-to-stroke-based
branch
from
cdade30 to
bd8c908
Compare
Since the visual changes to the icon block are caused by this PR, I think it is best to add the safeguard directly in this PR. |
|
Thanks for all the help @t-hamano, much appreciated 🏅 I saw you added icon-block changes to this PR. I agree with those changes to safeguard the Icon block, but just noting that means we won't need the separate #79116. |
|
Hah, we commented at the same time. Full agreement! I'll close 79116 |
|
As a side note, if this PR is shipped, it might require a dev note, as it could significantly impact consumers using the
|
|
If we need such a dev note, here's a draft: The icons were redrawn with a stroke base primarily to enable size-independent stroke widths. They use No CSS changes should be necessary after this change. Stroke-only icons additionally set an inline style="fill: none" so they remain resilient to ordinary overrides. The one case to watch for is CSS that sets fill on svg (or its children) using |
8 participants
What?
Followup/replacement to #78774, which now includes the change including resilience for compatibility.
In this PR, I redrew/changed 34 icons to be stroke-based rather than purely fill-based. An existing icon was already stroke-based,
square. This brings an enormous amount of flexibility as far as future animation and customisation properties, it simplifies the Figma to Icon flow, but notably it means that icons now maintain their stroke-width across sizes. Paragraph shown as example, 16, 24, 32px sizes:Why?
For the majority of these icons, no visual change should be perceptible in where they are used today, because we use them mostly in their standard 24x24px sizes. And we do that specifically because of the shortcoming in how they scale, shifting stroke-width making them look out of place next to each other.
By enforcing a monoline stroke-width, icons can now be shown at virtually arbitrary sizes (though they can get fuzzy if shown really small), and yet still feel as if they belong together since their stroke-widths are maintained. A future perspective is even to allow consumers of the componentry to either animate the stroke, both the width and the dash-style, offering some interesting options, such as how this WordPress logo is animated.
How?
This PR includes both the tech to maintain a uniform stroke-width across icon dimensions, as well as 35 icons that take advantage of it. I am working in a WIP Figma file to go through all existing icons, as such:
In black, those are the icons in this PR, done/redrawn. In red, still todo. In blue, icons we will not be redrawing as they are not/can't be stroke-based. In gray, icons that need redesigns anyway. Once this effort is done, these icons will move to the main Figma design library. I believe there are some additional icons shipping in trunk that are not depicted in this list, but I will be sure to identify those once I'm through this effort.
Testing Instructions
Check out the PR; run npm run storybook:dev, go to Icons > Library, and then observe that at both 16, 24, and 32, that icons look as intended. These icons:
Use of AI Tools
Claude Code was used to extract a todo-list of icons for me, but the rest is hand-rolled, human spirit work.