Update pencil icon. #25135
Update pencil icon. #25135
Conversation
jasmussen
added
[Type] Code Quality
jasmussen
requested review from
shaunandrews,
youknowriad and
MichaelArestad
|
Size Change: -153 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hey @jasmussen! Great icon here but pencil is being used from other components as well, that doesn't make much sense to show a brush icon. For example edit in rss or embed blocks among some others.
So we can have two directions here:
- Create a new
brushicon ( this one ) and use it FSE page for global styles - Rename this one properly and leave or change the
pencilicon that was used in other components foredittoediticon, as you mention in your PR's description.
What do you think?
|
Awesome observation, thank you for that context. The thing is, the edit icon that this replaces should not be used anywhere, it's an old icon. I will see if it makes sense to add a commit that updates those other components to use better icons. |
Do you want me to make these changes for changing pencil to the newer |
Thanks for the offer, that's okay I'll take a look now! |
jasmussen
requested review from
ajitbohra,
ellatrix,
Soean and
talldan
as code owners
|
Again, excellent catch. I believe I caught them all. Before:
After:
(honestly that component could use love and refactor to use the same link popover as normal links, but that's separate) Embed before:
After:
Actually that one should probably have a "Replace" label just like Image. But that's also separate, so as to keep this PR focused. I think the remaining question is whether to rename the "pencil" to "brush" given that we already have a pencil in the "edit" icon. @youknowriad any thoughts? |
|
The only one left is in
I think you could rename the |
|
Ugh, I had forgotten to search for "icon: pencil". Updated now:
Definitely happy to, just want to make sure that's okay to do with a green light from Riad, for example, since it updates the icon component. |
Yes you're right! It might be used by third party code.. |
|
Is there a reason why we can't have a separate pencil icon and a new brush icon? This would also allow us to copy the current edit icon to pencil and let edit export the "new" pencil icon without any back-compat issues. // edit.js
import { default as pencil } from './pencil';
export default pencil; |
I can do whatever we agree is best. Just making sure I understand correctly: this would effectively be a deprecation of the |
|
I think what is suggested and probably is a good idea is to leave |
|
Ah, if that's the case we'll still need to revisit the |
I wouldn't deprecate it, just making |
|
Oooh that makes sense. Thank you, I'll give that a shot (tomorrow, the kids are clawing at me). |
This makes a lot of sense to me, too. Has this been done before, and if so are there examples? I like the idea of having matches for both a specific icon (pencil) and a more general description (edit) and linking between the two. -- Its not clear to me if the paintbrush is a new or existing icon; Does this icon exist in WP currently?
For what it's worth, my first response to seeing this icon is "broom" or "sweep." I think that's because I see a "floor" represented by the horizontal line. I understand you're attempting to keep it consistent with the existing pencil icon (which has this line), but I'm not sure its helpful or actually even logical. When I think of paint brushes, I don't think of thin, straight lines; I think of swirls and blobs and drips. |
|
I'll second @shaunandrews, that the icon is ambiguous. I wasn't sure if I was seeing a spade. I think the rather large difference in width from the handle to the fiber bundle is mostly to blame. In case it's open to suggestion, I'll posit a brush meant for more coverage might be better: |
It exists insofar as it was designed for global styles and were added to the WordPress Design Library:
(rightmost) I'd prefer to keep this PR focused on replacing the dashicon that's incorrectly used for global styles. That's not to dismiss any critique of the brush, that's perfectly valid, and perhaps best given as comments directly in the Figma file. So outside of addressing Dominik's feedback on making "pencil" an alias for "edit", what's the next step? I can picture two approaches:
An alternative icon for global styles was also designed:
That's definitely more font specific, but I think it can work too. Happy to use that also. Most importantly, I would prefer we don't overthink this — the PR is meant to be a quick win, and I'm happy to do any followup PRs. |
|
Oh that's interesting. I just rebased and now the icon looks like this:
😆 — someone else had the same instinct! https://github.com/WordPress/gutenberg/pull/24250/files#diff-3924a41ad88da62082971d8c0aa8b20fL58 That effectively settles the discussion. I'm going to remove the Brush icon again, and encourage you all to share your thoughts in the Figma file. Then I'll refocus this PR on making the "pencil" icon an alias. |
jasmussen
force-pushed
the
update/style-icon
branch
from
2d9e7c0 to
b2a21d7
Compare
|
Alright, I changed "edit" to be an alias for "pencil". That helps update all the blocks that used the old dashicon:
I kept the updated brush, because it replaces this dashicon:
So, despite what I said in this comment: #25135 (comment) — this PR can be a place where we decide what the actual brush should look like, even though it's currently only used in the Legacy Widget block, and broken at that. But here's a stab:
Let me know what you think and I'll update the SVG. |
|
These work well together. The updates to the paintbrush's handle and tip do help it feel more like a paintbrush. (I also see a fountain pen now, but I think that's OK.) I'd say push this and we can circle back around when/if we find a use for the paintbrush and see how it works in context. |
|
Awesome, thanks so much Shaun. @ntsekouras if you can sanity check my last commit, 6815a59 — not urgent, just when convenient — let's land this and I promise I'll circle back to the brush icon if/when it gets used and feels like it needs additional updating. |
|
Well there you go! Thank you! |
Before:
After:
Note, the essence of this PR is to update the icon currently named
pencil. There's also a separateediticon, which is this one:Perhaps the "pencil" should be renamed "paintbrush" or "brush", but for now at least this updates the icon.
There's also this icon, which could be used for global styles: