Theme variations UI: ensure that equality check takes into account all default theme properties #41591
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
…aults where properties such as 'settings' or 'styles' are not present in the theme.json. This leads to issues when comparing user and theme variation theme data because Gutenberg global styles provider does add an empty object as a value regardless of availability. We're really talking about a UI problem here, but this is a quick fix to get around that bug.
Using aria-current because selected isn't support by buttons
ramonjd
added
[Type] Bug
|
Size Change: +32 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Jun 10, 2022
There was a problem hiding this comment.
Good catch @ramonjd! The code change looks simple and good to me, and it's testing well with the yellow variant in the Style variations theme:
| Before | After |
|---|---|
 |
 |
LGTM!
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?
Adding a default
settings: {}orstyles: {}values to theme style variations before checking equality.Also adding an
aria-currentattribute to the variation buttons to indicate the currently-selected item.Discovered while refactoring the style variation E2E tests.
Why?
At the moment, if a theme style does not contain one of 'settings' or 'styles', it won't be shown as active in the UI.
Properties such as 'settings' or 'styles' may not not present in the theme.json.
GlobalStylesContext adds empty object value for style and settings by default via
setUserConfig().This is merged with incoming theme.json data, so the properties exist regardless of whether they exist in theme.json.
This leads to issues when comparing user and theme variation theme data, and, therefore, the
activestate of the style variations in the site editor.We're really talking about a UI problem here, but this is a quick fix to get around the bug.
An alternative might look at modifying the default.
Or somehow returning empty objects from the API callback
How?
Adding a default
settings: {}orstyles: {}for style variation so that they match the theme.json model used everywhere.Testing Instructions
For this one we're going to have to go to the testing site: http://localhost:8889/wp-admin
Activate the gutenberg-test-themes/style-variations themes. Here is the working directory in the project.
Fortunately, the "yellow.json" style variation provides an example of our use case. It doesn't have a "settings" property.
You can also add your own to test themes without a "styles" property, by adding a new file under
/packages/e2e-tests/themes/style-variations/styles/{ "version": 2, "title": "Little Birdy", "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" } } }Head over to the site editor and browse the theme styles:
Now ensure that, if you select a new theme style, the
is-activeclass andaria-currentare present.This PR 😄
Trunk 😦