Change Group block default variation in inserter. #44176
Conversation
tellthemachines
added
[Type] Bug
tellthemachines
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Size Change: +2.4 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
| { | ||
| name: 'group-flow', | ||
| title: __( 'Group' ), | ||
| description: __( 'Gather blocks in a container.' ), | ||
| attributes: { layout: { type: 'default' } }, | ||
| scope: [ 'block' ], | ||
| isActive: ( blockAttributes ) => | ||
| ! blockAttributes.layout || | ||
| ! blockAttributes.layout?.type || | ||
| blockAttributes.layout?.type === 'default' || | ||
| blockAttributes.layout?.type === 'constrained', | ||
| blockAttributes.layout?.type === 'default', | ||
| icon: group, |
There was a problem hiding this comment.
Interesting. I didn't realise this approach had been taken before and there was already a group variation.
I wonder if this old variation can be removed? I'm not sure what a scope of block does, but any matching blocks would (or should) fallback to the normal block type, which would be fine.
There was a problem hiding this comment.
Good question, it seems to work well enough if I remove it. Let's try that.
There was a problem hiding this comment.
Thanks for pursuing this alternative! It seems to be working pretty well so far, which is promising.
I noticed that the update to the variations causes the selector at the top right of the inspector controls to toggle off Group when switching to the flow / default layout type:
I believe this is where the active block variation check happens in the BlockVariationTransforms component, in case it helps:
Are there any other places where the active variation check is being used like that? From a quick search for usage of getActiveBlockVariation, that appears to be the main one that would affect the Group block as far as I can tell 🤔
talldan
added
the
Backport to Gutenberg RC
|
@tellthemachines I think there are going to be a lot of snapshots to update. You may be able to cherry-pick 946e555 from my branch. |
Possibly the Might be able to refactor it to something like: |
Oh, I like that. Yes, sounds like a good way to ensure that it's a catch-all for everything that isn't Row or Stack 👍 |
My fault, I removed these lines while experimenting and forgot to re-add them 🤦 |
|
I'm not sure what should be expected when transforming a block or a multi-selection into a Group. It would be more consistent with the new default behaviour if those Groups also had constrained layout 🤔 (looking at the failed snapshots and wondering if we should consider some of them legitimate failures 😅 ) |
|
Ok I updated the multi-select transform so wrapping other blocks in a Group will create a constrained layout block. That fixed about half the failing snapshots 😅 and I've just updated the other half. |
|
This is testing well for me, can add all the group variations and all see to behave as expected, and site editor no longer freezes when adding template parts with groups. I also manually ran the group deprecations in the editor and all updated without any errors. Just one question I had was is |
There was a problem hiding this comment.
This is testing really well for me, too — I didn't run into any issues with any of the legacy or current markup that I tried it with 👍
I spotted one other place where we can Group blocks together, when multiple blocks are selected — should this one also get the constrained layout for consistency with the contextual menu Group item?
I personally don't mind it with the default layout type, so don't feel strongly about which type is used when grouping things either way.
That's a good question! It comes from #42763 where we add the new layout type to any legacy blocks with either We could always change that though! |
I don't have a strong opinion on it, but it may be worth tidying up to avoid confusing future us maybe? |
Good catch! I think it makes sense to do so, and have updated the toolbar accordingly. |
There was a problem hiding this comment.
Great work @tellthemachines, this is testing well for me, and I haven't been able to find any issues with it. Legacy block markup appears to be rendered / updated correctly, and I haven't run into any errors.
I thought for a moment that there was an issue with justification alignments, but it turns out it was a typo that's been resolved in trunk in #44179.
This seems like a hopefully fairly safe change given that the fixtures only needed to be updated to shift the position of the block attributes.
LGTM! 🎉
|
Thanks for the reviews folks! |
tellthemachines
added
Backport to WP 6.7 Beta/RC
* Change Group block default variation in inserter. * Remove unnecessary variation * Add back check for default layout Groups * Multi-block transforms should also have constrained layout. * Update failing snapshots. * Grouping paragraphs should add constrained layout.
|
I just cherry-picked this PR to the wp/6.1 branch to get it included in the next release: 83937b3 |
ockham
removed
the
Backport to WP 6.7 Beta/RC
andrewserong
added
the
[Feature] Layout
Why?
Fixes #43760.
How?
Removes the effect updating layout in the block edit, adds a constrained layout variation as default and removes flow layout variation from inserter.
Fixtures are updated because removing the effect caused a re-positioning of block attribute output.
Testing Instructions
Screenshots or screencast