Add inherit content size to base styles #42664
Closed
Conversation
This file contains 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
|
Size Change: +374 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is testing really well for me so far @tellthemachines, nice work figuring out how we can re-use the new baseStyles property for this case — I think it's a good confirmation that we've been heading in the right direction with the recent layout PRs, that this change wasn't too complex to implement 😀
Using the same test markup as the other PR, I can see that custom sizing still works with the wp-container-$id classname, and Group blocks set to inherit don't output the container classname:
I'm not too sure how feasible it is, but I've added a comment wondering if it'd be possible for us to combine both rules (for content and wide sizes) under the one classname? Not a big issue if it isn't possible, but just curious if the one class can handle both cases (given that there's a different child selector in use) or if it's better to keep them separate (which now that I mention it, would also be more explicit 🤔).
lib/compat/wordpress-6.1/theme.json
Outdated
Comment on lines
220
to
231
| "selector": ":where(.has-global-content-size) > :where(:not(.alignleft):not(.alignright):not(.alignfull))", | ||
| "rules": { | ||
| "max-width": "var(--wp--style--global--content-size)", | ||
| "margin-left": "auto !important", | ||
| "margin-right": "auto !important" | ||
| } | ||
| }, | ||
| { | ||
| "selector": ":where(.has-global-wide-size) > .alignwide", | ||
| "rules": { | ||
| "max-width": "var(--wp--style--global--wide-size)" | ||
| } |
There was a problem hiding this comment.
I'm wondering if we can use the same classname for these rules, since they have different child selectors.
Would it work to have a single has-global-content-sizes classname, so the second rule would become: :where(.has-global-content-sizes) > .alignwide?
There was a problem hiding this comment.
Yeah, we should be able to do that. Relatedly, I'm thinking that if there's only one of these sizes set, we should set the other to the same value. We already use this logic in layout.php so makes sense to use it here too.
There was a problem hiding this comment.
Hmm, can't seem to get fallback values to work for the variables defined in theme.json. I wonder if we're disallowing commas in the values or something?
There was a problem hiding this comment.
I suspect safecss_filter_attr has a problem with the commas, although its docs say it supports var. I'll have a look on Trac and see if there's any ticket about that.
In the meantime, I made the fallback work by always setting values for both variables to whichever value exists.
There was a problem hiding this comment.
Nice, always setting both values looks like a good way to handle the fallback for now!
| if ( isset( $used_layout['inherit'] ) && $used_layout['inherit'] ) { | ||
| if ( ! $global_layout_settings ) { | ||
| return $block_content; | ||
| } | ||
| $used_layout = $global_layout_settings; |
There was a problem hiding this comment.
For some reason I really like this line removal. To me it makes the whole behaviour in this PR feel like the layout support is a bit more logical and less magical, so it's nice we can remove this! 👍
Comment on lines
+757
to
+758
| $block_rules .= '--wp--style--global--content-size: ' . $content_size . ';'; | ||
| $block_rules .= '--wp--style--global--wide-size: ' . $wide_size . ';'; |
There was a problem hiding this comment.
Since theme.json settings aren't run through remove_insecure_styles should we add a static::is_safe_css_declaration check before outputting here? I think we'd be able to call it with max-width as the first param, and $content_size or $wide_size as the second param?
There was a problem hiding this comment.
This is looking pretty close to me, now @tellthemachines! Just added a question about where we put the logic in the theme JSON class, and I wondered if it'd be worth extending the existing tests to include the new rules? The base styles might not need to be extended in the tests, since the logic there hasn't changed, so we might only need a minimal test to ensure the content / wide size fallbacks are working for the CSS variable output, if you think it's worth it.
| * If there are content and wide widths in theme.json, output them | ||
| * as custom properties on the body element so all blocks can use them. | ||
| */ | ||
| if ( isset( $settings['layout']['contentSize'] ) && $settings['layout']['contentSize'] || isset( $settings['layout']['wideSize'] ) && $settings['layout']['wideSize'] ) { |
There was a problem hiding this comment.
Just another idle thought from working on #42544, would it be worth moving this block to the bottom of the get_layout_styles function within the static::ROOT_BLOCK_SELECTOR === $selector if statement? Mostly, I was wondering if a theme disables layout styles, should these CSS variable declarations also no longer be output?
There was a problem hiding this comment.
Mostly, I was wondering if a theme disables layout styles, should these CSS variable declarations also no longer be output?
Yes, that makes total sense!
I've transferred most of this logic to #42763 where I'm trying to solve the problem via a new layout type. It's working neatly so far because it makes the extra classname/selector unnecessary. If we decide not to go forward with that approach, I'll go on with this one, otherwise it's easier to apply this feedback directly on the other one 😄
|
Closing as this was superseded by #42763. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Companion to #42582; adds styles required for containers that use default content size to base styles, so that those containers don't need generated classes anymore.
Why?
If we're going to set content size by default on some blocks, it makes sense that the required styles be defined as base styles.
How?
Testing Instructions
Screenshots or screencast