Try using variable for root padding value #40926
Conversation
scruffian
force-pushed
the
add/root-padding
branch
2 times, most recently
from
705dad3
to
a768407
Compare
|
Size Change: +867 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
scruffian
force-pushed
the
add/root-padding
branch
from
30a66a6
to
a536b12
Compare
scruffian
force-pushed
the
add/root-padding
branch
from
bafbc83
to
0e90fd4
Compare
scruffian
requested review from
TimothyBJacobs,
spacedmonkey and
ellatrix
as code owners
There was a problem hiding this comment.
Thanks for splitting this out to a separate PR @scruffian! I really appreciated the testing gist, too, that made it much easier to play around with quickly 👍
It's looking quite close to me, but I ran into a few issues:
1. Body padding
Is the root body still meant to be applying padding with this setting enabled? It looks like I'm getting a double padding happening (so padding is applied directly on body as well as the CSS variable-based left/right padding on .wp-site-blocks). In the following screenshot, there should be a single 30px padding, but I see two sets of padding here, so the root blocks in my site effectively are 60px padded and I can't get it to go full width:
If I manually disable padding on body in DevTools then everything renders as I'd expect:
I'm testing using emptytheme and spacing.padding set to 30px in my theme.json file's styles object.
2. Variable output when flag is switched off
With the useRootVariables flag switched off the CSS variables are still output in the front-end of the site (it's just the styles that use these CSS variables that aren't being generated). Is that intentional?
3. How the variable is output when there is no value
If there is no value for spacing.padding then the variables appear to be generated incorrectly (no value is output):
Should there either be a default value, or if there's no value, then don't output the variable? (If we don't output the variable, then I suppose the styles that use that variable should have a fallback value?)
4. How this feature works in TwentyTwentyTwo (or existing themes)
In TwentyTwentyTwo, switching on the setting results in a horizontal scrollbar in the site editor. I'm guessing this is because the theme will needed to be updated to factor in the new setting? (This isn't a blocker, just wanted to mention it in case it's an issue — given that this new feature is guarded behind a flag, it seems fine to me if themes don't immediately support it once the feature lands 👍)
Thanks again for continuing on with this feature, it'll be really great to land it. Please let me know if you need any more info from my testing, or if I missed a step! 🙇
scruffian
force-pushed
the
add/root-padding
branch
from
0e90fd4
to
b07be22
Compare
|
Thanks for testing @andrewserong.
|
No! I have disabled this. |
Yeah I think so, I've made it 0 |
Yeah I think that's because we also need to remove the custom CSS from the theme that deals with this, so this is expected. @jffng made this PR for TT2: WordPress/wordpress-develop#2578 |
There was a problem hiding this comment.
Wonderful, thanks for implementing all those follow-ups @scruffian! This is testing nicely for me now:
✅ Root variables are not output when useRootVariables is set to false
✅ If there is no padding value, a default value of 0 is used
✅ Duplicate padding issue has been resolved
✅ Confirmed that for existing themes there is no change with this PR applied if it does not opt-in to the new useRootVariables setting
| Padding on site blocks looks good now | Negative left/right margins applies correctly to full width items |
|---|---|
 |
 |
0 fallback value is output correctly |
Variables are not output if setting is false |
 |
 |
Good to know that the fix for TwentyTwentyTwo is already in the works 👍, and given that this is an opt-in feature guarded behind a flag that defaults to false, and it's testing well, I think this PR looks in a good shape to merge. Then, folks can test it out in their themes by opt-ing in and having a play around to see if there are other tweaks we need to make in follow-ups. Thanks again for finishing this off, and kudos again to @tellthemachines!
LGTM! 🎉
| @@ -445,7 +447,7 @@ protected static function compute_style_properties( $styles, $settings = array() | |||
| // If a variable value is added to the root, the corresponding property should be removed. | |||
| foreach ( $root_variable_duplicates as $duplicate ) { | |||
| $discard = array_search( $duplicate, array_column( $declarations, 'name' ), true ); | |||
| if ( $discard ) { | |||
| if ( $discard !== false ) { | |||
There was a problem hiding this comment.
Thanks for fixing this up: such an interesting edge case here that if the shorthand padding was the first item in the array, then $discard would be 0 (the found "key" for the first item in the array) which is a falsy value, so this would never be reached. Nice catch! 🙇
andrewserong
assigned andrewserong, scruffian and tellthemachines and unassigned andrewserong
andrewserong
added
Global Styles
|
I've been testing this in a few themes and it works as described for me. Full width blocks receive the appropriate margins
Thanks for persevering with this @scruffian |
|
@youknowriad do you want to review this before I merge? |
|
To be completely honest, I'm not a fan of this PR. The reason is simple: I feel we're not addressing this conceptually by bringing a meaningful solution, instead, we're adding some CSS to all themes that target blocks randomly. It feels like hacks to me just to make the things people "usually" do in their themes look as they expect. The problem is in the "usually", here, if the theme is a bit creative, these styles will quickly become more of a burden than a real solution. They also don't take third-party blocks into consideration. -- |
|
I think if we want a more generic solution we need to add some kind of attribute to some blocks so that we know to treat them differently. That was the intent of this issue: #26407 I'm happy to look into this more if you think that direction makes senses. |
|
Or perhaps another approach is to simplify things as suggested here: #36082 |
I'm also thinking this is a layout problem. For me it's somehow related to the conceptual nut I've been trying to crack in relation to
I see a few challenges:
Maybe the approach lies in defining a canonical approach to such "global" styles. They function as subset of presets that future layout implementations can use. I don't know. 🤷 One thing I like about this PR is the If we decide to expose a set of root CSS vars globally, then I think they should all follow the same naming conventions and share similar assumptions. At any rate, they're always defined and need no theme.json flag to turn them on. Rather, the features that this PR achieves could be a related to an available layout option. See @shaunandrews's mockups here: Suggestion: Simplify the layout concept · Issue #36082 · WordPress/gutenberg · GitHub |
|
To be clear the root CSS variables is not what bothers me in this PR. I think it's fine to have root settings to be used by blocks...
I think this is where the problem is. In a block theme world: 1- either the the theme.json framework should know how to apply these values consistently and this should work for all themes. The current PR tries to do 1 (mostly) and a bit of 2 but there is not a convincing story about it. The way it tries to do 1 is not "universal", the way 2 is done is ad-hoc. I think we need to define clearly what we're doing in both cases and make sure that definition translates to all themes and blocks consistently. |
|
Thanks for the feedback @youknowriad, you raise great points! Just confirming that the main concerns here are surrounding the styles that we're outputting in I'm not too sure how we'd do it, but I wonder if a potential direction forward could be related to the idea of outputting layout styles via classname-based presets (like @tellthemachines was exploring in #39708). Basically, if instead of outputting the hard-coded rules in
I know the feeling of not wanting to be a blocker, but thank you for the push-back here, it's very much appreciated! It's always really valuable to flag things that we think might bite us further down the track, even if we don't have an immediate alternative — it's a good sign for us to experiment laterally, perhaps, to see how else we might implement things. 🙇
Good points @ramonjd — I'm cautiously optimistic that we might be able to take an approach for blockGap where the style generation goes something like:
Not sure if that helps here, but just thought I'd share my thinking there (that I'm hacking away at over in #40875) in case it sparks any ideas 😊 |
This is an interesting idea. I wonder if we could do most of the work in layout, provided that the block/theme opts into Words are failing me right now, so I have code: https://github.com/WordPress/gutenberg/pull/41042/files#diff-324697e41855298e2f2c74b078f174e0cbc9075cef736ce9c1e2c169bf64652eR144 |
Nice! The code example shows that idea well — that way of implementing it looks pretty consistent with the current way we do things with the layout support 👍. Longer-term, for these rules that aren't tied to a value at the individual block attributes, it'd be great to move the generation to global styles so that the style rule is always available (and doesn't fall into the issue we have of rendering container styles in async contexts, etc). But that's probably more part of the work in the exploration in #39708 Thanks for trying the idea in code! |
scruffian
force-pushed
the
add/root-padding
branch
from
a8399aa
to
c541bad
Compare
|
I started to look into how. we might achieve this by adding a block attribute as suggested above, but in the end I landed on a solution which is simpler. Instead of adding the block specific CSS to the root padding styles, we can just define it in the block itself. @youknowriad What do you think about this approach? Other blocks can opt in by adding that CSS... |
| @@ -3,3 +3,10 @@ | |||
| // Matches paragraph block padding. | |||
| padding: $block-bg-padding--v $block-bg-padding--h; | |||
| } | |||
|
|
|||
| .wp-block-group.alignfull { | |||
There was a problem hiding this comment.
Ok, so I think now I see what bothers me in these styles (same for cover and columns).
Basically, the style here assumes that a "full" aligned group block is necessarily a block that stretches from the left to the right of the window. Meaning it can use the "root" padding value to position itself.
The reality is that it's not the cause. "full" alignment doesn't have any relationship with "root". In other words, we can't use "root" padding values here because we have no idea what the result will be.
For instance:
- I can use two layouts in my template: one that stretched to the edge of the screen (in which root padding would make sense), but under it I might have another section that is more "centered" but inside it, I can still have "full" blocks, it's just that these "full width" blocks are "full width" within their container and not the whole screen, meaning using "root" padding for these don't make any sense.
- I can imagine having a "two columns" layout where each column of my site has its own "layout" with its own "widths", meaning there's no relationship here as well between "full width groups" and "root padding".
The only way I can see this PR moving forward is for us willing to accept this:
"full" width alignment means a block stretches from the left edge to the right edge of the window. I think it's a big assumption and a big breaking change from what we've been doing/allowing so far.
There was a problem hiding this comment.
I don't believe that's the case with this PR as it is. Here's a screenshot from Remote, which is a two column theme:
There was a problem hiding this comment.
<!-- wp:columns {"align":"full"} -->
<div class="wp-block-columns alignfull"><!-- wp:column -->
<div class="wp-block-column"><!-- wp:group {"backgroundColor":"vivid-red"} -->
<div class="wp-block-group has-vivid-red-background-color has-background"><!-- wp:paragraph -->
<p>test</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>tes</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>test</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:column -->
<!-- wp:column {"layout":{"contentSize":"","wideSize":""}} -->
<div class="wp-block-column"><!-- wp:group {"layout":{"contentSize":"200px","wideSize":"300px"}} -->
<div class="wp-block-group"><!-- wp:group {"align":"full","backgroundColor":"luminous-vivid-orange"} -->
<div class="wp-block-group alignfull has-luminous-vivid-orange-background-color has-background"><!-- wp:paragraph -->
<p>test</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>tes</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph -->
<p>test</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group --></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->
I'm using the empty theme, I just defined a padding for the root and enabled the "root padding" rule. With the content above, you can see some weird overflows happening on the second column.
There was a problem hiding this comment.
Please could you post a screenshot? This is what I see:
There was a problem hiding this comment.
yes, if you inspect the "orange" column. you'll see that it has a margin/padding that make it overflow on top of the first column. Basically, it's on top of the "column" block gap and a bit on top of the first column as well. It's more noticeable if you use a big padding on the root.
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 Improve the logic for warnings for Post Comments Form placeholder (#40563) * Improve the warning for Comments Form placeholder * Fix typo on showPlaceholder variable name Co-authored-by: Luis Herranz <luisherranz@gmail.com> Add optional chaining in case taxonomy visibility is not defined (#40532) Co-authored-by: Glen Davies <glen.davies@a8c.com> Merge remote-tracking branch 'origin/trunk' into try/root-padding-fix Added missing doc comment for parameter "$use_root_vars" Add alignments rules for blocks within post content Remove default padding match alignment rules to those used in blockbase move root padding rules to a new method revert unconnected change revert spacing change add docblock for new method remove file' fix merge mess formatting fix merge mess fix alignment append to ruleset fix issue in site editor match the styles for the site editor to those used in the front end formatting change move new code to 6.1 compat files fix linter
scruffian
force-pushed
the
add/root-padding
branch
from
5328524
to
e3226c0
Compare
|
@youknowriad I have been thinking more about this, and I'm struggling to think of a real world use case for nested alignfull blocks. What do you think about simply disabling this feature? |
I thought about it some more and there are some use cases, for example: |
|
What makes this hard is that when you set a block to be |
This is why I think #39871 is a better approach to solve this holistically no matter which context (and no need for CSS variables because these are faulty in flow layout) |
What?
This is a new version of #39926. I squashed all the commits to make the rebase easier, but I wanted to keep the history, so I opened a new PR instead. Props to @tellthemachines for all the work on the original PR.
This:
theme.jsonso themes can opt in to the new behaviour of root paddingWhy?
Partly fixes #35607 - only addresses the root block part of the equation. #39871 deals with full width in nested blocks.
Fixes #35884 for themes that opt into this change.
Themes shouldn't have to add their own rules to make full width alignments work properly.
How?
This allows users to set a global body padding in the global styles interface.
Then, on the server side, we set top and bottom padding to the root container, but left and right padding to the direct children of root. This is so that those children still stretch full width as expected, but their children get the effects of the padding.
On the site editor side, the direct children of root container get the left/right padding applied, so they still appear as full-width. (If we wanted to only apply full-width to some of the root children, maybe we could add padding to the root and enable the align control on them so they could be set manually? And add negative margins to
alignfullblocks)On the post editor side, the root container also gets its left/right padding, and any
alignfullchildren are fixed with negative margins.This is achieved using the alignment rules that are currently used by block themes.
Testing Instructions
"useRootVariables": trueto thesettingsof your theme'stheme.jsonto enable these changes."useRootVariables": truefromtheme.jsonreverts to the previous behaviour.Screenshots or screencast
You can also use this block markup to test: https://gist.github.com/scruffian/df0ce92c85cffdfee0d69fbe2bcb8995
cc @WordPress/block-themers