-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 improved "Group" and "Ungroup" icons #16001
Conversation
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'll leave the visual and design side to those who are better placed to comment. Technically I can't see anything that would block this being merged.
Is this going to be merged? 😀 |
😄 Just waiting for a design review too. @mapk or @jasmussen, do you have a sec to take a look? |
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.
These are nice, and definitely good enough to ship. No objections. Since you ask, I do kind of wish the "ungroup" had a more direct relation to group, kind of like how link and unlink have in the material set: I don't know that the diagonal cross over the group would work here, as it seems so destructive. But I do like the connection between the two. It's also interesting to see the noun project's page for this term: https://thenounproject.com/term/ungroup/ — lots of different interpretations. But those are mostly musings, and one should be careful with those because otherwise things can devolve into fence painting. The truth is, these icons are good, and are going to be shown next to labels, so let's ship it. But lets also be open to shower-ideas for futher improvements, should such be possible! |
These make sense to me. Using the icons consistently across where this action appears should help connect the two different entry points for users, making it easier to understand the behaviour. 🚢 |
Yeah, fully agree. :/ I couldn't quite get that crossed-out approach to work here. But I think we can sort this out in another round though. Anyway, thanks folks! I'll merge these in. |
@@ -4,5 +4,5 @@ | |||
import { Path, SVG } from '@wordpress/components'; | |||
|
|||
export default ( | |||
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><Path d="M19 12h-2v3h-3v2h5v-5zM7 9h3V7H5v5h2V9zm14-6H3c-1.1 0-2 .9-2 2v14c0 1.1.9 2 2 2h18c1.1 0 2-.9 2-2V5c0-1.1-.9-2-2-2zm0 16.01H3V4.99h18v14.02z" /><Path d="M0 0h24v24H0z" fill="none" /></SVG> | |||
<SVG width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg"><Path fill-rule="evenodd" clip-rule="evenodd" d="M9 8a1 1 0 0 1 1-1h6a1 1 0 0 1 1 1v4a1 1 0 0 1-1 1h-1v3a1 1 0 0 1-1 1H8a1 1 0 0 1-1-1v-4a1 1 0 0 1 1-1h1V8zm2 3h4V9h-4v2zm2 2H9v2h4v-2z" /><Path fill-rule="evenodd" clip-rule="evenodd" d="M2 4.732A2 2 0 1 1 4.732 2h14.536A2 2 0 1 1 22 4.732v14.536A2 2 0 1 1 19.268 22H4.732A2 2 0 1 1 2 19.268V4.732zM4.732 4h14.536c.175.304.428.557.732.732v14.536a2.01 2.01 0 0 0-.732.732H4.732A2.01 2.01 0 0 0 4 19.268V4.732A2.01 2.01 0 0 0 4.732 4z" /></SVG> |
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.
fill-rule
and clip-rule
are not valid names for props. When running with the SCRIPT_DEBUG
constant, you will see errors log to the developer tools console:
Warning: Invalid DOM property
fill-rule
. Did you meanfillRule
?
Warning: Invalid DOM property
clip-rule
. Did you meanclipRule
?
(The message includes the respective solution for each)
React uses DOM property names for elements, not the HTML element attribute names. This is the same reason we assign classes as className
and not class
.
See also:
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.
Thanks for catching that! I've submitted #16096 to correct the issue.
Closes #15602. Followup from #14908.
This PR replaces the Group and Ungroup icons with those decided on in #15602 (comment). It replaces both the action icons in the ellipsis menu, and also the Group block icon itself for consistency.
Screenshots