Add compound class to layout wrapper for global spacing styles #47952
Conversation
tellthemachines
requested review from
spacedmonkey,
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
tellthemachines
requested review from
mikachan,
youknowriad,
MaggieCabrera,
andrewserong and
apeatling
tellthemachines
added
[Feature] Design Tools
|
Size Change: +40 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2b6db31. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4350087564
|
There was a problem hiding this comment.
Very clever idea @tellthemachines, nice solution for working around the idea of blocks having to explicitly declare the class for the inner wrapper. I think concatenating the block classname with the layout type is a good way to go about it, and makes the output from global styles quite explicit, too. To me, this feels like a good part of the overall stabilisation of the layout support, in addition to fixing the issue at hand 👍
From a quick test, it appears to be working well for me so far, with setting block-level spacing for Cover and Social icons blocks in global styles, with real-block-level attributes overriding the global styles values, etc.
Just left a couple of comments about seeing if we can do a lookup for the block classname instead, and a question about the addition of :root as a prefix in the rules output.
Overall, though, I think this sounds like a good direction to me!
| @@ -1282,7 +1282,7 @@ protected function get_layout_styles( $block_metadata ) { | |||
| $spacing_rule['selector'] | |||
| ); | |||
| } else { | |||
| $format = static::ROOT_BLOCK_SELECTOR === $selector ? '%s .%s%s' : '%s.%s%s'; | |||
| $format = static::ROOT_BLOCK_SELECTOR === $selector ? '%s .%s%s' : ':root %s-%s%s'; | |||
There was a problem hiding this comment.
Sorry if this is obvious, but I wasn't too sure: why does this need the :root prefix, is it to increase specificity now that it's a dash, instead of a combined classname selector? And is :root preferable over using static::ROOT_BLOCK_SELECTOR?
There was a problem hiding this comment.
I'm pretty sure I added it for specificity, but am no longer sure of the actual rule it was meant to override 😅 I can try removing it and see how we go. Good idea to use static::ROOT_BLOCK_SELECTOR if we do end up needing it!
There was a problem hiding this comment.
Ok, so a bit of testing shows that we actually need :root to override the default layout styles, because it has a slightly higher specificity than body:
Leaving it as is for now.
There was a problem hiding this comment.
Thanks for confirming!
I had another idle thought — what if we added the layout classname to the selector rather than the :root prefix, so that the rule becomes .wp-block-group-is-layout-contrained.is-layout-constrained — I think this would then have the same level of specificity as before this PR. On the other hand, that's also an unwieldy looking selector 😅
I'm quite possibly overthinking it, but I quite like the idea of the block-level-in-global-styles rules being a kind of combination of the block's classname and the layout classname, if we can get it to work, without having to add :root in order to increase the specificity. My main concern about the :root prefix is that while it works, it wasn't obvious to me from looking at the rule that it's there for the extra specificity, whereas combining classnames seems a little clearer... but that could just be me (and definitely my naivety about the :root pseudo-class 🙂), so I don't feel too strongly about it.
There was a problem hiding this comment.
Hmm that's a good point. I think :root has the same specificity as a regular classname, so we could do something like that, or even duplicate the composite selector like .wp-block-group-is-layout-contrained.wp-block-group-is-layout-contrained, so it's even more obvious that the bump is for specificity.
There was a problem hiding this comment.
I'm not sure how this is going to pan out relative to #47858 though 😅
There was a problem hiding this comment.
I'm not sure how this is going to pan out relative to #47858 though 😅
Good point! I reckon we'll likely be doing a fair bit of nudging back and forth before we land on a sweet spot 🤞 😄
tellthemachines
force-pushed
the
add/layout-to-cover
branch
from
543568c to
6082aee
Compare
tellthemachines
force-pushed
the
add/cover-spacing
branch
from
2266be3 to
473fe6b
Compare
|
Edit: Never mind on this! I remembered we don't actually do this yet. It's tripped me up before because you'd 100% expect to see the setting show up as pre-configured. Started testing this, but humour me for a second. If I set a non-custom block spacing on the cover block in global styles, it does not seem to apply to a cover block inserted into a post? Is that working for you? I've tried multiple saves etc. The global styles setting sticks for cover, but does not apply to a cover block in a new post. Set in global styles specifically for the cover block: New cover block in a post: |
The global styles value should apply to the block, because the generated styles are loaded in the editor, but it won't be visible in the block's spacing control because we've only recently had the ability to access the actual values from global styles in the post editor (since #47098) and we haven't updated the design tools to show them yet 😅 |
Thanks for the extra context, everything worked fine. It was simply the control not showing the global value that threw me off. I think we should work on that now that the ability is there since it's something that connects dots for people, and seems broken if you don't know the context. |
There was a problem hiding this comment.
Nice work @tellthemachines, this looks like it's in a good place to me, especially since this PR is targeting being merged into the layout support for the Cover block PR! If (eventually) there's ever any concern about the additional classnames being injected in blocks that don't have global styles rules applied, we could always look into a potential follow up that only outputs the additional classname if a rule exists, but I suspect that won't be necessary.
✅ Re-tested that block level gap in global styles is output correctly in the site editor, post editor, and site front-end
✅ Block-level spacing rules still override those set at the block-level in global styles
✅ Fallback / default spacing rules in classic themes still work as-is (e.g. default flex gap that gets applied to social icons blocks)
LGTM! ✨
|
Thanks for reviewing @andrewserong! I won't merge this into the Cover block PR until that one's approved, given that it will make it harder to review. We can merge it directly into trunk if we need to. |
tellthemachines
force-pushed
the
add/cover-spacing
branch
from
7a3ffe8 to
b1cbd54
Compare
tellthemachines
added
Needs PHP backport
Dev noteCompound block + layout type classname applied to inner wrapper of layout blocksAs of WP 6.2, layout classnames are added to the block inner wrapper. For most blocks, this is the same as the outer wrapper, but some blocks (e.g. core Cover) have multiple inner containers. In 6.3, a new classname is added to the inner wrapper of all blocks with layout, comprised of block classname + layout classname, e.g.: |
What?
In #45326 I found that adding spacing support to Cover block didn't work out of the box, due to Cover's nested markup structure. As of #44600, layout classnames are added to the block's inner wrapper, so the
.wp-block-cover.is-layout-flowselector used for block spacing won't work.Because spacing styles need to target only the direct descendants of the layout wrapper, simply separating the selectors e.g.
.wp-block-cover .is-layout-flowalso won't work.We need a selector that targets the layout wrapper for each particular block type. This PR implements that selector, and adds spacing support to the Cover block as a testable example.
Note: I deliberately avoided any solution that would require specifying a target wrapper classname in the layout object, because once #47807 is done it should be possible to enable/disable layout on any block from theme.json, without knowledge of the block's markup structure. Also, the whole point of #44600 was to avoid doing precisely that 😄
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Front end with global spacing:
Block editor settings: