Avoid override certain paths on theme variation switch #59921
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 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. |
matiasbenedetto
requested review from
pbking,
scruffian,
mikachan and
creativecoder
matiasbenedetto
added
[Feature] Themes
|
Size Change: +150 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
| } ) ); | ||
| setUserConfig( ( currentConfig ) => { | ||
| // Avoids overwriting certain paths when applying a theme variation. | ||
| for ( const path of PATHS_TO_AVOID_OVERWRITING ) { |
There was a problem hiding this comment.
I don't understand why this needs to be a loop. Isn't it only really one path we want to avoid overwriting?
There was a problem hiding this comment.
It is currently but that could change in the future. If we wanted to simplify this PR we could avoid the loop and make the "path" a single string and not an array of strings.
There was a problem hiding this comment.
I wonder if that would be better, if we don't have other cases that this need to happen for.
There was a problem hiding this comment.
As Dave said, the loop makes the code simpler when more paths are added. In fact, I added a new path for custom color palettes in this commit: 651b30d
|
Just wanted to share that I am not sure I agree with the proposed change in this PR. Let me think about it more and I'll come back with better thoughts. Let's also try to get some opinions here |
|
Is the idea of the PR to keep "active font families" consistent when we switch variations? For me given that when switching variations we're also potentially switching the font families that are actually used... it doesn't make sense to keep the same "active font families" (even custom ones) because you want to optimize your frontend with and only load the ones that are used. If you want to reuse more font families, I think it's fine to go and active them again. |
|
Pinging Editor Triage leads for opinion - @fabiankaegy @annezazu 🙏 |
|
Thanks for working on this @matiasbenedetto 👍 FWIW, my perspective is that the behaviour when switching variation should be:
The only downside is that if the user has manually applied the Theme variation's fonts from an older Theme variation to specific elements, then these changes will be lost on Theme variation switch. However, that seems reasonable considering that it's unlikely you would want typographic treatments from a different style to continue to apply when you switch that style. |
For me, if that should be the case, it would mean that we need a separate place to store the active fonts and not reuse the "global styles" endpoint/storage... This creates a precedent that I'm not sure that we should allow at the moment.
Theme variation font or something you add manually is the same thing for me. there's no conceptual difference in how things are stored. So I believe we should treat them in the same way. It's just a "preset" for multiple user changes that a user can apply together. |
|
In fact, there's nothing different here compared to "colors" for instance. If I modify the color palette manually (add custom colors there) and then I switch theme variations, my custom colors are gone too. |
Also, if theme variations define less variations than the base variation then any colors you selected from the base variation will be lost when you switch to a variation with less colors. |
This is key here. We are fixing it here just for fonts, which I'm not particularly opposed to, but we gotta keep in mind the "bug", if we decide that's what it is, is affecting everything else that the user changes, not just the fonts. I've been chatting with designers using the site editor, and some of them find it jarring that changing a style variation overrides every single global style made by the user. If the user never saved their changes, those are lost and can't be recovered via revisions, which is very destructive, given that we don't warn that this will happen. I think the reason why this happens like this is because there is no way to go back to the "default" state other than resetting global style changes, but that changes if/when we merge #59717 I suggest approaching this holistically and looking for a way to solve the whole issue, even if we start by fixing just the fonts, we have to take into account the whole flow. We could create a new style variation with the user settings that doesn't get changed if the user changes any of the presets and that is applied on top of them since custom is already a layer on top of the theme. This makes sense technically, I wonder if it's confusing from a UI/UX standpoint. It's still better than losing the user's changes. |
|
From my perspective, theme style variations should only affect settings/styles defined by themes and not the custom styles defined by users. It seems like an error to assume the user wants to switch custom-defined settings such as color palettes and font families when switching a theme variation. What would be the reason to assume that activating a theme variant, which can be as 'small' as changing the spacing on the elements, should remove all the custom settings defined by the user? That intention can be safely assumed on a theme switch because of how historically themes worked on WordPress but not on theme style variations that are only partial changes of the same theme. From a technical perspective, the settings defined in the theme style variations are treated correctly as
Nope, that's not the only objective, that's only one of the objectives.
Exactly, that's unexpected, and there's no good reason that the user wants to wipe all the custom configs laboriously created.
They can be added to the list when we identify more areas where this is needed. I added more screencasts in the PR body featuring the differences between this PR and trunk. |
I think what's missing here is that while theme style variations are "mostly defined" by themes today, that's just a temporary thing. Variations are a set of styles and settings that override your own settings and styles entirely. The endpoint is already filterable, so they can come from plugins too and ultimately, users will also be able to save their own settings that they customized in the UI as a variations. So consider "custom" keys as "user changes" is actually wrong. Anything can be customized through the UI of global styles, not just the "custom" presets. Even "theme" presets can be overridden. (in fact it's already possible to do that for colors in the UI) Now, I agree that there are downsides to the current behavior, it's not clear to the user that he's replacing "everything" when picking a variations and this one of the reasons we're now exploring color presets and typesets as separate variations for the users to pick from. When picking a "color preset" only color related changes are overridden... I think overall, the issue here is more a UX issue, and I agree with @MaggieCabrera that we should take a holistic approach here. I think there's no rush in finding a solution now, this issue is present in global styles for multiple releases now. I'm sure this has been discussed already elsewhere so linking @jasmussen and @jameskoster |
That's a good point, but I think that it's correct to assume if the user is updating the presets labeled as 'theme', they will change when theme-related styles change but not when adding 'custom ' configs.
|
A quick CC for @SaxonF and @richtabor as well. The majority, for me, comes back to the fact that we have highly visible undo/redo buttons in the top toolbar, and we now also have very visual style revisions: #58578 (comment) That means whatever changes you make across are very easy to revert, so there's little harm done, whatever issues may be at the root. The point made is also clear, this is an issue present even with global styles. We have this issue to allow saving and managing custom style variations. A key piece: 
We can revisit this flow based on learnings since that issue was made, but the main flow is the same, whether it's complete style variations, or just color or font variations. This should mainly address the original issue that any customizations you've made on top of a chosen variation makes it show up as an unsaved custom variation. Like the undo stack and the style revisions, this ultimately adds to the logbook for what you've done. Which IMO is ultimately the solution here: we do allow you to change broad swathes of visuals in a single click, because it's trivial to go back. |
+1000. It has been discussed a lot, but I don't think I've seen a holistic design vision that connects all the things.
Imo this is the main hurdle to tackle – what is the ideal UX when you 'edit' a chosen variation, or switch to another variation? Do we mark variations as edited and provide a reset action (similar to theme templates)? Or, do we have a persistent "Custom" variation which becomes active when you edit an existing variation? Or do we do something else entirely? Solving this will address a big pain point in the whole global styles experience. |
|
This has come up in MANY different ways as evidenced in this related issue: #46397 It's really jarring to switch variations and lose everything. At the same time, this would technically be consistent with what is today. We also have more robust revisions at this point so, in theory, it should be "safer" to lose things considering one can compare and roll back. This feels larger than this release to solve as others have noted so I think it's not a deal breaking thing to fix this time around. I do think this current experience needs to be greatly improved or, at least, communicated better ("save before switching variations"). |
|
This is particularly annoying when you have added custom CSS which also gets removed. This work, which decouples fonts/colors from variations, should help a little bit as you can try fonts/colors from other variations without overriding other properties. You can make changes to templates provided by the theme , and "clear customizations" if needed, so I'd expect similar behaviour here. If I make a change, including choosing a font or colour set, I should be able to save those changes to the variation. If a user attempts to change variations without saving, we prompt to save or create a new variation before switching. I should be able to "clear customizations" to go back to theme defaults. |
What?
Avoid overriding certain paths on the theme variation switch.
Why?
Theme variation switching overrides certain
theme.jsondata paths that are not supposed to override.A theme variation is data intended to change the
themelevel data and not thecustomdata and it should only affect settings/styles defined by themes and not the custom styles defined by users.This change is added to fix part of this issue #59818
How?
Add an array of
theme.jsonpaths that the theme variation should not overwrite.Testing Instructions
settings.typography.fontFamiliestheme.json data (for example TwentyTwentyFour).Use cases
Custom colors
Let's say the user want to add 2 colors to create a notice paragraph on their posts. This is how it would work:
Trunk
2024-03-18.15-43-23.mp4
This PR
2024-03-18.14-44-25.mp4
Using custom fonts in posts
In this example, I'm installing a custom font and using it in a post. On Trunk, after applying a style variation, the custom font selected by the user previously is not rendered. Running this PR it is.
Trunk:
2024-03-18.15-58-51.mp4
This PR:
2024-03-18.16-04-46.mp4
Active fonts
In the trunk one the active fonts installed using the font library are lost of theme variation switch and in the this PR the font families remain active. It means that the active fonts used in posts or blocks will continue working as expected.
Trunk
2024-03-15.14-59-00.mp4
This PR
2024-03-15.14-54-46.mp4