-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add some more g2 icons #21825
Add some more g2 icons #21825
Conversation
[WordPress#21613](WordPress#21613) "hid" TypeScript emitted type declarations for `@wordpress/element` when it was found to conflict with 3rd party type declarations on DefinitelyTyped. Disabling `@wordpress/element` cascaded to require disabling type checking for `@wordpress/icons` and `@wordpress/primitives`, two dependent packages. The lint-staged flow is slightly different from the full `build:package-types` script in that it attempts to do a minimal build in the interest of speed. This flow checks for the presence of a `tsconfig.json` file in the root of the package with changes and runs its package build. While removing the affected packages from the main tsconfig.json did remove them from the primary build, changes to primitives and icons packages will fail because they cannot find the `@wordpress/element` types as was intended in WordPress#21613. By renaming the package tsconfig.json files, they are excluded from the lint-staged typechecking as well.
Nice work. The before/afters look good. Did we really not have the help icon before? I noticed btw. that the lowercase I info icon we might need to update that one to better match the new question mark in stem width. Separate thing to look at. The plug looks decent. I never loved that metaphor, but I can't think of a better one at the moment. And the verticality and outline-stroke adds some clarity that the diagonal one lacked. But what's important then is getting the name right, because "plugin" could be either a plug or a puzzle piece or any other metaphor. Material calls them "power", how about that? Here's the material extension icon: As for the new icons. I love the stylishness of the "House" icon. It's possibly too stylish, but I'd rather we start with that and pull back when we need to, rather than default to less. Bar charts and megaphones also look good, people looks great. Tool is a good name, I'm unsure about the wrench. It both feels too small and too dark at the same time. I wonder if there's a different metaphor that's more legible yet has potential for stylishness. I personally really love the simplicity of this pen, and the line it draws: These two both feel more balanced and clear, although of course they have nothing to do with wrenches: In the wrench, the hexagonal bit in the "head" is perhaps the most distinct part. Is there room to emphasize that? Could we even do with just a lugnut? What if instead of a tool we looked at the section it's intended for and found a different metaphor? This one for "tune" is also interesting as an alternative for settings, but perhaps not a 1:1 with the tool goal you're trying to accomplish: Support is tricky. I don't mind the lifesaver there, though it is a little dark with the dual strokes. Can it work in a solid version? Important to all of these is to get the names right. Good to go with the metaphor rather than the purpose, most of the time, as that lets us more easily revisit and refine them if we need to. |
I have no problem with "power" as a name, but the "plugins" icon already exists, so we'd have to leave it to maintain backwards compatibility. If we added a separate "power" icon with the plug metaphor, that would result in having two icons in the set that serve the same purpose, but with different names. Doesn't sound like a great situation. Seems simplest to just update the "plugins" icon to visually match the rest of the set, then perhaps add a separate "extensions" icon in the future. For the tool, I did originally try some other variants:
We intend to use it for our "tools" menu in WooCommerce. It contains features like importers, system status reports, and other utilities like permission resets, thumbnail regeneration, etc. If we'd like to explore something more stylish, "robot" or "automation" might be an avenue to go down. A lugnut / bolt could work, but I fear it's too close to the "cog" icon: Reversing the lifesaver just makes it feel darker, and less like a lifesaver, to me at least: |
Agreed on the lugnut and lifesaver. But we need to crack that wrench. This is obv. a lightbulb with a wrench in it, which won't work: But from that one, I like that:
I wonder if we can move towards that, somehow? |
I just noticed that empty / half-filled versions of the star icon exist in the g2 set, but they seem to have been transplanted from Dashicons. I'm going to update those in this PR as well. Top row is g2 - based on the previously approved star icon. Bottom row is the current implemented icons. |
Replaces old Dashicons stars with g2 equivalents
Much nicer stars, thanks Jay. Those are 1.5px strokes too, right? Big improvement. How do you feel about the innermost radius of the stars? It may be a non issue when shown at 1x but at a glance it feels slightly too "bloated" for want of a better term. Here's a fun star generator. With an outer radius of 100 and an inner of 38, it looks geometrically star-like: This one looks too sharp. With an outer 100 and inner 60, it's too childish: This is 100/46 — that seems like it balances the two extremes maybe? |
Thanks for such a fast response! Let's indeed try 100/50. |
fill-rule="evenodd" | ||
d="M9.706 8.646a.25.25 0 01-.188.137l-4.626.672a.25.25 0 00-.139.427l3.348 3.262a.25.25 0 01.072.222l-.79 4.607a.25.25 0 00.362.264l4.138-2.176a.25.25 0 01.233 0l4.137 2.175a.25.25 0 00.363-.263l-.79-4.607a.25.25 0 01.072-.222l3.347-3.262a.25.25 0 00-.139-.427l-4.626-.672a.25.25 0 01-.188-.137l-2.069-4.192a.25.25 0 00-.448 0L9.706 8.646zM12 7.39l-.948 1.921a1.75 1.75 0 01-1.317.957l-2.12.308 1.534 1.495c.412.402.6.982.503 1.55l-.362 2.11 1.896-.997a1.75 1.75 0 011.629 0l1.895.997-.362-2.11a1.75 1.75 0 01.504-1.55l1.533-1.495-2.12-.308a1.75 1.75 0 01-1.317-.957L12 7.39z" | ||
clip-rule="evenodd" |
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.
There are console warnings here:
Warning: Invalid DOM property
fill-rule
. Did you meanfillRule
Warning: Invalid DOM property
clip-rule
. Did you meanclipRule
The warnings should be pretty self-explanatory in their solutions, I hope 😄
See also: https://reactjs.org/docs/dom-elements.html#all-supported-html-attributes
Good work here! My general feeling towards adding icons, especially when there are hard-work PRs like this one, is that we should be liberal in adding them. So long as:
With the work you've done so far, I feel like we've reached that point with this PR! So from a design POV, 👍 👍 I have a bit on my plate today, so I can't code-review this. But feel free to get this sanity checked elsewhere and merged. Thanks Jay! |
@@ -113,7 +118,6 @@ export { default as search } from './library/search'; | |||
export { default as separator } from './library/separator'; | |||
export { default as share } from './library/share'; | |||
export { default as shortcode } from './library/shortcode'; | |||
export { default as star } from './library/star'; |
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.
Was this icon ever published as part of a stable WordPress or Gutenberg release?
If so, we can't simply remove it, since there's a backwards-compatibility commitment.
If it's only ever been published in the plugin, we can deprecate it after two versions.
See:
- https://developer.wordpress.org/block-editor/developers/backward-compatibility/deprecations/
- https://github.com/WordPress/gutenberg/tree/master/packages/deprecated
If it was published in a stable WordPress release, then it must be kept forever.
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.
Hm, or is this a unique package in how it's not actually enqueued in the editor? I guess at the very least, if it's only ever intended to be consumed as a package, we'd still want to include a CHANGELOG entry about the removal being a breaking change.
See: https://github.com/WordPress/gutenberg/blob/master/packages/README.md#maintaining-changelogs
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.
It was added, as an oversight here. star-filled
already existed (and was identical in it's intended application), so star
is extraneous. Let me know how you think we should proceed.
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.
I guess if it isn't exposed directly on the window
global, and it never landed in a published version of the package (verified), then no additional action should be necessary.
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.
From purely a code perspective, it looks good 👍
Co-authored-by: Andrew Duthie <andrew@andrewduthie.com>
Co-authored-by: Andrew Duthie <andrew@andrewduthie.com>
Thanks for the latest updates with the CHANGELOG 👍 |
In this PR I'm proposing some new icons, and some updates to existing ones.
Updates
The current cog icon is awkwardly rendered - its 'body' isn't quite circular. I've tidied this up.
The plugins and help icons have seemingly been lifted directly out of Dashicons, so don't match the rest of the set. I've used existing g2 icons as inspiration for updating these.
I also updated the alignment of the arrow-left icon so that it is perfectly centred.
New icons
Additionally, I'm proposing some new icons for home, bar chart, megaphone, people, tool, and support. Most of these exist in Dashicons, but support is new.
@jasmussen @pablohoneyhoney