Allow full width blocks to override parent or root container padding #39871
Conversation
glendaviesnz
added
the
[Status] In Progress
|
Size Change: +278 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
lib/block-supports/layout.php
Outdated
| * | ||
| * @return string CSS style. | ||
| */ | ||
| function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false ) { | ||
| function gutenberg_get_layout_style( $selector, $layout, $padding, $has_block_gap_support = false, $gap_value = null, $should_skip_gap_serialization = false ) { |
There was a problem hiding this comment.
Not related to this PR, but we might want to start using a single array of arguments for this function. I know BuddyPress has done similar deprecations in the past. So we could use a similar method.
I think we should follow the "two or fewer" recommendation from the Clean Code when it comes to function arguments.
There was a problem hiding this comment.
👍 The single array of arguments potentially just plasters over an underlying issue though of a method trying to do too much - would definitely be good to look at this method and see if a single method with all those arguments is the best approach for sure.
glendaviesnz
changed the title
|
This approach has some limitations as laid out in the original PR from this comment down. The intention with this PR is to further explore if there are any workarounds for those limitations. |
tellthemachines
force-pushed
the
update/layout-to-support-full-width
branch
from
0fa5b16 to
ac0b854
Compare
| @@ -143,6 +151,8 @@ export default { | |||
| } | |||
| ${ appendSelectors( selector, '> .alignfull' ) } { | |||
| max-width: none; | |||
| margin-left: calc( -1 * ${ padding?.left || 0 } ) !important; | |||
| margin-right: calc( -1 * ${ padding?.right || 0 } ) !important; | |||
There was a problem hiding this comment.
The zero here is causing an invalid property value error.
There was a problem hiding this comment.
Switched this to only add these margins when a padding is actually set, but not sure if there is a need to have them actually set to 0 if no padding set - but in my testing the padding object always seems to be undefined regardless of what padding is set on parents, so hard to tell if 0 margin is needed or not.
| ${ appendSelectors( selector ) } { | ||
| padding: ${ padding?.top || 0 } ${ padding?.right || 0 } ${ | ||
| padding?.bottom || 0 | ||
| } ${ padding?.left || 0 } !important; |
There was a problem hiding this comment.
I don't think this is the best way to go about it. If user sets a padding, we should respect that, and work with its value to add negative margins to full-width items. This is making it impossible to have a Group with both layout (needed for e.g. full width images) and padding, which is not ideal, especially if background color is set:
There was a problem hiding this comment.
This is making it impossible to have a Group with both layout (needed for e.g. full width images) and padding
Yes, I found that a bit challenging in testing, too, that toggling "Inherit default layout" hid the padding controls, which felt unexpected.
There was a problem hiding this comment.
If we keep the padding visible when we inherit a layout, we'd create another unexpected behavior though:
- A user can set a padding and this one will be used
- The user can remove the specific padding and the one from the inherited layout will take over (instead of no padding), we'd have to set
0to reset the padding.
Not sure if that's confusing enough or if we can explain it to the user.
tellthemachines
requested review from
TimothyBJacobs,
spacedmonkey and
ellatrix
as code owners
tellthemachines
requested review from
ramonjd,
youknowriad,
andrewserong,
kjellr and
scruffian
| isset( $padding['top'] ) ? $padding['top'] : 0, | ||
| isset( $padding['right'] ) ? $padding['right'] : 0, | ||
| isset( $padding['bottom'] ) ? $padding['bottom'] : 0, | ||
| isset( $padding['left'] ) ? $padding['left'] : 0 |
There was a problem hiding this comment.
Sorry, I will test this manually, but ran out of time today 😄
So this is a note for myself more than anything.
Let's say the result is padding: 10px 0 0 0;, where only padding-top: 10px was defined by the user in the editor.
Are there any problems with right, bottom and left being now 0?
What if a CSS style higher up says otherwise, and is consequently overridden?
There was a problem hiding this comment.
Oh, good point!
What if a CSS style higher up says otherwise, and is consequently overridden?
It sounds like that might be a similar challenge to what we've been discussing in the PR to add support for blockGap at the block level within global styles: #39870 (comment) — TL;DR: how do we reliably get the value that's cascaded down correctly at this point in the execution. E.g. do we need to look up global styles here as well, to get the right value?
There was a problem hiding this comment.
Actually, wait, I think I might be getting confused here — because this is a different No: it's coming from the padding that comes from the layout object instead?style object... 🤔
There was a problem hiding this comment.
E.g. do we need to look up global styles here as well, to get the right value?
I think we might, yes. Though ideally, either the block attributes would know about global styles too, or that cascade would be resolved someplace else by the time we get to layout. Is that within the scope of the style engine work?
There was a problem hiding this comment.
Is that within the scope of the style engine work?
I'm not too sure, to be honest, it's at least beyond the initial work of refactoring the existing block supports, but I think we might naturally bump into it when we go to consolidate style generation for global styles / theme.json.
There was a problem hiding this comment.
Just rounding back to this: global/theme.json block styles are overwritten for the reasons @andrewserong mentions above
Is that within the scope of the style engine work?
I see the intention is for !important to override inline styles. Works as intended, but the inline styles are still there.
Without promising anything 😄 I also wonder if the style engine will later dedupe this sort of stuff as well. Layout crosses over into spacing more and more it seems!
There was a problem hiding this comment.
Can you clarify why we need to render the padding here (I mean in layout)? Can't we assume the padding block support will just output the same thing (inline or using a style tag) later?
There was a problem hiding this comment.
I guess that's for the "inherited padding", maybe we can only output these styles if the layout is inherited to avoid duplication and avoid important?
| @@ -176,6 +176,7 @@ class WP_Theme_JSON_Gutenberg extends WP_Theme_JSON_5_9 { | |||
| 'layout' => array( | |||
| 'contentSize' => null, | |||
| 'wideSize' => null, | |||
| 'padding' => null, | |||
There was a problem hiding this comment.
Just want to make sure I understand the intention with this: I wasn't sure why we need to add an additional padding setting under the layout object. What's the difference between this, and the root level spacing.padding style value? From looking at the code, my guess is that this value is in settings, so it means it's a fallback value that can appear anywhere in the hierarchy rather than specifically meant to be applied to the root (or other block)?
If it causes confusion, is it worth contextualising this particular padding value with a prefix? (In much the same way as contentSize is an abstraction for max-width rather than calling it max-width directly)
There was a problem hiding this comment.
What's the difference between this, and the root level spacing.padding style value?
The root level value only applies to the root container. Currently it gets added to body (#39926 tries to change that logic to allow direct children of root to be full width even when there's padding set).
This value applies to any block that has layout set to inherit default, no matter whereabouts in the nesting structure it may be.
There was a problem hiding this comment.
This value applies to any block that has layout set to inherit default, no matter whereabouts in the nesting structure it may be.
Gotcha, thanks for clarifying!
There was a problem hiding this comment.
What an interesting challenge! According to the testing steps this appears to be working okay to have a block break out of one level of nesting. I was wondering, though, if full width should be able to reach the full edge of the page, rather than just the next container up?
| Before this PR | With this PR |
|---|---|
 |
 |
I found it a little hard reading through to work out the relationship between padding in the layout object and padding in the style object, and it felt unexpected to me that toggling the Layout's inherit control shows/hides the padding setting.
As Ramon mentioned, there's an interesting challenge, too, in figuring out where block-level in global styles padding fits in, and how to calculate / incorporate that into how the blocks break out of their containers.
From a quick read, though, I think most of my concerns were already summed up in this comment on the previous PR: #36214 (comment)
I've run out of time for a deeper look at the code today, but happy to do further testing tomorrow!
|
Thanks so much for all the hard work you are doing here. I'm very excited to start implementing this on our themes. I took it for a spin with one of the latest we've been working on here. The results:
I'm using this markup for testing and so far the only problem I'm seeing is with full width cover blocks, seems like the block is defining the width and that's interfering. |
|
Thanks for testing @MaggieCabrera !
The Cover block has The problem with removing that explicit width is it will cause breakage when Cover is inside a flex group. |
|
|
Thanks @glendaviesnz! Apologies, I missed that in the discussion 👍 |
|
I pushed a commit to #39926 which might work for both that PR and this one... |
scruffian
requested review from
fluiddot,
gziolo,
adamsilverstein,
talldan,
swissspidy,
ajitbohra,
ntwb,
nerrad,
ryanwelcher,
noisysocks,
draganescu,
adamziel,
kevin940726,
dmsnell,
atimmer,
mmtr,
ajlende,
juanmaguitar and
fabiankaegy
as code owners
|
It seems like unit tests needs some love. Luckily, it's just a different serialization order changed and not an actual bug: |
|
I've done a quick check on the mobile unit tests and noticed that these changes uncovered an issue on the native version. For the layout hooks , we provide a custom implementation for the native version where we disable a couple of filters because they are not supported. However, we're not re-exporting the functions from the original file, hence this is producing an error in both the editor and unit tests. I have a quick fix that I can push to this PR, @glendaviesnz would you mind if I include it? Otherwise, I can open a PR for including it into |
Sure, I can hold off the changes until including that PR 👍. Although I think we would still need the fix for the native version because it's something very specific to the custom implementation mentioned in this comment. |
|
Was superceded by #42085 |
What?
Part of the solution to #35607. This part only covers items nested within blocks that have a full width set, and also specify a content width for children.
Another solution for top-level blocks will be worked on separately.
Why?
Problem is outlined here.
How?
This PR initially just cherry picks the changes from #36214 onto a new branch off trunk - the layout module has changed significantly since that PR was opened so a rebase was problematic!
Testing Instructions
in
theme.jsonsettings.theme.jsonpadding value and child is still full width.Screenshots or screencast