Add layout classes to legacy Group inner container #56130
Conversation
tellthemachines
added
[Type] Enhancement
tellthemachines
requested review from
spacedmonkey and
ajitbohra
as code owners
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/layout.php ❔ phpunit/block-supports/layout-test.php |
|
Size Change: +260 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Oh, this is very compelling, thank you for picking this up!
In testing, provided that a theme doesn't add too many of its own rules to override spacing values, this appears to unblocks adding in the appearance-tools opt-in. I think it's still a good idea for it be an opt-in, because (for example) themes like TwentyTwenty have rules that don't completely play nicely with things like blockGap. I.e. here's an Image block in a Group block with blockGap set to 0:
| Editor view (TwentyTwenty) | Site frontend (TwentyTwenty) |
|---|---|
 |
 |
In terms of this code change, something I was wondering about is whether it's better to extract the classnames from the wrapper and apply to the inner container (as you've done here), or whether it'd be better to skip the injection of the layout classnames in the first place and then generate again for the inner wrapper. Given that the inner container is something that's applied to the already generated markup, I think I actually like the way you've done it here, as it groups together all the inner container logic in one place, so the main layout functions don't have to worry about it 👍
It could be good to update the unit tests, too. I see there's a test that appears to already look at the injection of classnames to an inner wrapper, but it doesn't quite cover the current behaviour or the change in this PR (possibly because of the order of the filters?):
A couple of other things I noticed while testing (unrelated to this PR) with appearance-tools opt-in applied. Just jotting these down here, but we can totally look into them separately:
- The position control is exposed for the Group block, which means that there is now the tabbed interface between settings and styles, with Position as the only control under settings. When layout controls are exposed, I think the placement of Position here makes sense, but it looks a bit odd when it's the only control in that panel. I wonder if we can conditionally move it to the styles tab if no layout controls are present, or something like that? (Or force there to be no tabs if in a classic theme)
- The spacing of some of the controls look slightly off, for example there's extra space between these color controls until you click into the interface:
- There is too much space between the Margin and Block Spacing headings and their controls:
I imagine we'll probably want to test with a bunch of different Classic themes to see where any potential pain points might be?
lib/block-supports/layout.php
Outdated
|
|
||
| if ( $processor->next_tag( array( 'class_name' => 'wp-block-group' ) ) ) { | ||
| foreach ( $processor->class_list() as $class_name ) { | ||
| if (str_contains( $class_name, 'layout' ) ) { |
There was a problem hiding this comment.
It's probably unlikely that folks might use custom classnames that contain layout, but to be a bit more specific about it, would it be worth using str_starts_with( 'is-layout-' )?
There was a problem hiding this comment.
Oh wait, I forgot about the compound classnames like wp-block-group-is-layout-flow 🤔
There was a problem hiding this comment.
I think we'll have to take the risk with custom classnames 😅 because the crucial one for block gap to work is wp-container-core-group-layout-x. It was fortunate that #55416 had already made that classname layout-specific!
|
Here's what I'm seeing for the same post in different themes - it's working pretty well for me! Classic<div class="wp-block-group">
<div class="wp-block-group__inner-container is-layout-constrained wp-container-core-group-layout-1 wp-block-group-is-layout-constrained">
...
ackground-color has-background">Welcome to WordPress. This is your first post. Edit or delete it, then start writing!</p>
</div>
</div>Block theme<div class="wp-block-group has-global-padding is-layout-constrained wp-container-core-group-layout-9 wp-block-group-is-layout-constrained">
...
</div>Gap works as described in editor and frontend for classic themes with
On the surface, this sounds reasonable to encapsulate the code in this way. Especially if it skips a few CPU cycles (or would |
Ah, well spotted! That test only checks I'd better write up a test for |
|
Tests added!
Actually I don't think we have a way of checking whether appearance tools are enabled on the editor side without theme.json. The core store In any case I'm not sure it would be a good perf tradeoff to add a check that would run in every single case (on the editor side) vs having the HTML tag processor run a couple of times in a contained function that only runs when certain conditions are met (Group block in a classic theme). My concern here was rather if changing the classnames around might break stuff for anyone; I think in terms of consistency it would be nicer to always have them applied to the inner container. |
|
Flaky tests detected in d110316. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6873080453
|
|
Looks like all the tests are passing now! I think this is ready for another review. |
There was a problem hiding this comment.
Nice updates @tellthemachines and thanks for adding explanatory comments, too, that helps the readability of the logic, and I'm sure will help with any future follow-ups, too 👍
✅ Layout classnames are correctly moved down to the inner group wrapper
✅ When child layout rules are applied (as in children of Row), then the container class with the rule for the child layout rules (e.g. flex-basis) is still correctly applied to the outer wrapper
❓ Not a blocker for this PR as I noticed it on trunk, too, but it looks like applying child layout styles to a child of Row means that the child sometimes winds up getting layout classnames applied (i.e. the paragraph block gets is-layout-flow and wp-block-paragraph-is-layout-flow classnames). Since it happens on trunk we can look at that separately. I think I know what's causing it, so I'll see if I can get a PR up.
✅ No style conflicts from moving the layout classnames down to the inner wrapper across a few Classic themes that I tried
✅ Tests look good
Overall this looks good to me, and I really like the approach, the more that I look at it, the more natural it feels to me. Kudos again to @carolinan for all the work in #47451 and for pressing forward with the appearance tools work.
LGTM! ✨ Might be worth getting another review, too, just to confirm.
| ), | ||
| 'group block with layout classnames' => array( | ||
| 'args' => array( | ||
| 'block_content' => '<div class="wp-block-group is-layout-constrained wp-block-group-is-layout-constrained"></div>', |
There was a problem hiding this comment.
Tiny nit: there's one other classname that we copy over and could be good to ensure gets copied, and that's the wp-container-core-group-layout-1 classname. Is it worth adding, or do you think we've got enough classnames in here to cover the layout match anyway?
There was a problem hiding this comment.
I think it's fine to leave it out given that it's not always present. We can't really test for that class being applied correctly in here, though it might be worth testing separately. Something like this but testing that the word layout is in there?
There was a problem hiding this comment.
Ah, yeah, that might work. I think we can probably leave it for now as it's possibly a little beyond the scope of this PR. For now, the tests you've added look good to me!
|
@tellthemachines You removed the Apologies if you did this and I missed it. |
andrewserong
added
the
Backported to WP Core
|
@getdave it was backported in: WordPress/wordpress-develop#5812 — I've added the |
|
@getdave the link previous to my removal of the label should tell you all you need to know. 
|
It's brand new. We added it to help manage the release. So not surprising 😄
Thank you. I'm going through a fair number of PRs right now so must have missed that link. Thanks for clarifying 👍 ✅ Backported to WP Core in: |
|
✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this has been backported and added the Trac and Core PR details. |
What?
Fixes #47386; alternative to #47451.
If appearance tools are enabled on a classic theme but no theme.json exists, an inner container is added to Group blocks, but layout classes are still added to the outer container.
Currently the filter that adds the inner container runs after the layout filter, but even if we switch their running order, the block inner content doesn't seem to change, so the layout classes never get added to the inner container.
This PR uses the HTML tag processor to remove the layout classes from the outer container and add them to the inner container, within the filter where that container gets added.
Testing Instructions
Note that this PR changes the position of the layout classnames whether appearance tools are enabled or not. The core styles applied to the default layout classnames don't appear to have any impact but this could potentially be a breaking change for themes that might have attached their own CSS to the layout classnames. I guess we could possibly put these changes behind a check that appearance tools are enabled. Thoughts?
Testing Instructions for Keyboard
Screenshots or screencast