Block Styles: Fix block style variation selector generation #58051
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
aaronrobertshaw
added
[Type] Bug
|
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❔ lib/class-wp-theme-json-gutenberg.php ❔ phpunit/class-wp-theme-json-test.php |
There was a problem hiding this comment.
Thanks for the ping @aaronrobertshaw! Just to make sure I'm following — the problem with the current approach on trunk is that anything that uses an element selector will be broken because it currently prepends the variation classname instead of appending, so we wind up getting .is-style-customp instead of p.is-style-custom.
Attempting to ensure that we're always adding the classname after the wrapping selector sounds like a good idea to me 👍. It's a bit tricky to predict, but I like the way you're honing in on what would be the valid characters before the bit we wish to inject.
Just left a few questions — my regex is a little rusty, so I copy / pasted it into https://regexr.com/ to play with some sample values, and I wasn't sure how much of the is|not|has|where we needed?
Also, it looks like we'll also need to update the JS logic for consistency. While manually testing, I noticed that the paragraph styles are output correctly for me in the post editor (which uses this PHP logic) but not in the site editor.
Post editor
The selector is correct:
Site editor
This block is set to my is-style-custom style, but doesn't get the color:
Here's the output in the site editor:
To reproduce:
- I added the following at the end of the paragraph block's
block.json:
"styles": [
{ "name": "fill", "label": "Fill", "isDefault": true },
{ "name": "outline", "label": "Outline" },
{ "name": "custom", "label": "Custom" }
]
- Opened up the site editor, went to the Paragraph block in global styles, edited the Custom block style and gave it a color.
- Added a paragraph block and selected the Custom style.
|
Flaky tests detected in 327d1ee. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7690849477
|
|
I've quickly updated the JS side of block style variation selectors but am out of time today. I'll give this a proper test tomorrow before flagging it ready for a final review. |
|
Size Change: +64 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
fix/block-style-variation-selectors
branch
from
026207d
to
3c7a5c1
Compare
|
I think this is ready for a final review. I fixed the expected selector in the JS unit tests I missed. The rest of the earlier unit test failures seemed unrelated but I've rebased this in case. FWIW the tests are passing locally. I also gave this PR another manual test in the browser and it behaves as expected. |
|
Thanks for the update, Aaron! I likely won't get time to take this for another spin today, but if no-one beats me to it, I'll give this another review on Monday 🙂 |
andrewserong
approved these changes
Jan 25, 2024
There was a problem hiding this comment.
Just managed to get this review in before wrapping up for the week. This is testing great for me now!
✅ Adding additional styles to the paragraph block as in my earlier review is now working nicely
✅ Paragraph and Button outline style updates are working well in the site editor, post editor, and on the site front end
✅ Regex pattern looks good to me, thanks for adding in the good test coverage for both the JS and PHP approaches!
While it's good to avoid complex regex patterns where we can, in this instance I think parsing the CSS selector with these regex patterns is nicely justified:
- The custom selector is only set by blocks, so is unlikely to be super complex selectors and is relatively controlled
- Due to the above, we're not parsing user entered data
- We're not attempting to parse all of the CSS spec or anything like that
So, all up I think this is a good approach to go with — it resolves the current bug while making the feature more robust and adding some good test coverage.
LGTM! ✨
aaronrobertshaw
force-pushed
the
fix/block-style-variation-selectors
branch
from
dfedef0
to
327d1ee
Compare
aaronrobertshaw
force-pushed
the
fix/block-style-variation-selectors
branch
from
327d1ee
to
f7f6b1a
Compare
This was referenced Feb 7, 2024
|
Backport is available in WordPress/wordpress-develop#6053 |
youknowriad
added
Backported to WP Core
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?
Updates the approach to generating selectors for block style variations so that they aren't broken if a block's selector happens to be an element tag such as the paragraph block.
Why?
With the current approach just pretending
is-style-whateverto the block's selector we end up with a broken selector for the paragraph block. As block style variations are being extended to support section styling it is highly likely theme authors and users might want to have some different styles within paragraphs on a per section basis.How?
Testing Instructions
npm run test:unit:php:base -- --filter WP_Theme_JSON_Gutenberg_Testis all green