Don't apply root padding to nested layout blocks#43842
Merged
tellthemachines merged 3 commits intotrunkfrom Sep 9, 2022
Merged
Don't apply root padding to nested layout blocks#43842tellthemachines merged 3 commits intotrunkfrom
tellthemachines merged 3 commits intotrunkfrom
Conversation
tellthemachines
requested review from
andrewserong,
ndiego,
ramonjd and
richtabor
tellthemachines
added
[Type] Bug
|
Size Change: +4.08 kB (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
richtabor
reviewed
Sep 6, 2022
| $block_rules .= '.has-global-padding > .alignfull { margin-right: calc(var(--wp--style--root--padding-right) * -1); margin-left: calc(var(--wp--style--root--padding-left) * -1); }'; | ||
| $block_rules .= '.has-global-padding > .alignfull > :where([class*="wp-block-"]:not(.alignfull):not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol) { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left); }'; | ||
| $block_rules .= '.has-global-padding :where(.has-global-padding) > .alignfull { margin-right: 0; margin-left: 0; }'; | ||
| $block_rules .= '.has-global-padding > .alignfull > :where([class*="wp-block-"]:not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol) { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left); }'; |
Member
There was a problem hiding this comment.
Shouldn't this also have :not(.alignwide) included as well? Since the .has-global-padding class is already applied to the parent block?
Currently wide width blocks inside a group block with global padding still get 2x the proper padding.
Member
There was a problem hiding this comment.
Seems like the remaining issue occurs when a wide width block is within a fullwidth block (that's not omitted from the above code).
Member
There was a problem hiding this comment.
It's especially noticeable when you add padding to the parent group block, regardless of constrained layout or not.
Editor:
Front-end:
You can try this snippet, with Twenty twenty Three to replicate.
Member
There was a problem hiding this comment.
Thanks for digging in on this — I know its a tricky one.
Contributor
Author
There was a problem hiding this comment.
Yeah, that padding will be applied to any block inside the nested alignfull block that's not also alignfull; you can see it also gets applied to a regular width block:
(the yellow Group blocks are outside the full width Group and the Media&Text ones inside it)
This is also complicated by the box-sizing property on the blocks that receive padding: some of them are set to border-box (e.g. Media&Text) and others to content-box (e.g. Paragraph, Image) so results vary depending on the block types.
I'm thinking we could target this extra padding rule only to children of alignfull blocks that don't have content width set, as they're the ones that really need that padding? Whereas in blocks with content width set it's more likely to break the alignment.
Contributor
Author
There was a problem hiding this comment.
Ok I've tried ignoring children of alignfull blocks with content width.
andrewserong
reviewed
Sep 9, 2022
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
This is testing pretty well for me @tellthemachines! With @richtabor's provided markup, when I switch the Media & Text block from wide alignment to full alignment, the nested full width alignment block no longer has extra padding applied 👍
Just left a couple of comments — now that the rules are fairly complex, it'd be good to try to add some inline comments explaining each rule, to aid future debugging. The only blocker I ran into was with an issue with the output in global styles, but looks like it should be a fairly simple fix 🤞
Comment on lines
609
to
612
| 'padding-right: 0; padding-left: 0; padding-top: var(--wp--style--root--padding-top); padding-bottom: var(--wp--style--root--padding-bottom) } .has-global-padding { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left); } .has-global-padding :where(.has-global-padding) { padding-right: 0; padding-left: 0; } .has-global-padding > .alignfull { margin-right: calc(var(--wp--style--root--padding-right) * -1); margin-left: calc(var(--wp--style--root--padding-left) * -1); } .has-global-padding :where(.has-global-padding) > .alignfull { margin-right: 0; margin-left: 0; } .has-global-padding > .alignfull:where(:not(.has-global-padding)) > :where([class*="wp-block-"]:not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol) { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left);.has-global-padding :where(.has-global-padding) > .alignfull:where(:not(.has-global-padding)) > :where([class*="wp-block-"]:not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol) { padding-right: 0; padding-left: 0; }'; | ||
| } | ||
|
|
||
| ruleset += '}'; |
Contributor
There was a problem hiding this comment.
This line appears to break global styles output for me. Can we split this into multiple lines so that it's easier to read / work out where we're concatenating } characters? (I think one of the rules is missing a closing brace, and the very final rule has an extra closing brace since the below ruleset += '}'; line will add a closing one)
Contributor
Author
There was a problem hiding this comment.
Ooooh well spotted, yeah this needs to be more legible 😅
Comment on lines
822
to
824
| $block_rules .= '.has-global-padding :where(.has-global-padding) > .alignfull { margin-right: 0; margin-left: 0; }'; | ||
| $block_rules .= '.has-global-padding > .alignfull:where(:not(.has-global-padding)) > :where([class*="wp-block-"]:not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol) { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left); }'; | ||
| $block_rules .= '.has-global-padding :where(.has-global-padding) > .alignfull:where(:not(.has-global-padding)) > :where([class*="wp-block-"]:not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol) { padding-right: 0; padding-left: 0; }'; |
Contributor
There was a problem hiding this comment.
Not necessarily a blocker, but now that these rules are fairly complex, is it worth adding a comment before each line about what the rule does? The other rules in this function feel fairly self explanatory to me, but just thinking of our future selves if we want to update these rules at any point, so that we don't forget why each line exists.
Contributor
Author
|
Thanks for the review @andrewserong ! I included some inline comments on what those CSS rules do. Hopefully they're clear enough! I'll also be working on a dev note for this functionality next week, that will further document the rules and our assumptions about how they should work. Once the feature's in Core we should add that documentation to the handbook. |
andrewserong
approved these changes
Sep 9, 2022
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
Thanks @tellthemachines, those added comments are super helpful and make it much easier to read 👍
This appears to be testing well for me in the site editor as well as the post editor now. It might be worth getting another pair of eyes from @richtabor or others to confirm that the fix largely addresses the pain points. But, given the RC for the next Gutenberg release isn't until next Tuesday, we could always merge and follow-up separately with any required tweaks, too.
LGTM! 🎉
Contributor
Author
|
I'll go ahead and merge this; if anything's still off we can address it in a follow-up. |
andrewserong
added
the
[Feature] Layout
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
Fixes #43095 by resetting each root padding rule so it doesn't apply where there are nested
.has-global-paddingselectors. The new rules added simply negate the existing rules if the.has-global-paddingblock is nested inside another.has-global-paddingblock.Behaviour before fix
.has-global-padding;.alignfulldirect children of blocks with.has-global-padding;.alignfullblocks with negative margins.Expected behaviour with fix applied
alignfullchildren will get negative left/right margins;alignfullblocks with negative margins mentioned in 2..I've tested this with a bunch of different nesting patterns and it seems to be working as expected. The one place where there can be a bit of weirdness is in the post editor view, because the
.has-global-paddingclass isn't output at root level even if the Post Content block has layout set. This means that a block with layout in the post editor might display some extra padding:which won't appear in the front end view:
I think the only way to fully fix that is to make it possible for the post editor to be aware of the Post Content block settings, which we should look into doing separately from this fix.
Testing Instructions
"useRootPaddingAwareAlignments": trueinside the settings object;Screenshots or screencast
Before:
After:
