Group: Add group block variations to Group toolbar #39920
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
the
[Status] In Progress
|
Size Change: +279 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
glendaviesnz
removed
the
[Status] In Progress
glendaviesnz
commented
Mar 30, 2022
| if ( newBlocks ) { | ||
|
|
||
| if ( newBlocks && newBlocks.length > 0 ) { | ||
| newBlocks[ 0 ].attributes.layout = |
There was a problem hiding this comment.
it would be nice if these attributes could be passed to switchToBlockType, or some related method, but I couldn't see a way to do that
There was a problem hiding this comment.
... maybe updateBlockAttributes would be better here, will take a look at that
There was a problem hiding this comment.
I tried replacing this with
updateBlockAttributes( newBlocks[ 0 ].clientId, {
layout: variationAttributes[ variation ],
} );which seems like the more legit way to do this, but it doesn't work for some reason?
There was a problem hiding this comment.
which seems like the more legit way to do this, but it doesn't work for some reason?
Given the new group block isn't yet in the block editor store, the use of updateBlockAttributes won't work here. As @talldan suggested, perhaps a comment explaining why the current approach is used might save someone else in future from wondering the same thing.
aaronrobertshaw
approved these changes
Mar 31, 2022
There was a problem hiding this comment.
Thanks for this update @glendaviesnz 👍
The new group variations in the toolbar work well for me and the current approach to tweaking the layout attributes also seems appropriate.
Screen.Recording.2022-03-31.at.10.40.18.am.mp4
Mamaduka
reviewed
Mar 31, 2022
packages/block-editor/src/components/convert-to-group-buttons/toolbar.js
Outdated
Show resolved
Hide resolved
|
This looks great, @glendaviesnz 👍 I wonder if this should be a dropdown, but it can be addressed in the follow-up. |
Good question, I just went off the design here from @jasmussen - what are your thoughts Joen on a follow up PR to make this a dropdown, or do you think it is ok with all 3 in the toolbar for now? |
|
Nice one! I like giving prominence to these by showing them directly in the toolbar. If they're in a dropdown, they might as well stay in the transform dropdown. It works well: However the metric are off. In a toolbar-group, each item should be 36x48, with extra padding applied at the start and end of the toolbar group:
|
|
For a little more context on toolbar button metrics, they were recently updated in #39630. Here's a codepen that illustrates the mechanism:
As a result, a group with just a single item will end up being 48x48px as it should, but icons in sequence should be better spaced. I'm not sure why that isn't automatically applied here 🤔 |
|
@jasmussen maybe the selectors are not being used on the multi-select toolbar? |
mtias
reviewed
Mar 31, 2022
packages/block-editor/src/components/convert-to-group-buttons/toolbar.js
Outdated
Show resolved
Hide resolved
mtias
added
the
[Feature] Block Multi Selection
|
This is looking like a great addition, thanks for the quick turnaround. |
youknowriad
approved these changes
Mar 31, 2022
packages/block-editor/src/components/convert-to-group-buttons/toolbar.js
Outdated
Show resolved
Hide resolved
|
@jasmussen The toolbar styles that apply that effect were a bit too complex. I pushed a commit that simplifies a bit and fixes the issue in this PR but it's still too complex IMO. The fix can impact other blocks/toolbars... so I'd appreciate some testing. |
|
Do we have plans to absorb all that CSS in the components that compose the toolbar? |
Initially this "smart" spacing was meant to be specific to block toolbars (and not any toolbar) I guess that's why it's done using these overrides. |
Mamaduka
reviewed
Mar 31, 2022
packages/block-editor/src/components/convert-to-group-buttons/toolbar.js
Outdated
Show resolved
Hide resolved
|
Thanks for the updates @aaronrobertshaw - this tests well for me with the changes, so will merge of the failing e2e tests passes this time. |
|
@youknowriad, I think changes from c3b3046 impacted Block Controls toolbar groups a bit.
It's more visible when Block Movers are disabled.
|
|
Thanks for the report, I have a fix for the metrics in #40037. |
This was referenced Apr 14, 2022
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds the new
RowandStackGroup block variations to the Group block toolbar that is shown when multiple blocks are selectedWhy?
To allow users to easily select which layout variation they want for the nested blocks, as was requested here.
How?
Adds two additional buttons to the toolbar with relevant icons, and adds the relevant layout attributes for the group type selected.
Testing Instructions
Screenshots or screencast
group.mp4