-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Disable settings.shadow.defaultPresets
for classic themes
#6309
Disable settings.shadow.defaultPresets
for classic themes
#6309
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 Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
settings.shadow.defaultPresets
settings.shadow.defaultPresets
for classic themes
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.
It completely blocks the shadow tools from the UI in classic themes (same as 6.4).
With respect to the newly introduced feature in block editor, tested a case where shadow applied in a block theme (in the post content) and then switched to classic theme.
It preserves the shadow in the post because the template has inline styles for shadow and core-preset css variables are still created.
LGTM!
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.
Left a longer comment in the issue WordPress/gutenberg#59989 (comment)
array( | ||
'name' => 'Natural', | ||
'shadow' => '6px 6px 9px rgba(0, 0, 0, 0.2)', | ||
'slug' => 'natural', | ||
), |
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.
It might be helpful to assert specifically that the natural
present has not been overriden by the Theme version from block-theme
's theme.json
.
The original bug related to the theme presets with the same slug overriding the core presets so it might be good to specifically assert to focus the test on this aspect.
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.
Maybe also include a reference to the location of the related core preset?
wordpress-develop/src/wp-includes/theme.json
Lines 196 to 200 in f25ca58
{ | |
"name": "Natural", | |
"slug": "natural", | |
"shadow": "6px 6px 9px rgba(0, 0, 0, 0.2)" | |
}, |
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 value would never be overridden. What would happen instead is that theme
array would have the natural
preset. You may want to check by setting shadow.defaultPresets:false
in core's theme.json and then execute npm run test:php -- --filter test_theme_shadow_presets_do_not_override_default_shadow_presets
.
Note that I've also added a theme preset that is actually added (test
) to make sure this is also working.
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.
Essentially, presets work this way:
- Once the different
theme.json
sources (default, theme, user) are merged, all the values are available under the source key. For example:
array(
'default' => array( /* come from core's theme.json */
array( 'slug' => 'natural', /* ...*/ ),
array( 'slug' => 'deep', /* ...*/ ),
),
'theme' => array( /* come from the theme's theme.json */
array( 'slug' => 'natural', /* ...*/ ),
array( 'slug' => 'test', /* ...*/ ),
)
)
- However, in certain situations, we don't want the theme presets to "override" default presets, so we remove any whose slug is the same as one of the default presets. This is what happens for
shadows
, resulting in the following:
array(
'default' => array( /* come from core's theme.json */
array( 'slug' => 'natural', /* ...*/ ),
array( 'slug' => 'deep', /* ...*/ ),
),
'theme' => array( /* come from theme's theme.json, after removing those whose slug is the same as any of the default presets */
array( 'slug' => 'test', /* ...*/ ),
),
)
This is the base behavior for all presets. From there, this data is used differently by the components:
- UI: some design tools may want list the presets from source (this is what colors do) but others display them all together (font families).
- CSS: in general, every preset item available in the
default
andtheme
array will be converted to--wp--preset--category--slug
(e.g.:--wp--preset--shadow-natural
). This was the issue Alex raised and that is fixed by this PR.
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.
Thank you for the additional clarification. I was understanding correctly, but perhaps not communicating that understanding very well.
When I say "overridden" I mean that the bug was that the Theme preset would take precedence over the Core preset.
Now with this PR the Core preset takes precedence over the Theme preset.
The condition is that they must have a matching slug. This is why you've added a "Test" preset that only exists in the Theme to verify that this functionality still works.
Committed in https://core.trac.wordpress.org/changeset/57885 |
All discussion for this issue should be done in WordPress/gutenberg#59989
Alternative to #6295
Default shadow presets are never shown for classic themes, and classic themes have no options for adding custom ones.
Essentially, this is the existing behavior for classic themes in 6.4.
This allows us to defer decisions on whether or not to make them opt-in or opt-out for classic themes until 6.6.
Trac ticket: https://core.trac.wordpress.org/ticket/60815
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.