Copy custom CSS between variations when switching #61752
Merged
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
glendaviesnz
added
[Type] Enhancement
|
Size Change: +164 B (+0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
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. |
|
How does this test for you @jasmussen |
jasmussen
approved these changes
May 17, 2024
There was a problem hiding this comment.
This is working great. I'd appreciate a gut-check by @annezazu and a code check, but this feels obviously the right behavior to me, even if there's a technical exception made.
Video shows me adding some custom CSS, saving, then switching style variations and that CSS persisting.
custom.css.mp4
|
This looks great to me. Ideally, as part of showing where styles come from in the future #49278 we can connect the dots to CSS but this feels better than the current situation with the CSS being lost entirely without warning. cc @WordPress/outreach in case anyone has time to test or has additional feedback especially since this has come up a fair amount in feedback from the former FSE Outreach Program. |
glendaviesnz
force-pushed
the
fix/custom-css-loss-in-variation-picker
branch
from
a6afd08
to
e5d6b08
Compare
|
@scruffian you were the most recent person to touch this file, do you have time to review this change please which keeps custom CSS applied when switching between variations. Background discussion here #56623 |
|
Some of my changes have broken things slightly - will try and get this fixed in next day or two |
|
Flaky tests detected in 95e4842. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9296407302
|
scruffian
reviewed
May 22, 2024
There was a problem hiding this comment.
If I create some custom CSS, and then reset Global Styles, the CSS is stripped from the default styles (expected), but it's still in the variations (not expected). I think we need to ensure that the user styles respond to the reset as well?
I do also have concerns about this feature more generally because it creates an expectation that user level changes are persisted when changing theme variations. If that's something we do for CSS, then I think users will expect that behaviour for all "user-set" values, but don't let my opinion block this work.
|
Thanks @scruffian, are you able to give me some steps to reproduce that issue, for me it clears from variations when clearing styles, but I may be misunderstanding what you are saying: clear-css.mp4@jasmussen, @annezazu do you have any thoughts about @scruffian's concerns? |
|
Ben is not wrong, but in this case the exception to the system solves a specific problem, and the merge button is green. I think the main question we need to answer, knowing this, is how we feel about the code quality, the documentation of this exception, and our confidence level of whether this ends up eventually becoming technical debt. I'd be okay with moving forward, because the omission of this behavior feels unintuitive and non-obvious; CSS currently lives in global styles, but it isn't clear that "styles" are always what people use CSS for, especially as more and more visual properties move to design tools. To that end, the fact that CSS lives in the same scope doesn't necessarily feel fully thought through, if we re-did this I would place it in more of a global scope, one that wasn't even reset when you reset global styles. But these are not strong opinions and I wouldn't want to ruin Ben's day! |
|
To reproduce the bug: Screen.Recording.2024-05-23.at.11.46.17.mov@jasmussen do you think it also makes sense to make the same exception for other user set styles? |
Not really, no. I think custom CSS, whether block-local, or global, is the only exception. In fact extracting the font pairings and color palettes to be separate from style variations is one aspect of giving options that aren't as sweeping as style variations are. And yes, a way I think of style variations is to think of them as alternate versions for a theme, changing everything potentially. We separately have an opportunity to make much more prominent the revision history feature, than it is now. It's an extremely powerful way to rewind accidental changes. Combined with clearer multi entity saving, you might end up in a place where all of these bits are more known. But yes, custom CSS feels like the exception, and something okay to persist, since it's explicitly typed out. |
Just looping back and wanted to note that I agree here. I think this change is also more "future proof" in that in the future, there are plans to make the Styles section more obviously globally and refined #53483 which I think would make keeping CSS even more obvious than it is now to do. |
|
@scruffian this should fix the bug you noted if you have time to retest please. |
glendaviesnz
force-pushed
the
fix/custom-css-loss-in-variation-picker
branch
2 times, most recently
from
175619f
to
385d2a1
Compare
|
@scruffian if you have a chance to give this another test it would be good to get it into 6.6 RC if you are happy with Joen and Anne's reasoning around the need for this. Let me know if you don't have time and I will ping some others. |
ramonjd
approved these changes
May 29, 2024
packages/edit-site/src/components/global-styles/style-variations-container.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/style-variations-container.js
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/style-variations-container.js
Show resolved
Hide resolved
packages/edit-site/src/components/global-styles/style-variations-container.js
Outdated
Show resolved
Hide resolved
|
LGTM - thanks again 🙇🏻 |
glendaviesnz
force-pushed
the
fix/custom-css-loss-in-variation-picker
branch
from
d4ecb05
to
ecbefc0
Compare
glendaviesnz
force-pushed
the
fix/custom-css-loss-in-variation-picker
branch
from
ecbefc0
to
95e4842
Compare
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.
What?
Copies any user custom CSS between variations when the user if switching between theme variations
Why?
Currently, if a user switches theme variations all their custom CSS is lost. It has been decided that this should move between variations by default. See background discussion on #56623
How?
Checks if there us any user custom CSS, either global or block level, and if so this is copied to the new variation.
Testing Instructions
Screenshots or screencast
Before:
custom-css-before.mp4
After:
custom-css.mp4
From right global styles menu:
gs-menu.mp4