Add a WP_Style_Engine_CSS_Declarations object
#42043
Conversation
aristath
force-pushed
the
add/style-engine/rules-object
branch
from
322550f to
b684a9f
Compare
There was a problem hiding this comment.
Thanks a lot for demonstrating this idea @aristath
I'm feeling pretty positive about having a "rule" object.
The only hesitation I'd have is trying to knock over everything at once.
Working out a simple rule object interface would be a good step to do first and get right since we don't need to store styles just yet.
As we slowly make our way through the application, a separate store we can plugin later might be more flexible.
As always, take what I say with a salt sandwich. 🤣
There was a problem hiding this comment.
I really like the ideas in this PR, particularly how it cleans up the readability of the generate function in the main style engine class.
Do you have an idea in mind @aristath as to how many separate classes we might want to have for organising how we store and then output styles? (I know @ramonjd described five or so broad areas in the discussion, and I was wondering how some of that could be logically grouped 🤔)
Between this PR and #41896, it sounds like so far, we've got something along the lines of:
- A main style engine class, that is the entry point for block supports and global styles to call it to generate / register styles
- A class for CSS key/value pairs (declarations) in this PR
- A class for storing all the rules / outputting the
styletag with de-duped rules (as in Add an inline styles handler #41896) — perhaps this one is the registry / store as well as the thing that does the outputting?
We can also keep discussing that sort of structural question over in #42028 if you think that's a better place for it 🙂
Thank you! I like the idea of having a separate object for style-rules-declarations as well, because it's small enough to be used everywhere we print styles! Since rules-declarations are not tied to a selector, it can be used to add inline styles like
That sounds about right... |
aristath
changed the title
WP_Style_Engine_CSS_Rules objectWP_Style_Engine_CSS_Declarations object
ramonjd
added
[Package] Style Engine
Thanks again for helping out with the thinking on this @aristath 🍺 Great to have more folks working out the tricky issues! |
|
@ramonjd I still see the |
Interesting! 🤔 Because the selector is the same between tests, it looks as though the store is using the value from the previous test So either we'd need to clear the store in between tests, or overwrite store values with incoming values where the selector matches an existing record. It's a good example of why we'll need some tests when we eventually implement the store. Until we start looking at consolidating style tag output1, we don't need the store at all. This makes the tests pass: // Build CSS rules output.
$css_selector = isset( $options['selector'] ) ? $options['selector'] : null;
// If we're instantiating this object for every style, why not pass $css_declarations to the constructor?
// After all, the declarations array is a dependency. 🤷
$style_rules = new WP_Style_Engine_CSS_Declarations();
$style_rules->add_declarations( $css_declarations );It might even work out better for now since we wouldn't have to define a public function Footnotes
|
Yes, once that PR lands, I think we'll be able to have a bit of fun digging into the Layout support and seeing how we can improve the output there. I think that (and the rest of the global styles class) makes another good case for the architecture of the class in this PR 👍 I ran out of time to give it a closer look again today, but I'll be back from next Tuesday and happy to do more reviewing then. Thanks again for all the work here @aristath, like Ramon mentioned, it's so nice to have more folks to collaborate with on this part of the puzzle! |
Well... We have 3 options:
If we pass
Wow... excellent debug skills!! It makes perfect sense now, thank you!! 🙇 2 options here: Either remove the primitive store, or change the selectors in the tests, so they're different 🤔 |
That's a very good point! Thank you for explaining your thinking here. It's just a personal preference, but I'm drawn to your first option "Pass $css_declarations to the constructor, and keep the add_declarations() method." That way we have only what we need now (the constructor), and the flexibility later in
That's true. The class's functionality is deliberately narrow though – even the name We'd have the style engine class as the facade to deals with subsystem parts, namely, instantiating the objects and ensuring that they work together. I am very biased in that I'm always trying to shave off as much as possible things we don't need, but I appreciate keeping options open as well 😄 I don't think we can tell what the future holds. If I could I'd be writing to you from a super yacht. 🤣 |
Consider me convinced. I don't see any real downside to adding a constructor with an optional Pushed the change 👍 |
aristath
force-pushed
the
add/style-engine/rules-object
branch
from
c7d74df to
26d71a5
Compare
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
|
This PR simplifies the main The I believe this is now ready for a final review... Structurally and future-wise it makes sense, all comments have been addressed, tests pass, and we can use it in a number of other Style-Engine-related PRs 👍 |
What?
Adds a new
WP_Style_Engine_CSS_Declarationsobject in the Style Engine.Part of the explorations on the Style Engine structure in #42028
The
WP_Style_Engine()->get_css_rules_object( $selector );method gets an instance of the "rules" object. This helps gather all rules for a specific selector to the same object, and these can later be used when printing.The object takes care of parsing CSS strings to an array (prop=>value pairs), sanitizes the styles and handles compiling them to a string that can then be used in other parts of the style engine.