Selectors API: Fix for global styles hook, style variations, and duotone #49393
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] Regression
|
Despite not yet having a chance to polish and thoroughly test the code here, I've optimistically created this draft PR in that it may offer an alternative to reverting #46496 and get us to a stable Selectors API sooner. |
This was referenced Mar 28, 2023
|
Size Change: +315 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 372c6f8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4549009557
|
|
I've given this a bit of a test and it's working about as expected. So given this would be good to get in before the next Gutenberg release, I'll mark the PR ready for review and iterate on it as required. |
aaronrobertshaw
requested review from
spacedmonkey,
ellatrix,
ajitbohra,
ajlende,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
oandregal
reviewed
Mar 28, 2023
oandregal
reviewed
Mar 28, 2023
|
I wasn't able to test how/whether this fixes duotone and style variations. This is good in regard to the removal of editor selectors, and everything else I've reviewed. I've left some minor comments here, and it seems we can also remove other functions (can be a follow-up like this other). I have an errand to run this afternoon, so I won't be available to look at this until tomorrow. Please, go ahead with this, so it's part of the 15.5 RC (provided others confirm duotone/style variations are fixed). We can always prepare smaller follow-ups within the RC cycle, if necessary. |
gziolo
added
the
[Feature] Block API
ajlende
reviewed
Mar 28, 2023
packages/block-editor/src/components/global-styles/get-block-css-selector.js
Show resolved
Hide resolved
Comment on lines
+26
to
+31
| // Duotone ( no fallback selectors for Duotone ). | ||
| if ( path === 'filter.duotone' ) { | ||
| // If selectors API in use, only use its value or null. | ||
| if ( hasSelectors ) { | ||
| return get( selectors, path, null ); | ||
| } |
There was a problem hiding this comment.
I didn't comment on this in the previous PR because I felt like it could be included in the duotone follow-up, but I do think it would be better for duotone to use fallback selectors to match everything else.
I want to avoid as much duotone-specific logic as possible—that was the point of making supports.__experimentalDuotone experimental—eventually we could remove the duotone specifics.
There was a problem hiding this comment.
I want to avoid as much duotone-specific logic as possible—that was the point of making supports.__experimentalDuotone experimental—eventually we could remove the duotone specifics.
A quick search of the WP plugin directory showed some hits for __experimentalDuotone out in the wild so I'm not sure it's a simple removal, we'd need to follow an official deprecation process right? In the same way, we can't just remove the __experimentalSelector support.
We'll need some form of specific duotone code here as its old location under color.__experimentalDuotone doesn't follow the general feature.__experimentalSelector pattern.
I do think it would be better for duotone to use fallback selectors to match everything else
So to confirm, it's ok for a duotone selector, if requested, to fallback to filter.root or the block's root selector?
Combining this with the need to update duotone block support to not scope its selectors, it might be better in a separate follow-up to this PR.
What do you think about landing this now (assuming style variation updates get greenlit) to fix the glaring issues before 15.5RC and create a separate PR to address this immediately after?
There was a problem hiding this comment.
I haven't reviewed the whole PR in depth, but the variations-related changes look OK and the feature is working as expected! Compared to trunk, the most obvious changes can be seen in the Image block variation:
- In trunk, adding a border to the "Rounded" variation adds it to the block wrapper instead of the image element;
- In trunk the updated variation style isn't visible in the post editor.
Both the above issues are fixed in this branch.
talldan
approved these changes
Mar 29, 2023
|
Thank you everyone for all the eyes on this 🙇 In the interest of landing the major fixes here before the 15.5 RC is cut, I'll merge this one as is leaving any smaller fixes and tweaks for follow-ups. Known Issues for Follow-up
|
|
I've tested the current state of duotone and this is what I found:
While both images have proper duotone, the global-level duotone gets applied to any image (whether or not it's a block image). Note how the avatar at the top-right gets the global-level duotone.
How everything works:
/* CORE-BLOCK-SUPPORTS-INLINE-CSS STYLESHEET */
/* purple-green => duotone applied at block-level */
.wp-duotone-purple-green img,
.wp-duotone-purple-green .components-placeholder {
filter: var(--wp--preset--duotone--purple-green) !important;
}
/* CORE-BLOCK-SUPPORTS-INLINE-CSS STYLESHEET */
/* midnight => duotone applied at global-level */
.wp-duotone-midnight img,
.wp-duotone-midnight .components-placeholder {
filter: var(--wp--preset--duotone--midnight) !important;
}
/* WP-BLOCK-IMAGE-INLINE-CSS OR GLOBAL-STYLES-INLINE-CSS STYLESHEET */
/* duotone applied to global-level */
img, .components-placeholder {
filter: var(--wp--preset--duotone--midnight);
}Questions:
If we update the image selector to be whole, the block-level duotone breaks. The reason is that the block-level duotone scopes the given block selector |
3 tasks
Working on this at #49436 |
12 tasks
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 21, 2023
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 22, 2023
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 25, 2023
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 26, 2023
tellthemachines
pushed a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 27, 2023
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.
Related:
block.jsonAPI: consider graduating__experimentalSelectorand__experimentalDuotonefrom experimental to stable #45194Made obsolete by this PR:
What?
#46496 landed in a broken state. It missed addressing the following:
useGlobalStylesOutputThis PR aims to fix the broken functionality and add the missing pieces that should have been in #46496.
In addition, some concerns were raised around the introduction of the editor-only selectors and whether the need warrants having to support that part of the API moving forward. In an effort to unblock the general Selectors API, the editor selectors functionality has been removed in this PR. It can be added in a follow-up and discussed separately.
Why?
The Selectors API introduced a few regressions, that need fixing. This PR is one pathway to that. The alternative to this PR is to revert #46496 completely.
How?
filterskey tofilteruseGlobalStylesOutputto use the Selectors APIThis PR might be easiest to review commit-by-commit as they should match each item from the list above.
Next Steps
Testing Instructions
npm run test:unit packages/block-editor/src/components/global-styles/test/use-global-styles-output.jsnpm run test:unit packages/blocks/src/api/test/registrationnpm run test:unit:php -- --filter WP_Get_Block_CSS_Selector_Testnpm run test:unit:php -- --filter WP_Theme_JSON_Gutenberg_Testnpm run test:unit:php -- --filter WP_Duotone_Gutenberg_TestNote: There's an existing issue with Global Styles duotone filters not applying in the block editor (fix underway).