-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Style engine: generate root global styles from global styles object #42143
Conversation
ae9d7ec
to
cfa557e
Compare
a5408e3
to
b2c2734
Compare
@@ -413,12 +448,17 @@ public function get_block_supports_styles( $block_styles, $options ) { | |||
foreach ( $definition_group_style as $style_definition ) { | |||
$style_value = _wp_array_get( $block_styles, $style_definition['path'], null ); | |||
|
|||
// If the style value contains a reference to another value in the tree. | |||
if ( isset( $style_value['ref'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rise in new global styles features such as ref
and quirky ones like --wp--style--block-gap
makes me think we can extract parsing to a new class, where this stuff can take place.
Integrating root style generation into global styles.
Added prettify option.
Pleasing the linter Remove elements const
…, which moves things to the layout implementation.
b2c2734
to
5cc7119
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work digging into this @ramonjd! Just added a couple of thoughts while reading through — they might not be immediately relevant, but thought I'd add them in case it helps inform any direction at this early stage 🙂
$global_stylesheet .= $root_level_styles['css'] . ' '; | ||
} | ||
|
||
// @TODO Layer 1: Elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a question for a follow-up PR instead, but just curious about the potential layers in follow-ups. Currently the theme JSON class iterates over nodes, so generate_global_styles
is currently called once in this PR for the root. In a follow-up, would we call generate_global_styles
in each loop as it happens now, or would it the iteration happen in the style engine?
(I wasn't sure which would be ideal, so curious to hear your thoughts about the role/responsibility of the theme JSON class versus style engine here).
One way I was imagining, incorporating the ideas from #42222 is that (long term) we could potentially call the style engine once per iteration of the loop in the theme JSON class, but instead of it returning a css string immediately, it adds the rules to the style engine CSS rules store, and then once it finishes looping over each of the nodes, it calls the style engine method to retrieve all the generated CSS for output? I don't have any strong ideas about how any of that should work, so just thinking out loud 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these thoughts @andrewserong !
In a follow-up, would we call generate_global_styles in each loop as it happens now, or would it the iteration happen in the style engine?
Very good questions.
To be honest, I don't know. 😄
The main motivation behind this PR was to experiment with throwing the entire merged theme.json at the style engine and generating an entire "global styles" stylesheet. I think this could be a cool feature of the public API anyway.
So you're right, this approach won't work in the current foreach ( $block_nodes as $metadata )
loop, but we eventually wouldn't need those loops if the style engine could handle an entire theme.json.
we could potentially call the style engine once per iteration of the loop in the theme JSON class, but instead of it returning a css string immediately, it adds the rules to the style engine CSS rules store, and then once it finishes looping over each of the nodes, it calls the style engine method to retrieve all the generated CSS for output
This sounds like it would be a more sensible approach, especially if it keeps any custom values parsing (such as fetching those ref
values) in WP_Theme_JSON for now.
It's also similar to the approach we had in mind for block supports, so it would make doubly more sense to have a global styles analogue.
We could put this PR to the side and concentrate our efforts on the store, which we'll need first anyway. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could put this PR to the side and concentrate our efforts on the store, which we'll need first anyway. What do you think?
Yeah, that might be a good next step. Each of these explorations is a great way of working out which puzzle piece to do next, and I think the store is probably the thing that's most helpful for illuminating the path forward for this PR. It'll probably also be the thing that highlights the value of switching over to the style engine? (The clearer separation between generating and outputting styles)
But this PR's great for highlighting how we can hook in the style engine, and it's really exciting seeing all the style engine efforts starting to come together like this!
array( 'selector' => $selector ) | ||
); | ||
|
||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the simple if/else to figure out how we can incrementally switch over to the style engine by just tackling root to begin with 👍
|
||
// If the style value contains a reference to another value in the tree. | ||
if ( isset( $value['ref'] ) ) { | ||
$value = static::get_ref_value( $block_styles, $value['ref'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this assume that $block_styles
will always contain the whole tree? I was wondering if for get_ref_value
we might need an additional param for the complete tree (for when we add in elements / block level styles)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it'd have to in this scenario, though it'll fail reasonably gracefully if the value isn't found.
It's another argument for the one-node-at-a-time approach discussed above: letting WP_Theme_JSON do the ref value fetching for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to have a method that accepts an entire theme.json styles tree, then it could be a separate one where we'd assume that the entire tree is passed, e.g., wp_style_engine_generate_stylesheet
or 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's another argument for the one-node-at-a-time approach #42143 (comment): letting WP_Theme_JSON do the ref value fetching for us.
It's so fascinating these sorts of problems: even just the wording of your comment there makes it feel clearer that the theme JSON class is probably a better place to do ref lookups (as the owner of that data structure), and the style engine should probably treat whatever it's given as the "real" thing. I don't think there's any real right or wrong way to go about it, but these sorts of subtle distinctions really do help us tease apart what the style engine is and isn't! (Or how much scope we're comfortable taking on at any one time 😄)
Let's put this one on ice and turn our attention to building out the store. I'm still convinced it would be helpful to offer a way to generate a stylesheet from an entire theme.json, perhaps not for immediate needs, but further down the line where theme authors might want to bypass WP_Theme_JSON or even for theme.json builder GUI utilities and the like. Thanks for the discussion on this one @andrewserong |
What?
Adding a new method to generate global styles: wp_style_engine_generate_global_styles
Because there are loads of idiosyncrasies in global styles, we're starting with the root layer and will then turn to
style.elements
andstyle.blocks
This PR takes care of:
styles
in theme.jsonblockGap
to--wp--style--block-gap
ref
value is found (See Global Styles: Allow references to values in other locations in the tree #41696)Why?
Instead of plugging in the block style generator into WP_Theme_JSON like in #40955, let's see if we can pass the entire merged global styles object to the style engine to generate a stylesheet.
How?
The global styles object will generate a stylesheet:
Up next
To keep things smaller and more testable, we're focussing on root-level global styles. Elements and blocks styles will be worked on in stages.
For example:
Testing Instructions
Create a new post with some layout blocks, or any blocks for that matter.
Style your site using theme.json and/or the site editor global styles tools.
Some theme.json inspiration
The styles should appear in the
body
tag ofglobal-styles-inline-css
in the HTML. E.g.,Unit tests
All tests
Style engine tests