-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add style engine support for nested at-rules. #58867
Conversation
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-processor-test.php |
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 looks great to me, thanks for splitting this out from #58539 and for adding the tests!
✅ The inclusion of the set_at_rule
, get_at_rule
behaviour and storing based on the at rule concatenated with the selector sounds like a good way to solve the current use case, while not impeding any future progress on more complex at rules in the future (like @keyframes
potentially)
✅ Tests look good and cover both prettified and non-prettified states nicely 👍
✅ Existing style engine output is unaffected (double-checked layout output, etc on the site frontend)
LGTM! ✨
$css_declarations = $this->declarations->get_declarations_string( $should_prettify, $declarations_indent ); | ||
$nested_css_declarations = $this->declarations->get_declarations_string( $should_prettify, $nested_declarations_indent ); |
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.
Tiny nit, and doesn't affect the API changes here, so not a blocker: Why is get_declarations_string
called twice? Is there ever a time we'd need to have access to both $css_declarations
and $nested_css_declarations
?
Just wondering if something like the following could avoid the extra call:
$at_rule = $this->get_at_rule();
$has_at_rule = ! empty( $at_rule );
$nested_css_declarations = $this->declarations->get_declarations_string(
$should_prettify,
$has_at_rule ? $nested_declarations_indent : $declarations_indent
);
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.
Oooh yeah that's a good idea! There's no need to access both at the same time.
Flaky tests detected in 9aa251a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7839043609
|
What's the status of the backport here? Is the style engine something that is backported automatically with package updates? (Can't remember) |
Style engine PHP changes are backported as per usual. If it's okay, I'd like to hold off on this one as I have alternative PR that takes this idea a bit further: Edit: it's basically ready just needs 👀 |
@youknowriad these changes aren't targeted at WP 6.5 so we can leave it for now! I added the backport label so it doesn't get missed for next time. |
What?
Extracts the style engine-specific changes from #58539 for easier review.
Essentially allows adding to the
$css_rules
array parameter fromwp_style_engine_get_stylesheet_from_css_rules()
anat_rule
item, with a string containing a nested at-rule identifier plus rule, such as@media (max-width: 700px)
.It then outputs the nested rule.
This is a fairly minimal change intended to support dynamic generation of at-rules where needed as part of core generated styles. It doesn't include support for nesting multiple rules under the same at-rule.
The discussion on the other PR provides relevant context for this approach.
Testing Instructions
Run the tests!
You can also try passing
wp_style_engine_get_stylesheet_from_css_rules()
an array that includes an at-rule, such asTesting Instructions for Keyboard
Screenshots or screencast