Adding color filled icon #67202
Adding color filled icon #67202
Conversation
|
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. |
matiasbenedetto
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.82 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 9fcded7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11973403890
|
There was a problem hiding this comment.
Thanks for putting up the PR to add the new icon to the library @matiasbenedetto 🚀
Code wise it looks pretty good to me. I think both of @mirka's suggestions are good ones to tweak.
I took the icon, as is, for a spin in the real world use case from #67140. It works well for me as shown in the video below.
Screen.Recording.2024-11-22.at.2.33.11.pm.mp4
For my test, the switch section styles button component just adds the stroke and stroke width styles inline.
Quick diff using the new icon for the switch section styles button
Note: This diff was applied after rebasing #67140 onto this branch.
diff --git a/packages/block-editor/src/components/block-toolbar/switch-section-style.js b/packages/block-editor/src/components/block-toolbar/switch-section-style.js
index 4101b85d3f..38b4956411 100644
--- a/packages/block-editor/src/components/block-toolbar/switch-section-style.js
+++ b/packages/block-editor/src/components/block-toolbar/switch-section-style.js
@@ -5,9 +5,8 @@ import {
ToolbarButton,
ToolbarGroup,
Icon,
- Path,
- SVG,
} from '@wordpress/components';
+import { colorFilled } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';
import { useDispatch, useSelect } from '@wordpress/data';
import { useContext } from '@wordpress/element';
@@ -22,23 +21,6 @@ import { GlobalStylesContext } from '../global-styles';
import { globalStylesDataKey } from '../../store/private-keys';
import { getVariationStylesWithRefValues } from '../../hooks/block-style-variation';
-const styleIcon = (
- <SVG
- viewBox="0 0 24 24"
- xmlns="http://www.w3.org/2000/svg"
- width="24"
- height="24"
- aria-hidden="true"
- focusable="false"
- >
- <Path
- stroke="currentColor"
- strokeWidth="1"
- d="M17.2 10.9c-.5-1-1.2-2.1-2.1-3.2-.6-.9-1.3-1.7-2.1-2.6L12 4l-1 1.1c-.6.9-1.3 1.7-2 2.6-.8 1.2-1.5 2.3-2 3.2-.6 1.2-1 2.2-1 3 0 3.4 2.7 6.1 6.1 6.1s6.1-2.7 6.1-6.1c0-.8-.3-1.8-1-3z"
- />
- </SVG>
-);
-
function SwitchSectionStyle( { clientId } ) {
const { stylesToRender, activeStyle, className } = useStylesForBlocks( {
clientId,
@@ -101,9 +83,11 @@ function SwitchSectionStyle( { clientId } ) {
label={ __( 'Switch style' ) }
>
<Icon
- icon={ styleIcon }
+ icon={ colorFilled }
style={ {
fill: activeStyleBackground || 'transparent',
+ stroke: 'currentColor',
+ strokeWidth: '1.5',
} }
/>
</ToolbarButton>
I did notice that there are some icons that have hardcoded stroke values in the library. Not sure if that was a deliberate decision or not.
It might not hurt to get design signoff here if @jasmussen or @jameskoster have a moment to cast an eye over the proposed icon.
|
Thanks for the PR, thanks for the ping! I ran storybook, and sure enough, there's a solid new icon. However, the use cases outlined in the initial comment gives me a bit of pause, because there are some do's and dont's that we need to be careful of, for this particular utility icon. Notably, the icon should always have a stroke, unless it's black. That is part of the morphology of the icons. I recognize there's a use case of colorizing the stroke of the icon according to any border-color set, it's not actually clear to me that we want to do that. But entertaining the thought, the following could be okay:
Note here how there's always a stroke, and that stroke is on the inside. This is what I see in the original examples, the stroke being applied on the outside, and some cases of showing the icon in just a single color, without any of the black color:
Stacked to illustrate inner vs. outer strokes:
The black color of the icon serves a contrast purpose. It is colored according to the UI to ensure there's always at least 3:1 contrast with the surroundings. For that same reason, I'm ultimately hesitant about adding a fully filled icon like this, at all, because it's not clear you should ever use it like that. The function is only ever designed to show a single color, like so:
I understand that the above is a bit tricky to achieve with how the icon is structured at the moment. Perhaps there's a separate utility component created, that is not part of the library, that has things separated out more cleanly? |
|
@matiasbenedetto might be able to answer better than I but this PR aimed to make the icon more flexible than our real world use case in #66465 (comment) & #67140 requires.
If this icon did always have a stroke would that make it suitable for inclusion in the icon library?
I think this aligns exactly with the intended use in #67140. |
Certainly. If I can include a |
|
Was that clear? The thing about landing in the icons library means we also add it to the Figma WordPress Design System, where people use the icons for mockups. If there's a utility icon there that's never meant to be used on its own, it probably shouldn't be part of the library. |
|
Thanks for taking the time to clarify all that 👍
I believe that is the main intent of this PR, to provide a duplicate that's purpose is to allow coloring the innards. It just went an extra step to not enforce any stroke. A small tweak on that front should have us all on the same page again. |
|
That sounds right. But I'm now focusing a bit on the part where we'll have a filled version of the icon present also in the Figma library, that may not be meant to be used as a standalone icon, just a utility that's part of the other icon. To that end, I wonder again, should it be separate, not live in the icons package? Or could it be a hidden fill, |
Co-authored-by: Lena Morita <lena@jaguchi.com>
I'm not sure if I got you right, but for retro compatibility, I think this isn't great because the fill of the current droplet icon is the 'border'. So, changing the existing icon may cause the places where a color fill was applied to colorize the 'border' now to be applied to the proper 'fill'. |
|
I found that's difficult to try things on icons quickly. So I submitted this PR to improve props manipulation for icons in Storybook: #67242
@jasmussen Thanks for the input! I get you point but from what I can see in the library, no file icon has a border defined. That's what I wanted to replicate in this PR: Only a few examples: But there are many more:
|
If the icon is tied to a very specific piece of functionality I'd be inclined not to include it. Did we consider alternatives to an icon, like the Duotone swatch, or overlapping swatches? 
|
The icon was discussed in this issue: #66465 |
I'm possibly misunderstaning Joen's suggestion but I took it as adding a new filled path within the original used for the border. This new path is hidden by default which maintains backwards compatibility. Then when using it in places like the "switch section styles" button in the block toolbar, the consumer sets that inner path to display and applies the desired fill color. If that suggestion is adopted, we can have a single icon that meets all the desired use cases living in the icon library. There's no "duplicate" only to be used in certain scenarios.
The use of the Duotone swatch was discussed but not overlapping swatches. Using overlapping swatches might tie the section styles control in the toolbar more toward colorways, which section styles was closely related to in early iterations. In the linked discussion, a potential desire to reflect multiple colors was mentioned. A possible downside with the overlapping color swatches might be that the user could expect it to reveal color palettes more so than block styles. This might not be any different than with the current use of the color icon though 🤷 |
|
To expand on what Jay says here:
We need a build step (#38850). Which means every icon in the icons package needs to be super generic, flat, predictable. Because we'll be running For that reason, every icon in the icons package needs to be possible to use without supplying any color, because if you use the icon from the future icon-font source, there won't be any stacking or layering you can tap into. The icon needs to be able to work on its own. The single bordered droplet does that: it symbolizes color:
We want to be able to color the inside fill of that icon, to be sure! That's cool. But as far as I can tell, it adds a new icon to the icons packaga in order to do that and then uses fill and border to emulate the end result. But someone who uses the icon font would just see this:
That icon should not be used, as it doesn't symbolize anything. That's why I've called it a "utility" icon. You can add it, and then stack one on top of the other—because the border should alwasy be black. But the utility icon shouldn't be in the package. |
What?
Adds a filled version of the 'color' icon, 'colorFilled'.
Why?
To be able to implement the UI proposed in #66465 (comment) for #66465
How?
Adds a new icon to the icons library.
This is a slightly modified version of the SVG code proposed by @aaronrobertshaw in #67140 (review) props to him.
I removed some unwanted or hardcore properties to make the SVG code looks like the rest of the filled icons in the package.
Testing Instructions
Import the 'colorFilled' from the
@wordpress/iconspackage and observe how it looks.Try to set a fill color and a stroke color too.
Screenshots or screencast
Default:
With custom fill color set:
With custom fill and stroke color set:
