Global Styles: Only use single property variations as color/type presets #62469
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
scruffian
added
[Type] Bug
|
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. |
|
Size Change: -147 B (-0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
@scruffian, according to the specification...
...shouldn't there be just one preset corresponding to the only color-only variation in TT4 (Onyx)? At least this is the behavior I would expect |
The other one is the default, so you can switch back to it. |
That was my guess, yes. I understand the need to reset to the default colors. However, I find this mechanism 'not immediately easy to grasp' in its current form. Are there other approaches (from a UI perspective) to 'un-apply' the preset that could be used here? I think this whole feature would be a bit easier to understand if there was only as many presets available as the ones generated from color-only variations (or typography-only variations). But maybe this is just me. I'll let others chime in and share their feedback. |
|
I think this approach is fine. It just looks strange in TT4 due to the way the style variations were originally built. Ideally, TT4 would have more dedicated color-only style variations to demonstrate this feature, or the Onyx style variation could be updated with an additional superfluous setting to disable the color-only palette generation. |
|
cc @jasmussen for a design perspective |
|
I think it's fine that the first one is the default. I'd rather update TT4 with more colors than opt the theme out of this completely. It's a symptom of how many theme variations are built (to be color only). |
It would be a nice marketing highlight as well. "With WordPress 6.6, the Twenty Twenty-Four theme has been updated to include X new color palettes..." |
|
I tested this with the Frost theme, and it works |
jasmussen
approved these changes
Jun 12, 2024
ajlende
reviewed
Jun 12, 2024
| @@ -51,14 +51,12 @@ export function removePropertyFromObject( object, property ) { | |||
| * A convenience wrapper for `useThemeStyleVariationsByProperty()` that fetches the current theme style variations, | |||
There was a problem hiding this comment.
Since useThemeStyleVariationsByProperty is deleted, this comment and the file names should probably be updated.
ajlende
reviewed
Jun 12, 2024
| @@ -71,27 +69,26 @@ export function useCurrentMergeThemeStyleVariationsWithUserConfig( { | |||
| }; | |||
| }, [] ); | |||
| const { user: baseVariation } = useContext( GlobalStylesContext ); | |||
| const clondedBaseVariation = cloneDeep( baseVariation ); | |||
There was a problem hiding this comment.
Typo.
Suggested change
| const clondedBaseVariation = cloneDeep( baseVariation ); | |
| const clonedBaseVariation = cloneDeep( baseVariation ); |
There was a problem hiding this comment.
Actually, I don't think there's any reason to do a deep merge here. mergeBaseAndUserConfigs uses deepmerge which doesn't mutate the original object.
There was a problem hiding this comment.
Since I fixed a bug in ba440bd, I think we do need the clone again..
ajlende
reviewed
Jun 12, 2024
Comment on lines
74
to
80
| const variationsWithSinglePropertyAndBase = variationsFromTheme | ||
| .filter( ( variation ) => { | ||
| return isVariationWithSingleProperty( variation, property ); | ||
| } ) | ||
| .map( ( variation ) => { | ||
| return mergeBaseAndUserConfigs( clondedBaseVariation, variation ); | ||
| } ); |
There was a problem hiding this comment.
Move this inside the useMemo. As it is now, the useMemo is just pure overhead because this is creating a new array every time.
|
I found a bug in this - the default variation doesn't contain any other user settings, so when I select it, everything is reset, not just the colors. |
|
@ajlende thanks for the review, I think I fixed all the issues :) |
ajlende
reviewed
Jun 14, 2024
...ges/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js
Outdated
Show resolved
Hide resolved
...ges/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js
Outdated
Show resolved
Hide resolved
| return isVariationWithSingleProperty( variation, property ); | ||
| } ) | ||
| .map( ( variation ) => { | ||
| return mergeBaseAndUserConfigs( baseVariation, variation ); |
There was a problem hiding this comment.
Should this be using the clonedBaseVariation? If removePropertyFromObject mutates the argument, then the old code below is merging the one without the property.
There was a problem hiding this comment.
Yes you're right, great catch. We want to apply only the typography styles from the variation, not include the ones from the base. I updated the PR and renamed a few variables too for clarity.
…eme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz>
…eme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz>
scruffian
force-pushed
the
update/color-and-type-variations
branch
from
2c48f57
to
b3388de
Compare
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…ets (WordPress#62469) * Global Styles: Only use single property variations as color/type presets * remove unused code * remove unused code * remove unused code * update comment * fix typo * Add the user settings to the base variation * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz> * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz> * Remove the property from the user variation before merginng it with the new variation * apply the name to the correct object --------- Co-authored-by: Alex Lende <alex+github.com@lende.xyz> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: juanmaguitar <juanmaguitar@git.wordpress.org> Co-authored-by: ndiego <ndiego@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org>
|
Is this 6.6 related? Should it be backported? |
Yes, I believe it should be. Can you confirm @richtabor and @scruffian? |
|
Yes sorry it should be |
scruffian
added
the
Backport to WP 6.7 Beta/RC
ellatrix
pushed a commit
that referenced
this pull request
Jun 25, 2024
…ets (#62469) * Global Styles: Only use single property variations as color/type presets * remove unused code * remove unused code * remove unused code * update comment * fix typo * Add the user settings to the base variation * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz> * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz> * Remove the property from the user variation before merginng it with the new variation * apply the name to the correct object --------- Co-authored-by: Alex Lende <alex+github.com@lende.xyz> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: juanmaguitar <juanmaguitar@git.wordpress.org> Co-authored-by: ndiego <ndiego@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org>
|
I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: dbd9830 |
ellatrix
added
Backported to WP Core
ellatrix
pushed a commit
that referenced
this pull request
Jun 25, 2024
…ets (#62469) * Global Styles: Only use single property variations as color/type presets * remove unused code * remove unused code * remove unused code * update comment * fix typo * Add the user settings to the base variation * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz> * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Alex Lende <alex+github.com@lende.xyz> * Remove the property from the user variation before merginng it with the new variation * apply the name to the correct object --------- Co-authored-by: Alex Lende <alex+github.com@lende.xyz> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: ajlende <ajlende@git.wordpress.org> Co-authored-by: juanmaguitar <juanmaguitar@git.wordpress.org> Co-authored-by: ndiego <ndiego@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: colorful-tones <colorful-tones@git.wordpress.org>
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?
In this PR we change the behaviour of color and typography presets so that we use only variations that contain nothing but that specific property.
Why?
This reduces the impact of the change to introduce color and typography presets as it means that only variations that target a specific property are used.
How?
Filter out all variations unless they contain only a specific variation.
Testing Instructions
Screenshots or screencast
Before:
After:
