Full width alignments and root padding combined. #41377
Conversation
scruffian
added
the
[Feature] Themes
scruffian
requested review from
TimothyBJacobs,
spacedmonkey and
ellatrix
as code owners
|
Size Change: +745 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
@scruffian can you please verify that this is the definition of Given the depth and complexity of this issue we will want to try to help define what the Testing Step: |
|
I've done some testing of this PR while matching the Testing Instructions above. One issue I noticed is that the 3rd image on the page (using the markup provided in the Gist) with caption: "This one is VERY wide. So wide that I would call it FULL WIDTH. This full width image should go all the way to the edge of the viewport." However, the image does not go full width and does not touch the edges of the viewport. Note: I did go into the Site Editor and add Here is a screenshot of this particular issue: |
There was a problem hiding this comment.
Thanks for working on this! It's not a straightforward issue and part of the problem is we're not all agreed on what the behaviour should be.
The problem is that the root padding PR assumes that blocks that implement "inherit default layout" will always be at the root of the document, so it uses .wp-site-blocks > * > .alignfull to target them.
The root padding PR makes no assumptions about "inherit default layout". Its only purpose is to allow full width for direct children of the site root, including any direct children of those children (hence the .wp-site-blocks > * > .alignfull). It was purposely tackled separately from full width in all nested blocks (covered in #39871) because there was no single satisfactory solution for both problems at once.
However this is not always the case - the current behaviour in trunk is that alignfull items inside the outermost block in the document that implements inherit default layout will stretch to the edge of the screen.
This only happens if the block set to inherit default layout is either a direct child of the root block, or is itself set to full width. If it's not, then its full width children will only stretch to the limits of the block.
The approach here is to set the negative margins to counteract the root padding on all .alignfull blocks, and then to override this with more specific negative margins for inner alignfull blocks.
Not all containers of alignfull blocks have any padding at all, so blanket setting negative margins on all of them may not be the best approach.
I think overall it would be best to assume as little as possible: whether about the behaviour of "inherit default layout" (should it also use the root padding? it's unclear at the best of times what this setting is supposed to do.), or the padding of parent blocks in general. I kind of like the idea of the default gap value being set as default root padding, but I wonder if even that isn't over-assuming too.
| // Add padding to full wide blocks that inherit default layout, so the content doesn't touch the edges. | ||
| $style .= "$selector > .alignfull {"; | ||
| $style .= isset( $padding['left'] ) ? sprintf( 'padding-left: %s !important;', $padding['left'] ) : ''; | ||
| $style .= isset( $padding['right'] ) ? sprintf( 'padding-right: %s !important;', $padding['right'] ) : ''; |
There was a problem hiding this comment.
$padding['left'] and $padding['right'] are coming from core theme.json, so they'll never not be set. This has the side-effect of adding a --wp--style--block-gap-sized negative margin to any full-width child of a default layout block, whether a root padding is set by the user or not.
Also, do we really need to use !important here?
There was a problem hiding this comment.
$padding['left'] and $padding['right'] are coming from core theme.json
Not always. They can be coming from the parent block's padding settings (I think!)
There was a problem hiding this comment.
Also, do we really need to use !important here?
Probably not, this is just a PoC to see if the idea is even workable :)
There was a problem hiding this comment.
Oh wait I see what you mean, this should be passed default padding...
| opacity: 1; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this change unrelated?
|
Thanks for taking a look...
This is a problem because full width blocks only make sense inside blocks that inherit default layout. Without that setting toggled then all child block content will already stretch to the edge of the parent block. Alignfull isn't an option unless the parent has "inherit default layout". (I notice that this isn't the case for direct children of the post content block, but I think that's the only case and that it's a bug. I'm not sure we have yet created a theme that doesn't apply "inherit default layout" to the post content block.)
It depends what themes you're looking that. The problem is that the behaviour of alignments is defined by the theme not by core, so there is no consistent model for it. I've been basing my work on the alignment styles in TwentyTwentyTwo and the themes that Automattic has created. However in the cases I've looked at I believe that the first "inherit default layout" block can be nested as deep as you like, and as long as none of the parents have padding set on them, align full blocks will go right to the edge of the screen.
This is true, but because we need to set that on the first level, then we need to on the lower levels in order to override that setting, even if it just means putting the margins back to zero.
The problem we have at the moment is a lot of themes already make a lot of assumptions about how this works. In general they all try to replicate the same behaviour and simply copy/paste the same code between every theme. My intention in this PR is to replicate that behaviour in core so that themes no longer have to use this CSS. I believe that approach taken by themes has evolved to best describe the behaviour that the majority of themes want from this setting. My concern with taking a different approach in core is that themes will continue to ship the custom CSS they already have as it gives them the behaviour they want, which I feel would make this a somewhat futile exercise. I do agree that this model is hard to understand, and we should probably look into adding other, simpler layout options, but this one already has a history that would be hard to escape I think. |
github-actions
bot
added
the
[Status] In Progress
cbravobernal
removed
the
[Status] In Progress
To be honest I'm not sure inherit default layout makes sense as a control at all. There's quite a bit on that in #36082, but for the root padding part of #35607 we're only looking at blocks that are direct children of root. Those may not necessarily be Group blocks, so they may not have the inherit default setting (e.g. they might be a Cover block). That's why we ignored it.
I'm not sure I get it: should we or shouldn't we allow direct children of post content to go full width?
This is why we tried separate solutions for root level blocks and nested blocks: at root, we can simply add the root padding to the blocks themselves while allowing them to go full width, whereas for nested blocks we need to calculate based on whatever padding they have. An overall solution will likely result in code bloat, with no real benefit attached. An alternative approach for root padding if we didn't want all root blocks to be full width by default could be to allow root blocks to set themselves as full width or not, when root padding is set. I feel that would be more intuitive UI-wise, not sure if there are good reasons not to do it though.
This makes sense but we do need to make sure the assumptions we're dealing with are solid, and consistent with the UI options we're trying to provide. |
I think that should depend on whether or not the post content block has the "inherit default layout" option selected. |
|
I think it's best to manually set the outer side padding using the group block exlusively(from gutenbergs UI), specifcally on a group block that has inherit default layout set to on. The only thing that's lacking in my opinion is the ability to set dyanmic padding I.E. padding that responds to variable screen sizes. If a theme author had the ability to define custom sizes for side padding E.G. max(3.5vw, 16px) or clamp(16px, 3.5vw, 32px) and be able to set that manually from the group block UI that would be awsome! I've been working on my own theme for a while and went through plenty of prototypes as iv'e been learning gutenberg, for my last protoype i used a block style on group blocks to set the sites outer side padding. On my current prototype I'm using a custom class in the additional classes section as an alternative, if i could set a custom size directly from the gutenberg UI, for me that would be a perfect solution to setting a sites (responsive) side padding. |
Root vars shouldn't apply to block global styles Fix issue with shorthand padding on server side. Temp fix for file path error Fix site editor top padding not updating Support shorthand properties in site editor. Fix full widths in the post editor Check selector inside function. Fix kebab-casing of CSS variables. Fix borked merge conflict solving Move post editor full width styles to edit-post package. Set default padding value to 0. Update test string. Fix PHP unit tests Fix PHP lint Fix failing PHP tests. Use block gap variable as default root padding value. Fix linting errors. Add opt-in setting via package.json Generate correct block editor styles Fix tests and add prop to core theme.json Fix unit tests properly this time Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix Added missing doc comment for parameter "$use_root_vars" fix rebase
scruffian
force-pushed
the
combine/root-padding-full-width
branch
from
9c506cb to
75bf1d7
Compare
|
I rebased this and it broke... |
|
Now that #42085 has been merged, can we close this one? |
|
Yes! |
What?
This combines #39871 and #39926 which makes it clear that there are some shortcomings with the approach in these PRs.
The problem is that the root padding PR assumes that blocks that implement "inherit default layout" will always be at the root of the document, so it uses
.wp-site-blocks > * > .alignfullto target them.However this is not always the case - the current behaviour in trunk is that alignfull items inside the outermost block in the document that implements
inherit default layoutwill stretch to the edge of the screen.I have made some adjustments so that we can achieve the aims of both PRs...
Why?
So we can remove custom CSS from block themes.
How?
This is difficult to achieve, because we want the padding set on the root element, but the negative margins set on the outermost
.alignfullblock. The approach here is to set the negative margins to counteract the root padding on all.alignfullblocks, and then to override this with more specific negative margins for inneralignfullblocks.To achieve this it was necessary to add a utility class (
.wp-containter-default-layout) to all blocks that inherit the default layout, so that we can target those that are more than one level deep. I appreciate this is convoluted, but it's the only way I could get this to work.Another benefit of the approach here is that we don't have to force blocks that implement "inherit default layout" to use the root padding - they can have their own padding values, which should work, and the alignfull blocks should adjust for this. I haven't yet implemented this, but I believe it will work.
Right now this is only working in the frontend
Still to do:
-[ ] Remove the code that prevents users from changing padding on "inherit default layout" blocks
-[ ] Update WP_Theme_JSON to use the set padding values if they are defined
-[ ] Move WP_Theme_JSON changes to the 6.1 compat dir
-[ ] Update the CSS in the editor to match the changes in the frontend.
Testing Instructions
Screenshots or screencast
TODO
@WordPress/block-themers