Core CSS support for root padding and alignfull blocks #42085
Conversation
tellthemachines
force-pushed
the
try/another-root-padding
branch
from
6093046 to
892dac1
Compare
|
Size Change: +5.52 kB (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
tellthemachines
changed the title
tellthemachines
added
[Type] Enhancement
tellthemachines
requested review from
ramonjd,
jffng,
MaggieCabrera,
andrewserong,
scruffian and
glendaviesnz
|
Would be good to get some feedback from @WordPress/gutenberg-design too! |
There was a problem hiding this comment.
Thanks for the ping @tellthemachines. This is testing well for me at the root level, but I didn't quite follow how it's meant to work for nested blocks. Apologies, I quite possibly missed something obvious while testing at the end of the day!
I ran into an issue with trying to use the Use global padding toggle within the editor. It appeared that the toggle doesn't seem to be doing anything (tested with emptytheme and TwentyTwentyTwo):
My understanding is that the idea of that toggle, is that if the theme has global padding set, that it then applies that padding to the current group block?
Possibly unrelated to this PR, but I also found it tricky to work out when I could or couldn't have children of the Group block be set to full width. It looks like the container Group block needs to be set to Full width, in order for children to be able to be set to Full width?
Happy to do more testing tomorrow!
lib/block-supports/layout.php
Outdated
| @@ -64,6 +65,17 @@ function gutenberg_get_layout_style( $selector, $layout, $has_block_gap_support | |||
| $style .= "$selector .alignfull { max-width: none; }"; | |||
| } | |||
|
|
|||
| // Handle negative margins for alignfull children of blocks with custom padding set. | |||
| // They're added separately because padding might only be set on one side. | |||
| if ( isset( $block_padding['right'] ) ) { | |||
There was a problem hiding this comment.
With this rule, is it only useful when the block uses inherit? Without setting inherit I wasn't able to access the full alignment on nested child blocks. If so, I was wondering if we might be able to make this rule a little more conditional, to potentially reduce the amount of times we need to output these additional rules?
Update: I think that might have been because my container Group block wasn't also set to full width 🤔. If so, (that it's conditional on state), then maybe that's another potential conditional to use so that we only output these styles when needed?
There was a problem hiding this comment.
It should work whenever a block has content width set, whether it's through inherit or setting an explicit content width on the block. Theoretically it's already being output only when needed, because blocks without content width won't even go through this layout logic. We shouldn't need them to be full width either, but I'll re-test tomorrow to see if I missed anything!
There was a problem hiding this comment.
Oh wait you're right, this bit will always run even if the block has no content width. Makes sense to put in a condition then.
| @@ -101,6 +105,17 @@ export default { | |||
| 'Customize the width for all elements that are assigned to the center or wide columns.' | |||
| ) } | |||
| </p> | |||
|
|
|||
| <ToggleControl | |||
| label={ __( 'Use global padding' ) } | |||
There was a problem hiding this comment.
Should the toggle for this one be displayed conditionally? (E.g. if there is no global padding, do we disable the option to opt-in to using it?)
There was a problem hiding this comment.
Ooooh good point, I'll look into it!
There was a problem hiding this comment.
Oh, just having a second look at this one, I suppose the conditional needs to be that the root variables exist (or useRootVariables is true) and that the value is non-zero?
There was a problem hiding this comment.
Yeah, this is going to be tricky. We should be able to get useRootVariables from the block editor settings, but I'm not sure how to access the actual value of the padding from here. Root padding value is set on the root element, which behaves as if it were any other block for most purposes. I'm not sure we'd want to get the value from editor-styles-wrapper with a DOM query 😅 but I can't think of any better way to do it.
There was a problem hiding this comment.
I'm not sure we'd want to get the value from editor-styles-wrapper with a DOM query 😅 but I can't think of any better way to do it.
In that case, maybe it isn't worth the trouble to check the padding value directly. Perhaps it's enough that if a theme is opting in to useRootVariables, we can assume that they're most likely going to use a non-zero value (🤞)! 🙂
|
Thanks for the review @andrewserong !
Yup, that's the idea. For it to work, we have to set
This is a very good question; it took me a while to work it out too 😅 So for children of a block (Group or any other that has layout capabilities) to display the full width control, the parent block has to have a content width set. So, either it inherits the theme content width, or it has a custom width set in the sidebar under "Layout". If we want the child block to stretch to full viewport width (as opposed to just the full width of its parent block), then the parent block also has to be set to full width (as well as any other blocks with content width it may be nested inside!) It doesn't feel super intuitive, but the idea is that if a container block doesn't have a content width set, its children naturally take up its full width, so there's no need for explicit full width controls. And full width is considered to be the full width of the container block, which may not always be full viewport width. |
|
Forgive my ignorance, but why don't we just add the padding control, and let design tokens do the heavy lifting? IE if my 'global padding' is set to 'medium', I just apply that same value to whatever block I'm working with. |
|
@jameskoster we want to avoid having to set the same padding manually on all the blocks we'd likely want to use it on - which in most cases would be all blocks that extend full width of the viewport, and contain other blocks within them. There's an extensive discussion regarding how root padding should behave in #35607, if you would like to know more! |
andrewserong
force-pushed
the
try/setting-layout-class-names-in-theme-json-for-blockgap-support-at-block-level
branch
from
e33cc4e to
0438461
Compare
|
Thanks for the extra info @tellthemachines!
Ah, I can see now that with the Use global padding flag set to Now that I'm having a play with it again, and following on from James mentioning design tokens, there's an interesting feature that @glendaviesnz landed in #41990 which extended the style engine to support the spacing presets CSS variables. I wondered if another option for storing the desired padding value could be to use the root padding CSS variable in the Happy to do more detailed testing once this has been rebased — thanks again for your patience with all the changes in the other Layout PR! 🙇 |
tellthemachines
force-pushed
the
try/another-root-padding
branch
from
afbe6af to
3b2ee09
Compare
After re-checking out this branch after the rebase, and working through the testing steps again, this is working for me now! I'm 99% sure I must have just missed a step in my earlier testing. 🙂 |
Thanks, good to know! I was wondering what might have gone wrong 😅
Hmm, would be good to have some design input on that! From the user perspective, it would probably make more sense to have the padding controls together, but on the other hand root padding only exists for blocks with flow layout, so will it be confusing if it only appears for some of the blocks with padding controls? I don't think there are any core blocks with layout that don't have padding, but it's always possible third party blocks might exist, so we also have to think about what that might look like. (Or should Dimensions and Layout be grouped as a single section? In my mind at least spacing is hard to separate from layout.) From a dev perspective, because the root padding logic is so tied with layout, it's certainly cleaner to have the setting in the layout object. But I'm sure we can find workarounds if it's for the sake of better UX 😄
Thanks for pointing me towards that PR, I'll have a look at it! |
So what would happen if I toggle "Use global padding" on, then set a different value in the dedicated Padding control? How do those options interact? |
The custom padding control overrides the root padding value. If you then remove custom padding, root padding will apply again. It's worth pointing out that root padding is only applied on the left and right sides of the block, because its main aim is to provide a consistent distance from the edges of the viewport for blocks that go full width. Top and bottom root padding values are only applied to the root container, or The other thing worth taking into account is that a root padding value can be set by the theme, and overridden in the global styles panel by the user, or it can be set in global styles if the theme doesn't provide it, but the behaviour implemented in this PR, where root padding can be applied to nested containers, has to be enabled at the theme level. This was considered necessary for back compat, so that behaviour doesn't change for existing themes, otherwise I think it would make more sense to have it enabled by default. |
|
Please check #42913 |
|
I've found a potential issue with the padding visualizer and
If I remove as shown in this GIF:
The curious part here is, it's always 60px and it's only when Edit: this issue furthermore appears to be site editor only! And since the top header is exactly 60px tall, one might assume the issue could be with the positioning assuming the header is to be compensated for, even if it sits inside the site editors iframe which negates it. The curious thing is that this math is correct in the site editor, if |
This change backports the following changes from Gutenberg repository: - [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087 - [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792 - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665 - [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085 Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`. Props andrewserong, aaronrobertshaw, isabel_brison. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54159 602fd350-edb4-49c9-b593-d223f7449a82
This change backports the following changes from Gutenberg repository: - [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087 - [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792 - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665 - [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085 Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`. Props andrewserong, aaronrobertshaw, isabel_brison. See #56467. Built from https://develop.svn.wordpress.org/trunk@54159 git-svn-id: http://core.svn.wordpress.org/trunk@53718 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This change backports the following changes from Gutenberg repository: - [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087 - [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792 - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665 - [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085 Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`. Props andrewserong, aaronrobertshaw, isabel_brison. See #56467. Built from https://develop.svn.wordpress.org/trunk@54159 git-svn-id: https://core.svn.wordpress.org/trunk@53718 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This change backports the following changes from Gutenberg repository: - [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087 - [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792 - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665 - [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085 Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`. Props andrewserong, aaronrobertshaw, isabel_brison. See #56467. Built from https://develop.svn.wordpress.org/trunk@54159
bph
added
the
Needs Dev Note
This change backports the following changes from Gutenberg repository: - [WordPress/gutenberg#40875 gutenberg#40875] Layout: Use semantic classnames, centralize layout definitions, reduce duplication, and fix blockGap in theme.json - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42087 gutenberg#42087] Theme.json: Add block support feature level selectors for blocks gutenberg#42087 - [WordPress/gutenberg#43792 gutenberg#43792] Global Styles: Split root layout rules into a different function gutenberg#43792 - [WordPress/gutenberg#42544 gutenberg#42544] Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles gutenberg#42544 - [WordPress/gutenberg#42665 gutenberg#42665] Layout: Reduce specificity of fallback blockGap styles gutenberg#42665 - [WordPress/gutenberg#42085 gutenberg#42085] Core CSS support for root padding and alignfull blocks gutenberg#42085 Note that it doesn't entirely port over PR40875 — the remaining PHP changes for that PR will be explored in a separate PR targeting `layout.php`. Props andrewserong, aaronrobertshaw, isabel_brison. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54159 602fd350-edb4-49c9-b593-d223f7449a82
What?
Fixes #35607; alternative to #42034.
Why?
Because root padding doesn't always make sense applied only to the root element (e.g.
body). As explained in #35607, we should be able to apply it instead to full-width blocks at any level of nesting.How?
theme.json.bodyin the front end,.editor-styles-wrapperin the editor);UPDATE: As of 02fb1dd, this PR no longer adds the root padding toggle to blocks without content width. This part of the work will be dealt with separately, as suggested here
Testing Instructions
"useRootPaddingAwareAlignments": trueto the settings of your theme's theme.json to enable these changes."useRootPaddingAwareAlignments": truefrom theme.json reverts to the previous behaviour (root padding is applied only at the root level, so no blocks extend to full viewport width)Screenshots or screencast
Trunk:
This branch without "useRootVariables" enabled in the theme:
This branch with "useRootVariables" enabled in the theme: