Global styles: keep custom CSS when switching between style variations #56623
Conversation
glendaviesnz
added
[Type] Enhancement
|
@richtabor, @SaxonF this is a draft PR that stops custom CSS being wiped when switching theme variations. UX wise, do you think it needs some explanation on the variation switcher menu to say the custom CSS will be preserved, or an option there to toggle whether it is preserved or wiped? If you think the toggle to choose is needed where would you place that and how would you word it? |
|
Size Change: +1.69 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Question: what happens if you switch from a block theme to a classic theme, is it preserved? Asking because when you switch from a block theme to a block theme, we surface this if you preview first:
Really, that activation dialog should always show, regardless of whether you preview or not, when you switch from a block theme to a block theme. Can we make that happen? And can we show, on that list, a checkbox that says "Custom CSS", checked by default? We ideally need to show every change, in a long and detailed list, in that modal, just like we do when you change titles or styles:
If we can't do this immediately, we might need a lower level solution — a notice? A snackbar? But long term, we need to make progress on block theme switching improvements. Here are some thoughts that deserve a good mockup to tie the room together:
Ultimately you should be able to combine all of those aspects together. Consider this: for Black Friday I'm having a sale on my store. So I change my site logo, I add a suffix to my site title, I change my colors and fonts, I add a banner at the top of my header pattern, and I switch to a new block theme but only to get the new templates, nothing else. Not only that, but after queueing up all these changes, I schecule them to go live on November 24th. It's a tall order, but the single interface here should be able to break out all those changes, list them, allow me to schedule in a split save button/dropdown combo. Considering this as the end state, what do we do today? Nice PR! |
The custom CSS is saved against the block theme, so if you switch to a classic theme it won't transfer to the classic theme, but it will still be there when you switch back to the block theme. The switching between block themes is a slightly different issue as each theme maintains its own global styles entity, so you don't lose your custom CSS in the same way when switching themes as you do when switching variations. Your suggested enhancements sound great, but I wonder if we should focus on just the custom CSS data loss issue when switching variations in this PR - but in doing so being mindful of the direction the wider theme switching flows might be heading? |
Ah yes, so it's definitely similar to copying over the patterns, templates, colors, fonts, or other styles from your old block theme to the new one. Only, those don't "feel" like data-loss, because they are understood to be theme related, whereas (and I agree), custom CSS feels like it's a separate layer on top.
Absolutely and exactly — it's just important that we visualize the end-state, so that whatever fixes we apply in the mean time do not prevent us from moving to that endstate at a later time.
|
|
Thanks for the feedback @jasmussen, I will explore some different options for notifying the user about what is happening - @richtabor, @SaxonF feel free to still add in any thoughts you might have on UX around this issue with custom css being lost when switching variations. |
glendaviesnz
changed the title
|
Having had another look at this, with the style variation switching a dialog to confirm keeping of custom CSS doesn't seem to work as users are likely to want to switch quite quickly between style variations and a dialog before applying each time is going to be painful. I have added a toggle to this PR, defaulting to on, which preserves any additional CSS when toggling between variations - and they can then see what the variation looks like with and without their custom css. This doesn't fix the wider issue here of all user global styles settings being lost - this is a much trickier issue as these would require a complex merging of user and variation styles. I think the solution to that issue would be to create new global styles entity for each variation switch so at least the custom styles are preserved for the variation they were applied to and still there when switching back. This sort of elevates a variation change to the same level as a theme change so it needs a bit more discussion about whether we want that or not. We could fix the custom CSS though independently while we decide if the variation switching process should be reviewed or not. This PR is definitely just a suggested approach, open to ideas/feedback on this. |
| ...( blockStyles[ blockName ] | ||
| ? blockStyles[ blockName ] | ||
| : {} ), | ||
| css: user.styles.blocks[ blockName ].css, |
There was a problem hiding this comment.
need to modify this as theme variations can have block level CSS set so we need to merge instead of overwriting here
|
The toggle isn't quite working correctly yet - toggle on and off doesn't always remove and apply the custom CSS correctly - will get it fixed. |
|
Flaky tests detected in fbdd11a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7041635765
|
|
Ok, the toggle is working more how it should, providing a live preview of how the style variation looks with and without your custom CSS: toggle.mp4The code needs a tidy up and a good test, but I will hold off doing that until there is some agreement that this is worth implementing as a temporary fix to this issue while wider changes to variation/theme swapping are explored. |
glendaviesnz
changed the title
| ...variation, | ||
| settings: variation.settings ?? {}, | ||
| styles: variation.styles ?? {}, | ||
| } ) ), |
There was a problem hiding this comment.
Will extract the following into a method/hook, simplify the object spreading, and add some tests if there is some agreement to go with this approach
| currentUserStyles.styles.css, | ||
| preserveAdditionalCSS, | ||
| ] ); | ||
|
|
There was a problem hiding this comment.
would be good to move the below to an event handler rather than useEffect, but need to refactor the update flows slightly to do this - will worry about that if we decide to go ahead with this refactor
glendaviesnz
changed the title
|
@richtabor, @SaxonF did you have any thoughts on this approach to keeping the custom css when switching variations? It seems like there needs to be some wider discussion about how user global style changes are managed in the variations context, but this change could at least help prevent additional CSS data loss in the meantime and does not affect the data structure at all so shouldn't impact any future changes in relation to style variation data storage. |
|
Thanks for putting this together @glendaviesnz . Agree there is more thought to be put into style variation management, in particular if colours and fonts are pulled out. There is a proposal here that prompts a user if they want to save their changes to a variation before switching. I think we could take a similar approach here so instead of a checkbox we just prompt if we detect CSS changes "Do you want to bring your CSS changes over to the new variation?" |
+1 to this. |
|
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. |
|
To expand a bit on the note here, there's a long term view that blurs the boundaries between switching between block themes, and saving multiple entities. Theoretically you should be able to batch site logo, title, page content, synced pattern changes and more with switching a theme, and then schedule it to go live at a certain time. That's a mouthful and we want to carefully work towards that, even just unifying the publishing flow is one first step. Any small changes that can unstick this in the mean time are fine to do. But just to further the idea of where we can go in the future, here's a slightly fresher sketch of what a block theme activation modal could look like: |
|
Trying to unstick this, I want to ask a possibly obvious question: why would you ever untoggle "keep custom CSS"? Is there any use case for not keeping it when switching themes and style variations? If we decided that no, custom CSS is kept until you manually edit or delete it, then we can remove the checkbox and solve the issue with a minimal version of this branch. CC: @jameskoster @annezazu @richtabor |
|
Keeping custom css when switching between style variations seems like a decision we can make. But as for switching themes entirely, that is really hard to say. If your custom css is based on the active theme (maybe using css vars, or adding integration for a plugin) then you probably wouldn't want to bring those styles with you. I have no idea how common that is though. Given it's relatively easy to manually remove custom css, it may be okay to remove the checkbox, if that will unstick this work? |
For switching themes, we should definitely have a checkbox in the theme switching modal. But for style variations, this PR can land with that always kept, IMO. |
I see it like this: If unedited If the |
I could see a case where someone doesn't mind losing the CSS and would untoggle it. With that said, that toggle is still great information to have and, without it right now, it's leading to a form of content loss (where this all started). I think adding a toggle would go a long way or following what Rich shared above. |
|
I'm unsure how to move this forward. @glendaviesnz how are your feelings on this for 6.6? Although I don't love extracting this one piece from the flow and adding a toggle, it's also not the end of the world. Can the toggle be contextual so it only shows up if you do have custom CSS? Also CC @scruffian, as I think he had thoughts as well. |
|
sorry @jasmussen I have been out of the loop on this on a different project. In terms of narrowing the discussion back down to just the question of custom CSS moving between variations on a block theme is the general agreement that we should just make this happen by default, so no toggle option, if the user wants to |
|
Thanks for the update. I'm a fan of that approach. I think @scruffian had some nuance, and CC @annezazu as well. |
|
I'm a fan of that approach too. If someone is adding CSS, I'd expect them to want to keep it. Of course, some folks might forget they added it in the first place but, if they found the CSS to add, I would expect they would find it once more to remove. |
|
Okay, IMO that sounds like a reason to unblock this. @glendaviesnz if you have energy to pick this back up, should we try with no toggle for now? |
|
Something got messed up while trying to rebase this, going to just open a new PR given direction has changed slightly as will be quicker than sorting the rebase. Closing in favour of #61752 |
What?
Preserves user custom CSS when switching between style variations
Why?
User can find it surprising that their custom CSS is lost when they switch to a different theme variation.
Fixes: #47433
How?
Adds a toggle to specify if any existing custom CSS should be copied over to the new style variation CSS
Testing Instructions
Keep additional CSSand check that the custom CSS is wipedScreenshots or screencast
Before:
save-css-before.mp4
After:
save-css-after.mp4