Skip to content
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

Global Styles: Store active variation origin #46397

Open
talldan opened this issue Dec 8, 2022 · 9 comments
Open

Global Styles: Store active variation origin #46397

talldan opened this issue Dec 8, 2022 · 9 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@talldan
Copy link
Contributor

talldan commented Dec 8, 2022

Step-by-step reproduction instructions

  1. Open the Global styles panel
  2. Select Browse Styles and choose a variation with a noticably different color palette to the default
  3. Use the back arrow and select 'Colors'
  4. Open the Palette
  5. From the Theme color options dropdown (the three dot icon) select 'Reset colors'

Expected: Colors are reset to the active variation color palette. (or the 'Reset colors' option shouldn't be available)
Actual: Colors are unexpectedly reset to the default theme colors

Screenshots, screen recording, code snippet

Kapture.2022-12-08.at.18.03.22.mp4

Environment info

  • Latest Gutenberg trunk
  • Mac OS / Brave

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@talldan talldan added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Dec 8, 2022
@talldan talldan changed the title Global styles color palette - using 'Reset colors' while a variation is active resets to the default theme colors Global styles color palette - using 'Reset' while a variation is active resets to the default theme style instead of the variation style Dec 14, 2022
@ramonjd
Copy link
Member

ramonjd commented Dec 14, 2022

It looks like that variation settings and styles are written to the user's global styles config, and the path 'color.palette.theme' is completely wiped with undefined when resetting color palette defaults.

Do you think perhaps we should be storing the default active variation somewhere or a pointer to it? That way we could know the selected variation and reset the palette from the base theme settings.

It might involve a general rethink about how we reset global styles as well.

For example, if I select a variation, then change some global styles, should "Reset global styles" reset back to the variation's default settings? At the moment it's all wiped.

2022-12-14.15.53.07.mp4

Maybe @youknowriad or @jorgefilipecosta (I plucked your names from git annotations!) might have an instinct of what should happen?

@talldan
Copy link
Contributor Author

talldan commented Dec 14, 2022

For example, if I select a variation, then change some global styles, should "Reset global styles" reset back to the variation's default settings? At the moment it's all wiped.

The top level reset could possibly work differently, as Browse styles is encompassed by it visually. The top level reset does seem fairly dangerous though, I have wondered if it should be less visible rather than on the top bar all the time (and visible when navigating to individual settings). I would be interested to know if there's ever been any feedback about that (cc @annezazu).

I do think it's really unexpected when resetting an individual setting that it'll undo the style variation, as the user then ends up with a mishmash of some styles from the variation and some from the theme defaults.

@aaronrobertshaw
Copy link
Contributor

Would it be desirable to officially include an additional theme.json origin?

So we'd have something like: Core > Theme > Variation > User

@youknowriad
Copy link
Contributor

Yeah, this is the expected behavior for now. We discussed a number of times whether we should change it or not, but the issue is actually not that simple.

When initially developing style variations, we had two choices:

  • Save the active variation as a "flag"
  • Save the active variation as user changes

Each one of these has pros and cons.

  • If you save as a flag, you can reset to the active variation easily
  • If you save as a flag, you'll introduce a dependency between the style generation and the way variations are provided in other words, the framework would have to know how to "fetch" variations from the theme... making it harder for plugins, filters to provide "virtual" variations that doesn't come from the theme, or making it harder to download variations from a directory or other themes (these things are use-case and features we discussed about implementing)

1- If you save as user change (like now), you can't restore to the active variation easily
2- If you save as a user change, you don't care about the "source" of the variation, they can come from anywhere.


So ultimately, we probably need both but in order to implement it properly, we need to have a "central place/abstraction" in WordPress where we'd "store/fetch" variations independently from the source (in other words, multiple sources could plug in there).

For pragmatism we went with the second solution initially, because it introduces no new concepts/APIs and allow us to solve 90% of the use-cases. Designing the right abstraction for 1 is harder and requires more thoughts.

@annezazu
Copy link
Contributor

I would be interested to know if there's ever been any feedback about that (cc @annezazu).

This exact issue hasn't yet been discovered with the FSE Outreach Program but that's likely a product of the tests that have been done rather than a reflection of user expectations. I would agree that while this is working as intended for now, it definitely breaks expectations. While this direct situation hasn't come up, similar feedback has that reflect the same underlying expectations:

Generally speaking, I think when folks make changes to Styles they expect a way to save them and be prompted not to lose them outright.

@Mamaduka
Copy link
Member

Mamaduka commented Mar 8, 2023

@talldan, do you mind changing the issue title to be more generic for how variations are saved? Then I think we can close duplicate issues like #43395 (comment).

@talldan
Copy link
Contributor Author

talldan commented Mar 9, 2023

@Mamaduka Would you be happy to go ahead and do that? I'm not really sure what the best title would be as I haven't looked at this issue for a while.

@Mamaduka Mamaduka changed the title Global styles color palette - using 'Reset' while a variation is active resets to the default theme style instead of the variation style Global Styles: Store active variation origin Mar 9, 2023
@annezazu
Copy link
Contributor

Noting this was mentioned as a pain point in the FSE Outreach Program's Rapid Revamp call for testing:

When I made some revisions to the site design and clicked “Reset to defaults,” I expected to be taken back to the default settings of the active style (grapes), but instead the Editor presented me with TT3 default theme.

@ramonjd
Copy link
Member

ramonjd commented May 29, 2023

So ultimately, we probably need both but in order to implement it properly, we need to have a "central place/abstraction" in WordPress where we'd "store/fetch" variations independently from the source (in other words, multiple sources could plug in there).

It's probably not a very appetising UX, but what if we obliged users to save the style variation selection before closing the panel? (Or better, we perform a background HTTP POST when the panel closes). We could save it as a separate wp_global_styles post type, attributed to the current theme.

That way we could fetch the "revision" at anytime.

As for pluggability, would there be anything against adding a filter in get_style_variations()?

Another thing is how to deal with reset hierarchy? Perhaps we need some UI so that user can select which level they'd like to reset, e.g., "Reset to last style variation" or "Reset to theme default").

Just thinking out loud here so take it with a wheelbarrow of good salt 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants