WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.45k stars 4.17k forks source link

Global Styles: Store active variation origin #46397

Open talldan opened 1 year ago

talldan commented 1 year ago

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

https://user-images.githubusercontent.com/677833/206418961-054084a7-bdfd-4272-ae9b-e19f38c8a624.mp4

Environment info

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

ramonjd commented 1 year ago

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.

https://user-images.githubusercontent.com/6458278/207509216-9cad09bb-48db-4e08-ba59-9b0790d7b7bd.mp4

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

talldan commented 1 year ago

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 commented 1 year ago

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

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

youknowriad commented 1 year ago

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:

Each one of these has pros and cons.


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 commented 1 year ago

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 commented 1 year ago

@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 https://github.com/WordPress/gutenberg/issues/43395#issuecomment-1425375575.

talldan commented 1 year ago

@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.

annezazu commented 1 year ago

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 commented 1 year ago

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 😄