Disable core shadow presets by default, let themes opt-in #58766
Conversation
|
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 ❔ lib/theme.json ❔ phpunit/class-wp-theme-json-test.php |
|
@jasmussen @richtabor Can you suggest UI/copy when core presets are disabled and theme doesn't provide any presets for shadows? 
Duotone Reference: 
|
|
Flaky tests detected in 93d91e0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8017993810
|
I'll defer to Rich if he has any input, but I'd simply omit the entire shadow control, and if need be, the entire panel, if there are no shadows defined. |
|
I agree with this change. Most importantly, 1) it doesn't break existing buttons where the user has saved a shadow. 2) The shadow can still be reset. Kindly, inform me if this is planned to be included in 6.5. I will open new issues for the bundled themes, but it is a matter of when those issues need to be fixed. |
Removing the control or panel would prevent resetting, and the user would be stuck with their shadow if their theme is not updated to opt-in. |
cc. @richtabor, for visibility.
This is a good point. I think we could maintain the reset button even if we hide the rest of the controls, but it might confuse the experience — ie. why is there a reset button but no shadow controls? |
|
In absence of better suggestions, at least doing the same as Duotone does here may be the path forward for now. Then improve both, across, in a holistic way, separately. |
madhusudhand
force-pushed
the
shadow-opt-in
branch
from
d0b8e41
to
8624a1c
Compare
|
@jasmussen Introduced the unset option for shadows and is now ready for review. 
|
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +195 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
I have to look at some other things today, so I'm happy to defer to others. But immediately the square shape stands out to me, and opens the question of whether the shadow presets should be circular and the same size as duotone, or colors as well. I think @richtabor may have some context there, so I'll defer to him. |
|
Also, should we introduce 
|
|
It seems like the same use case would exist for that, yes. |
I don't leveraging the existing duotone UI (circles, with the same unset control). |
What's the scenario here? If someone has applied a core shadow at the global level? Why not enable shadow all the time in global styles (like all other controls) and omit it at the block level if there are no shadow presets?
Yes. The big difference is that we do have core duotone presets... so the control is useful out of the box. If a theme does not add their own shadows (I wouldn't expect existing themes to do so much) then the component looks broken and effectively provides no value—just confusion. The current "always enable the component" flow makes much more sense when the shadow UI affords creating shadows. Then it's added value to the experience. |
I did not understand what this means. |
You can't add a shadow to a block, if there are no presets. |
No, but if you already have added them you get locked in. |
There was a problem hiding this comment.
I think we also need to update the schema in theme.json.
gutenberg/schemas/json/theme.json
Line 80 in 5939c41
This PR is labelled as an enhancement and we're past the enhancement stage for 6.5. Can you elaborate why it is important to have this in 6.5 still? |
|
The core fix was to change the core preset shadows from As the root problem is a bug fix, I guess we may want to change it to Bug fix instead? |
madhusudhand
added
[Type] Bug
|
To me this feels like a Bug Fix. It seems originally introduced prior to 6.5 but I'd be inclined to include it if it produces a better experience for users in 6.5. What do you think @swissspidy? Also pinging @annezazu and @fabiankaegy as Editor Triage leads. We don't have long here as packages will need to be synced shortly. |
|
I trust your judgement 👍 |
|
@getdave i agree on this. Let's include it |
|
Ok @madhusudhand let's bring this in. Perhaps @swissspidy can approve your Core Backport PR? Also please could you test the bug fix works as expected during the Beta 3 release party and subsequent release? |
* disable core presets by default, let themes opt-in * add shadow/defaultPresets to appearance tools optins * update unit tests related to appearance tools * Update packages/block-editor/src/components/global-styles/shadow-panel-components.js Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> --------- Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
|
I just cherry-picked this PR to the cherry-pick-wp-6-5-beta-3 branch to get it included in the next release: a446441 |
getdave
added
Backported to WP Core
* disable core presets by default, let themes opt-in * add shadow/defaultPresets to appearance tools optins * update unit tests related to appearance tools * Update packages/block-editor/src/components/global-styles/shadow-panel-components.js Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> --------- Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
|
I'm also in agreement that this feels bug territory for the record. Thanks for handling this so promptly! |
andrewserong
added
the
[Feature] Design Tools
| @@ -191,7 +191,7 @@ | |||
| "text": true | |||
| }, | |||
| "shadow": { | |||
| "defaultPresets": true, | |||
There was a problem hiding this comment.
@madhusudhand This is a breaking change for themes that have shadow presets with the same slug as the core presets as it changes the CSS output for the theme. It should have a theme.json version bump and migration to maintain backwards compatibility.
I've been discussing this in #58409 (comment) since I want to change the default for the font sizes presets as well.
There was a problem hiding this comment.
This change updates defaultPresets from true to false, and also it is controlled by appearanceTools setting.
Meaning that defaultPresets are off by default and when appearanceTools is set to true, they are enabled, which makes existing themes that are using default presets to work as before.
Also, I created #59893 to discuss on this idea of theme being able to add(override) a preset with same slug as core preset.
There was a problem hiding this comment.
Meaning that
defaultPresetsare off by default and when appearanceTools is set to true, they are enabled, which makes existing themes that are using default presets to work as before.
appearanceTools is also false by default, so it's still a problem for themes that haven't configured either.
Before this change, with a theme that has custom settings.shadow.presets defined with matching slugs to those in the defailt lib/theme.json and settings.shadow.defaultPresets and settings.appearanceTools both undefined, you'll end up with different CSS that gets output to the page.
In the first case the default styles will be output and in the second case, the theme styles will be output.
Here's an example of what I'm talking about.
// Relevant parts of theme.json
{
"settings": {
// `appearanceTools` is not set
"shadow": {
// `defaultPresets` is not set
"presets": [
{
"name": "Natural",
"slug": "natural",
"shadow": "2px 2px 3px #000"
}
]
}
}
}/* Generated CSS before */
--wp--preset--shadow--natural: 6px 6px 9px rgba(0, 0, 0, 0.2);/* Generated CSS after */
--wp--preset--shadow--natural: 2px 2px 3px #000;It's because to the settings.shadow.defaultPresets being used as the value for prevent_override in PRESETS_METADATA in WP_Theme_JSON. The setting defines the behavior of matching slugs in the default and theme origins for presets in theme.json.
gutenberg/lib/class-wp-theme-json-gutenberg.php
Lines 2868 to 2876 in cdfd751
I'm suggesting that we revert this for 6.5 (at least this line since it is the one causing the problems). Then we can use the theme.json version bump to opt-in to breaking changes like this.
I'm adding the theme.json version bump in #58409. So you'd just need to update the migration from that PR to make sure that existing themes are unaffected.
Hopefully that all makes sense. 😅
There was a problem hiding this comment.
Thank for details @ajlende.
I see that it may break a shadow when the following conditions are met
appearanceTools: false- theme defined custom shadows
- and they are defined with same slug as defaultPresets
Also, prior to 6.5 shadows are enabled for button block and in global styles only.
It seems to be very unlikely edge case and shouldn't have big impact on themes.
@annezazu @swissspidy @t-hamano what do you think?
There was a problem hiding this comment.
Also cc @fabiankaegy editor triage co-lead.
There was a problem hiding this comment.
I decided it was best to create a dedicated issue where this can be discussed.
This is also useful as it can be placed in Triage on the WP 6.5 Editor board for review by Editor triage leads.
Leonardus-Nugraha
added
has dev note
|
@Leonardus-Nugraha was there a DevNote published for this PR? |
|
@juanmaguitar this was initially a part of the Miscellaneous Editor Changes dev note, but it was removed as @madhusudhand informed that the change was reverted. |
|
Thanks for the info @Leonardus-Nugraha! |
What?
This PR
Testing Instructions
unsetoption.theme.jsonand verify core presets in the dropdown.