Global Styles: Allow variations to be filtered by multiple properties #62847
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
update variables add filtering to the default variation add spacing to variation
scruffian
added
the
[Type] Code Quality
|
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: -11 kB (-0.63%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jul 1, 2024
| ]; | ||
| }, [ property, userVariation, variationsFromTheme ] ); | ||
| }, [ properties, userVariation, variationsFromTheme ] ); |
There was a problem hiding this comment.
I'm not familiar with this code at all but there's something that is probably not right here. If you're using properties as a dependency for a useMemo call, I'm guessing that you expect the same object instance to be returned here if the properties don't change (maybe for some performance reasons), but here it's almost never the case since properties is passed as an inline array, so properties will always trigger a recompute and a new instance returned by useMemo.
There was a problem hiding this comment.
Ah yes good point. Would it work to spread the properties in the dependency array, like this?
Suggested change
| }, [ properties, userVariation, variationsFromTheme ] ); | |
| }, [ ...properties, userVariation, variationsFromTheme ] ); |
There was a problem hiding this comment.
That wouldn't work either, because the size of the properties array might change and I believe it's not allowed in these react hooks to have a variable size.
There was a problem hiding this comment.
What if we convert the array to a string?
… can compare them accurately
|
I'm not familiar enough with this part of the code to do a full review here. I'll let others chime in. |
ramonjd
reviewed
Jul 3, 2024
There was a problem hiding this comment.
I couldn't see any regressions in the editor - between trunk and this PR the style variations in TT4 are the same in both the Site Editor sidebar (view mode) and the global styles inspector. 👍🏻
The change makes sense to me - extracting several properties at a time.
Looks like the unit tests need to be updated.
...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
...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
...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 ( | ||
| JSON.stringify( variationWithProperty?.styles ) === | ||
| JSON.stringify( variationWithProperties?.styles ) === |
There was a problem hiding this comment.
Optional, but also available is areGlobalStyleConfigsEqual
const { areGlobalStyleConfigsEqual } = unlock(
blockEditorPrivateApis
);
...
return areGlobalStyleConfigsEqual( variationWithProperties, variation);
…eme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
…eme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
…eme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
…eme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
|
Thanks for the review! I have updated the tests, hopefully this is good to go :) |
ramonjd
approved these changes
Jul 4, 2024
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…WordPress#62847) * Global Styles: Include spacing rules as part of typography variations update variables add filtering to the default variation add spacing to variation * Global Styles: Allow presets to use more than one property * treat the properties in the dependency array as a string so Object.is can compare them accurately * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * Update packages/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js Co-authored-by: Ramon <ramonjd@users.noreply.github.com> * update docs * use areGlobalStyleConfigsEqual * update tests --------- Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@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?
This updates several functions to allow presets to incorporate several properties.
Why?
This was originally proppsed as part of #62709, and was also part of #62741. It's not clear what direction we want to go with this, but this code maintains the same functionality while adding flexibility by allowing you to provide an array to these functions rather than a string.
How?
Update several functions to accept an array rather than a string.
Testing Instructions