Experiment: Style Engine Rules & Store objects #42222
Conversation
|
Size Change: +500 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
Thank you for working on this! I ran out of time to test it out today, but will get to it ASAP 🙇 Just wanted to say thanks 🙏 |
I love the concept! I know support might be a looooong way off, but do you think we'd also be able to leverage this system for CSS nesting? It'd be awesome one to offer it as an opt-in. |
There was a problem hiding this comment.
+1 this is a very cool exploration @aristath! I like the hierarchical structure, it feels like this will enable a lot of flexibility when it comes to optimising output 👍
A couple of questions:
- WIth the nested structure, will we ever run into an issue where we need a flat list of all selectors for de-duping, etc, and needing to flatten the data structure first might make it harder? I don't think so personally, but just curious if there are any downsides to storing a more nested version of rules within rules when it comes to optimisation.
- What are the potential trade-offs between the static
$rulesproperty of the store, and having it an object instance property? Apologies if my OOP naming is slightly off here (😅), to be more verbose: it looks like in this PR we assume a single store exists for the lifecycle of the current request. Will there be a case where we need to have two separate stores, even as an interim step? I imagine the goal is that ideally for a single request we only ever have a single store, but I was curious if it makes it easier or harder if we have a single store when it comes to using the style engine in global styles and block supports at the same time. For example, if incrementally we use the style engine to improve the styles that get output to the global stylesheet, and separately for the styles that are output tostyletags for individual blocks at render time. Eventually it'd be great for these to be in the one sheet, but in incremental steps, it might be easier if they're separate. Just a thought, though, as I know both you and @ramonjd have been doing more exploration into the global styles implications recently!
| $parts = array_map( 'trim', $parts ); | ||
|
|
||
| // Remove empty parts. | ||
| $parts = array_filter( $parts ); |
There was a problem hiding this comment.
Nice that you're handling accidental double-spaces in the selector 👍
| $parts = array_filter( $parts ); | ||
|
|
||
| // Split parts on ':' characters. | ||
| $parts = static::deconstruct_selector_array_on_character( $parts, ':' ); |
There was a problem hiding this comment.
Do we need to factor in double-colons, too? I doubt we'll be doing too much with pseudo elements to begin with, but just in case we need to implement it at some point (e.g. ::first-letter)?
A single store with no frills, that is, one that just stores With so many bespoke features being added to I definitely like the thrust of this PR and we'll probably need to structure the store/and or its contents to make certain pre-processing tasks easier, especially when we want to experiment with de-duping and whatever. Also theme.json probably will, and probably should, be doing a lot of the work for us in terms of cascade and inheritance, and even nesting. So we should try to leverage that if it all possible when building the rules. For example, the pseudo elements model introduced in #41786 {
"blocks": {
"core/group": {
"elements": {
"link": {
"color": {
"text": "green"
},
":hover": {
"color": {
"text": "red"
}
}
}
}
}
}
}gives us all the information we need to build the following selectors: .wp-block-group a {
color: green;
&:hover {
color: red;
}
} |
|
Thanks for adding the extra thoughts, Ramon!
Oh, I hear that! 🤣
Good point, and perhaps the restriction of only having a single store will help avoid accidentally adding additional complexity that could then be hard to wind back if we had multiple stores floating around? After all, with the goal of having a single de-duped output in a single style tag or stylesheet, having a single store makes sense to me 👍 |
Definitely! If we have the structure in place with hierarchical objects for selectors, then all we'd have to do is to change the method that generates the styles.
Yes, having a flat list of all selectors would be needed. That's why the [
'.wp-block-button' => Obj,
'.wp-block-button a' => Obj,
'.wp-block-button a:hover' => Obj,
]The
You're right, in this implementation there's a single store.
Definitely, +1! There are a lot of things that can and should be improved, but before I dedicate more time to this I wanted to hear what the 2 of you think and if this is direction we want to take with the style engine. |
Personally, I think recording rule hierarchy is something that'll make processing decisions easier down the track. I love these experiments - they're really forward thinking. Not necessarily now, but it'd be interesting to test some processing scenarios, and think about how deeply nested we'd allow things to be to control recursion and performance. We might come up with a different approach as a result 🤷 Some unit tests would be illustrative in order to get a better understanding of how things might be integrated into the flow. As far as the store model and processing, I'd say time is on our side. For 6.1 it'd be nice to have a super, super simple store in place, one that can be extended with the type of functionality that exists in this PR.
YES! 🕺 I was chatting with @andrewserong and it sounds like the store is the next major component we'll need to unblock the next round of shorter term goals, namely, reducing the amount of style tags and building a global styles stylesheet. Seeing as processing is a bit further down the track, and we won't need to store a parent->children relationship, do you think it's feasible to reduce the scope of our store here? I mean to say, keep a rule object and a rule store, but work on the more complex selector parsing and hierarchical storage in another, later PR? That way we'd have time to test things with real-world styles, come up with way to interact with the store model, react to new feature grenades, and basically not commit to a feature before there's a need for it. Given that the style engine has complete control of parsing/processing/storage, and it all happens at runtime, I think it'd be safe to iterate, i.e., we're not going to cause backwards compatibility issues. For preset purposes, I'm not 100% sure yet, but I'm thinking about a class that we could instantiate for each store we'd need, e.g., one for block supports and one for global styles with key/value pairs (selector => rules) and nothing else. Maybe down the road, in the land of unicorns 🦄 , we'd end up with one store for everything!
Yeah, similar to what we now have with An example might be a prettify option where we'd pass a
As mentioned, I reckon it's a great idea, and one we'll likely need. I'm a bit of a slow poke though, and would love to see a base store, something as simple as we can make it, and then iterate towards the hierarchical/nested model with test cases. It'd give us time (of which we have plenty) to discuss and performance test and so on. It's just a personal preference, but I think my attitude comes from having had things change under my feet so often, I'd hate for us to commit to one thing and then have to rearchitect a bunch of stuff, thereby losing all the hard work we've done. 🍺
[
'.wp-block-button' => Obj,
'.wp-block-button a' => Obj,
'.wp-block-button a:hover' => Obj,
]Just curious, would we still store I have the case in mind where a block in theme.json declares styles for Or is it just for possible nesting scenario? |
ramonjd
added
[Package] Style Engine
|
Thanks for all the extra context @aristath, this is just a short comment to say that I think the ideas here are worth exploring further (and thanks for explaining the flat list of selector keys in the store 👍). As Ramon mentioned, I think the Store will come in very handy as we iteratively look at introducing the style engine to global styles, and the structure here is looking really appealing to me so far! I sometimes find these issues difficult to tease apart in my head in the abstract, but when looking at concrete tasks, the features seem to reveal themselves a bit better (like the use of the Declarations class in #42366 🎉). So from my perspective I think it's very much worth digging in a little further here to see how it might work! |
|
Thank you @ramonjd and @andrewserong for the comments!
A usage example would look like this now with these changes: // Get a named store.
$store = WP_Style_Engine_CSS_Rules_Store_Gutenberg::get_store( 'global-styles' );
// Get the rule object for a selector.
$rule = $store->get_rule( '.wp-block-button a:hover' );
// Set the declarations for the rule.
$rule->set_declarations(
// Use the Declarations object. We can pass multiple declarations to the rule
// at any time on a per-store basis.
new WP_Style_Engine_CSS_Declarations_Gutenberg(
array(
'color' => 'var(--block-button-link-hover-color)',
'outline' => '1px solid var(--block-button-link-hover-color)',
'background-color' => 'black',
)
)
);
$css = $rule->get_css();
// string(96) ".wp-block-button a:hover {color: var(--block-button-link-hover-color); background-color: black;}"The implementation is now a lot simpler since we got rid of the parent-child relations, and the objects are pretty minimal 🎉 |
I love it, I think the simplicity here makes it a really solid addition that we can build on. I also really like the clarity of the APIs you've created between the Declarations class and the Rule class — the method for If we can add a few tests, I think this PR looks like it'll be good to merge! |
There was a problem hiding this comment.
Thanks for working on this @aristath 🙇
I had to see it in action so I created some tests over at #42408 to test a few scenarios that we'll need.
Feel free to 100% ignore them if you had a different idea of implementation. It was just me trying to understand how this particular store as it stands would serve us.
The main requirement of a store right now is being able to register styles incrementally until a hook is fired, whereupon we'd want to fetch the store's entire contents in order to build a stylesheet.
Also I'm wondering about where CSS property merging would take place when adding CSS declarations, e.g., when we have 2 or more WP_Style_Engine_CSS_Declarations objects in a WP_Style_Engine_CSS_Rule with the same properties?
Should we merge before we store them? 🤔 If so, it might be easier for all classes to accept property => value declaration pairs, and return instances of WP_Style_Engine_CSS_Rule | WP_Style_Engine_CSS_Declarations in the get functions where required?
I'm not sure. Just asking.
It might be something that'll come out in the wash later too, so not a blocker. We can continue to iterate.
I'm happy to get this in as soon as we have tests and then carry on refining.
| * | ||
| * @param string $selector The CSS selector. | ||
| */ | ||
| public function get_rule( $selector ) { |
There was a problem hiding this comment.
Unless I'm missing something, we'll also need to add rules to stores from around Gutenberg.
When we ultimately want to enqueue a store's rules, then we'd want to retrieve them all at once.
There was a problem hiding this comment.
Ah apologies, I think the get_rule threw me and I expected a corresponding set_rule.
The references penny didn't drop. 😄
It did eventually when I tried to write tests: https://github.com/WordPress/gutenberg/pull/42222/files 😮💨
So just to confirm we'd be doing something like this:
// Deal with incoming css rules from around Gutenberg.
$store_rule = $new_pizza_store->get_rule( $selector );
$css_declarations = new WP_Style_Engine_CSS_Declarations( $new_raw_declarations );
$store_rule->set_declarations( array( $css_declarations ) );
// Somewhere else... eventually.
$new_pizza_store->get_all_rules();
// Build the stylesheet from the rules...There was a problem hiding this comment.
That's a good point about getters vs setters and potential confusion. Perhaps using a separate setter could make the API a bit more explicit?
One of the things for us to keep in mind as we explore the style engine API is what sort of behaviour (and naming) we expect in WordPress APIs in general. So, for example, if the WP_Block_Type_Registry, WP_Block_Styles_Registry, and WP_Block_Patterns_Registry classes have separate register and get_registered methods (and get_registered returns null if the item is not registered), then consumers of this class might expect comparable behaviour / methods.
There was a problem hiding this comment.
I think I found a use case for an explicit set_rule: https://github.com/WordPress/gutenberg/pull/42408/files#r920830003
Aside from the idea that it might be handy for people like me who don't like to think too hard 😆
There was a problem hiding this comment.
You're right, get_rule() is probably not the right name for this method.
What if we called it add_rule and also added a remove_rule method? 🤔
The method basically returns the rule if it already exists, or creates a new one if it doesn't.
There was a problem hiding this comment.
add_rule and remove_rule work for me!
|
|
||
| // Create the rule if it doesn't exist. | ||
| if ( empty( $this->rules[ $selector ] ) ) { | ||
| $this->rules[ $selector ] = new WP_Style_Engine_CSS_Rule( $selector ); |
There was a problem hiding this comment.
Why would we always want to create and store a new rule here? Should it be explicit through a set_rule method or something like that?
It might end up creating unnecessary records. I could be missing something though!
There was a problem hiding this comment.
Oh I see 🤦 This is the setter and getter.
There was a problem hiding this comment.
Yeah... it's a 2-in-1 method. Which is why we should probably rename it... 😅
packages/style-engine/class-wp-style-engine-css-rules-store.php
Outdated
Show resolved
Hide resolved
Ditto 🙇 |
|
Lots of amazing feedback here, thank you both!
With the above changes, we could now write the code from my previous comment like this: $css = WP_Style_Engine_CSS_Rules_Store_Gutenberg::get_store( 'global-styles' )
->get_rule( '.wp-block-button a:hover' )
->set_declarations(
array(
'color' => 'var(--block-button-link-hover-color)',
'outline' => '1px solid var(--block-button-link-hover-color)',
'background-color' => 'black',
)
)->get_css();
// string(96) ".wp-block-button a:hover {color: var(--block-button-link-hover-color); background-color: black;}"@ramonjd Thank you for writing the tests on #42408!! Writing tests is not my strong suit, I have a (slowly shrinking) gap of knowledge there 😓 |
aristath
force-pushed
the
add/style-engine/rules
branch
from
f69e90d
to
23c4dba
Compare
ramonjd
force-pushed
the
add/style-engine/rules
branch
from
a2b92ff
to
0c1dd59
Compare
|
@aristath I've closed the separate tests PR and added them to this one. Hope you don't mind. Saves us looking after two PRs! |
There was a problem hiding this comment.
Just doing a flyby ✅
This is once again awesome work. Things are starting to come together 🎉
Since we're not using these classes yet, I'd be fine with merging this (assuming tests pass) and doing the add_rule/set_rule/whatever follow up later, or adding them in this PR.
Up to you 👍
There was a problem hiding this comment.
LGTM too, and like @ramonjd mentions, we could either update get_rule to add_rule now, or in a follow-up PR, whichever way you prefer, since we're not yet using these classes anywhere 👍
Thanks for creating nicely scoped PRs, too, it's been a great way to contain the discussion to incremental pieces so we can pragmatically move the project forward, piece by piece 🎉
aristath
force-pushed
the
add/style-engine/rules
branch
from
0c1dd59
to
c3bfa2f
Compare
What?
This is a proof of concept, experimenting with possible implementations for the future of the Style Engine and how we may structure things.
This PR adds 2 objects:
WP_Style_Engine_CSS_RuleWP_Style_Engine_CSS_Rules_StoreReading the comments in other tickets, I think we'll need to create an object for rules, and allow them to be nested to accommodate child elements, pseudo-selectors etc.
The
WP_Style_Engine_CSS_RuleobjectThis object holds a selector, the parent (if one exists), and the declarations, along with selector-specific methods.
The
WP_Style_Engine_CSS_Rules_StoreobjectThis object holds a collection of
WP_Style_Engine_CSS_Ruleobjects by selector.Usage
The above will create 3
WP_Style_Engine_CSS_Ruleobjects, one for each of these selectors:.wp-block-button.wp-block-button a.wp-block-button a:hoverThe benefit of this structure is that this way, we can know which children - if any - a selector has, we can assign style-declarations on each of them, we can add & remove children and so on.
CSS becomes hierarchical and reflected in the objects structure. We can then process them as we see fit
Unit tests