Style engine: add support for nested CSS rules to wp_style_engine_get_stylesheet_from_css_rule()
#58918
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ phpunit/style-engine/class-wp-style-engine-css-rules-group-test.php ❔ lib/load.php ❔ phpunit/style-engine/class-wp-style-engine-css-rule-test.php ❔ phpunit/style-engine/class-wp-style-engine-css-rules-store-test.php ❔ phpunit/style-engine/class-wp-style-engine-processor-test.php ❔ phpunit/style-engine/style-engine-test.php |
ramonjd
commented
Feb 12, 2024
| @@ -33,26 +33,25 @@ class WP_Style_Engine_CSS_Rule { | |||
| protected $declarations; | |||
|
|
|||
| /** | |||
| * The CSS nested @rule, such as `@media (min-width: 80rem)` or `@layer module`. | |||
| * A parent CSS selector in the case of nested CSS, or a CSS nested @rule, such as `@media (min-width: 80rem)` or `@layer module`.. | |||
There was a problem hiding this comment.
Most of the changes in this PR are renaming $at_rule to $rules_group
| $nested_declarations_indent = $should_prettify ? $indent_count + 2 : 0; | ||
| $suffix = $should_prettify ? "\n" : ''; | ||
| $spacer = $should_prettify ? ' ' : ''; | ||
| $rule_indent = $should_prettify ? str_repeat( "\t", $indent_count ) : ''; |
There was a problem hiding this comment.
Just reverting to what is was before #58867
| if ( isset( $this->css_rules[ "$at_rule $selector" ] ) ) { | ||
| $this->css_rules[ "$at_rule $selector" ]->add_declarations( $rule->get_declarations() ); | ||
| continue; | ||
| if ( $rule instanceof WP_Style_Engine_CSS_Rules_Group ) { |
There was a problem hiding this comment.
This is the major change of this PR - processing rules groups and rules before CSS compilation mainly to:
- allow merging of nested rules, and therefore grouping nested rules under the same, single rules group selector/at-rule,
- allow for separate processing of groups, e.g.,
get_cssand also printing them at the bottom
6 tasks
ramonjd
added
[Type] Enhancement
|
Size Change: +15 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Add missing tests Update CHANGELOG.md
|
Flaky tests detected in 7a07ab4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7867203769
|
ramonjd
commented
Feb 12, 2024
| if ( isset( $this->css_rules[ $selector ] ) ) { | ||
| $this->css_rules[ $selector ]->add_declarations( $rule->get_declarations() ); | ||
| continue; | ||
| if ( $rule instanceof WP_Style_Engine_CSS_Rule ) { |
There was a problem hiding this comment.
If this change is too heavy, we can revert and just introduce the WP_Style_Engine_CSS_Rules_Group class to allow for separate processing.
There was a problem hiding this comment.
What would the alternative look like? Just trying to wrap my head around the proposal and what the trade-offs might be.
There was a problem hiding this comment.
Reverting would just lose the ability to group nested rules.
Not a huge deal.
Co-authored-by: Andrew Serong <[email protected]>
There was a problem hiding this comment.
Nice follow-up @ramonjd, I quite like the idea of the extra class, it feels like it follows the OOP conventions already used for the existing style engine classes 👍
It is a heavier change than just including the group / at_rule in the rules class, but I like how this opens up the possibility for more complex nested output, which could be really useful for things like a plugin that might want to allow users to build custom animations and then output the CSS via keyframes. With that in mind, in terms of the test cases, I was wondering if it'd be worth also adding a complete @keyframes example — that could be another good example of the benefits of grouping and nesting, since for the keyframe to work, it all needs to be combined in the one grouping @keyframes declaration?
Overall, though, I do like the idea if we're adding in at_rule support of it feeling somewhat complete in terms of feature set, since this is a public API, and this seems to get us very close to that goal, it seems!
Curious to hear what @tellthemachines and @aristath think, too 🙂
| if ( isset( $this->css_rules[ $selector ] ) ) { | ||
| $this->css_rules[ $selector ]->add_declarations( $rule->get_declarations() ); | ||
| continue; | ||
| if ( $rule instanceof WP_Style_Engine_CSS_Rule ) { |
There was a problem hiding this comment.
What would the alternative look like? Just trying to wrap my head around the proposal and what the trade-offs might be.
|
Thanks for reviewing this and also indulging my proposal.
This was one of the motivations. But it is a heavier change, true. I'm totally open to stripping it down further and/or just keeping the variable name change for now and doing the rest in follow ups. I
Agree that it would be good! I tried it in one of the previous tests but the declarations were stripped via WordPress sanitize functions 😱 |
Oh, interesting! Were you using
Good point. Splitting out the variable name change into a separate PR could help keep the changes more isolated, too, if you're up for it! |
Yes, 🤦🏻
I'll do that. I know @tellthemachines had some good points about taking it slowly, and grouping is a nice to have. I think the main intellectual hurdle for me was ensuring we could do all these amazing things later. 😄 |
|
Converted this to draft Minimal update here: |
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?
Builds on:
by:
$at_ruleto$rules_group.A related exploratory PR for supporting nested rules is here:
Why?
@at-rulesdon't affect specificity.How?
Main changes are the the
WP_Style_Engine_Processor, which merges incoming/stored rules and outputs compiled CSS with rules groups appearing after any regular CSS rules.Potential follow ups in the future
wp_style_engine_get_styles()"$rules_group . $selector"keys are stored in the store, which means they can be overwritten. Later we can allow to add and merge nested rules to the store..ham { color: red; &:hover { color: blue; } }Testing Instructions
Run the tests
npm run test:unit:php:base -- --group style-engineFire up this branch using a block theme, e.g., 2024. Check that the
core-block-supports-inline-csson the frontend is exactly the same as trunk.You can manually check how rules are generated and printed.
Here are some rules declared by gutenberg_style_engine_get_stylesheet_from_css_rules()
The CSS should be printed at the end of
core-block-supports-inline-cssrules stylesheet: