Global styles: resolve block style variation URIs #66731
Draft
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
…es_paths() to return the paths for all styles nodes in a theme.json tree including elements and variations. Use this helper method to resolve relative URIs in theme json resolver.
ramonjd
added
[Status] In Progress
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
ramonjd
commented
Nov 5, 2024
| * | ||
| * @return array An array of paths to style nodes in theme.json. | ||
| */ | ||
| public function get_styles_nodes_paths() { |
There was a problem hiding this comment.
Nit: The naming here with three successive plurals seems a bit clunky. What about get_style_node_paths?
This avoids each word within the name "modifying" those before it making it a little less awkward and easier to parse.
| @@ -2610,11 +2611,19 @@ protected static function get_style_nodes( $theme_json, $selectors = array(), $o | |||
| return $nodes; | |||
| } | |||
|
|
|||
| $include_node_paths_only = $options['include_node_paths_only'] ?? false; | |||
| $nodes[] = array( | ||
| 'path' => array( 'styles', 'blocks', $name, 'variations', $variation ), | ||
| ); | ||
| if ( isset( $node['blocks'] ) ) { |
There was a problem hiding this comment.
Check with @aaronrobertshaw about whether this is all correct 😄
There was a problem hiding this comment.
It appears to match the original code in the else statement. I'm missing context here though around the inclusion of include_node_paths_only. It looks like the variations part of the equation was mistakenly overlooked during that enhancement.
One thing I haven't grokked yet is where the paths get handled for element styles within block style variations. Were they covered?
There was a problem hiding this comment.
One thing I haven't grokked yet is where the paths get handled for element styles within block style variations. Were they covered?
No not covered. They'll need to be added. I deliberately skipped over them, because I wasn't sure whether it was appropriate to overload get_block_nodes() in that way when they didn't previously deal with nested block styles or elements in block style variations.
TBD I guess.
It looks like the variations part of the equation was mistakenly overlooked during that enhancement.
That's what I thought - I moved it up to make this PR work, but I suppose it should be a separate PR.
Fortunately it's only used in one place for now WP_Theme_JSON:merge and only in the context of merging background images. So I don't think there's any urgency to it.
ramonjd
commented
Nov 5, 2024
| * some values provide exceptions, namely style values that are | ||
| * objects and represent unique definitions for the style. | ||
| */ | ||
| $style_nodes = $theme_json->get_styles_nodes_paths(); |
There was a problem hiding this comment.
This change can be a separate PR.
There was a problem hiding this comment.
Anything that generally makes theme.json class related PRs easier is fine by me 😅
In this case though the PR isn't huge, so I don't mind reviewing as is.
|
Size Change: +53 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 151348d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11677235227
|
There was a problem hiding this comment.
Thanks for addressing this @ramonjd 👍
I've only had a chance to read through the code so far but have left a few small comments below in case they help keep the momentum going while I get to testing.
| * | ||
| * @return array An array of paths to style nodes in theme.json. | ||
| */ | ||
| public function get_styles_nodes_paths() { |
There was a problem hiding this comment.
Nit: The naming here with three successive plurals seems a bit clunky. What about get_style_node_paths?
This avoids each word within the name "modifying" those before it making it a little less awkward and easier to parse.
| $nodes[] = array( | ||
| 'path' => array( 'styles', 'blocks', $name, 'variations', $variation ), | ||
| ); | ||
| if ( isset( $node['blocks'] ) ) { |
There was a problem hiding this comment.
It appears to match the original code in the else statement. I'm missing context here though around the inclusion of include_node_paths_only. It looks like the variations part of the equation was mistakenly overlooked during that enhancement.
One thing I haven't grokked yet is where the paths get handled for element styles within block style variations. Were they covered?
| array( | ||
| 'name' => 'group-background-twinky', | ||
| 'label' => 'Group block twinky background', | ||
| 'style_data' => array( |
There was a problem hiding this comment.
We probably need to expand this test block style a little so it covers element styles at both the variation root and inner block levels.
Co-authored-by: Aaron Robertshaw <[email protected]>
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.
This PR:
WP_Theme_JSON->get_styles_nodes_paths()to return the paths for all styles nodes in a theme.json tree including elements and variations.Why?
This increases support for relative path URI resolution to block style variations and elements. The latter is a side-effect, however the former is necessary because theme developers are able to add relative background images to block style variations. These URIs however do not get resolved.
How?
By resolving all background urls, regardless of the their location in the styles tree.
Ensuring style variations are resolved in the clientside in
toStyles().Testing Instructions
Add a few images to your local theme somewhere relative to theme.json.
Now create a new a block style variation theme.json file under
/stylesthat includes a background image:{ "version": 3, "title": "Group pizza variation", "slug": "group-pizza-variation", "blockTypes": [ "core/group" ], "styles": { "background": { "backgroundImage": { "url": "file:./img/pizza1.jpg" } }, "blocks": { "core/group": { "background": { "backgroundImage": { "url": "file:./img/pizza2.jpg" } } } } } }For good measure add a background image to an element style too. This will be in your main theme.json
{ "version": 3, "settings": { "appearanceTools": true }, "styles": { "elements": { "h1": { "background": { "backgroundImage": { "url": "file:./img/pizza3.jpg" } } } } } }Ensure that style variations appear in the editor and the frontend. Pay attention to nested variation styles. They should be resolved and work as expected.