-
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: trim multiple selector strings #45873
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
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 picking this one up, and for the eagle eyes on the formatting of the output!
I've left a comment — I'm mostly wondering if it's worth only enabling this for prettified output, to attempt to (albeit very slightly) avoid extra processing when not prettifying. What do you think?
// Trims any multiple selectors strings. | ||
$selector_trimmed = implode( ',', array_map( 'trim', explode( ',', $this->get_selector() ) ) ); | ||
$selector = $should_prettify ? str_replace( array( ',' ), ",\n", $selector_trimmed ) : $selector_trimmed; | ||
$css_declarations = $this->declarations->get_declarations_string( $should_prettify, $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.
Since this adds an extra step of processing, what do you think about only enabling this when $should_prettify
is true
?
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 looking at this.
That's a fair point, and I totally agree.
Just to note that if we skip it, it'll mean that non-prettified selectors will have the space, e.g., the difference between:
.test, .me{...}
and
.test,.me{...}
This is, as they say, "no big deal". 😄
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.
DONE!
…d by a comma, passed to the style engine. This is so we can correctly align prettified content, and remove spaces between multiple selectors with the same styles.
77c4092
to
7e6f26c
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.
What?
This PR trims multiple selector string, that is a string of selectors separated by a comma, passed to the style engine.
Basically removing spaces between selectors in a given string. A "selector" is defined by a comma boundary:
','
.For example, let's say we pass a selector of
.poirot, .poirot:active, #miss-marple > .st-mary-mead
for a given set of CSS declarations.Before
.poirot, .poirot:active, #miss-marple > .st-mary-mead{margin-left:0;font-family:Detective Sans;}
After
.poirot,.poirot:active,#miss-marple > .st-mary-mead{margin-left:0;font-family:Detective Sans;}'
And, prettified, this:
Becomes this:
Why?
This is so we can correctly align prettified content, and remove spaces between multiple selectors with the same styles.
How?
Exploding, mapping, trimming, imploding.
Testing Instructions
Run the tests!
npm run test:unit:php -- --filter WP_Style_Engine_CSS_Rule_Test
If you're feeling really keen, try adding multiple selectors to a block supports style. Layouts is a good place to test.
Here's a diff:
For any page with a default layout (e.g., add a simple group block to a page), the styles should be printed out neatly: