Style theme variations: add property extraction and merge utils #58803
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
|
Size Change: +1 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d6d0d12. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7825744695
|
ramonjd
commented
Feb 8, 2024
| ).__experimentalGetCurrentThemeGlobalStylesVariations(); | ||
| }, [] ); | ||
|
|
||
| const { user } = useContext( GlobalStylesContext ); |
There was a problem hiding this comment.
I think we should. by default, provide a util that just extracts a property from a single object, e.g., 'typography' from a theme.json tree before and make site editor-specific tools like user style merging optional/provide another util.
ramonjd
commented
Feb 8, 2024
| return []; | ||
| } | ||
| /* | ||
| @TODO: |
There was a problem hiding this comment.
Also @TODO, test with all theme.json properties.
Maybe we need a white list of just top-level styles?
ramonjd
force-pushed
the
try/global-styles-theme-variation-filters
branch
from
d6d0d12
to
0840fbd
Compare
ramonjd
changed the title
ramonjd
added
[Type] Enhancement
ramonjd
commented
Feb 13, 2024
| filter, | ||
| baseVariation, | ||
| } ) { | ||
| return useMemo( () => { |
There was a problem hiding this comment.
I measured the performance of this function and useMemo increases performance, assuming the same dependencies.
E.g., The initial operation takes .9ms, subsequent operations with the same dependencies take .01ms or thereabouts
| * with the theme variation `styleProperty` properties. | ||
| */ | ||
| const clonedBaseVariation = cloneDeep( baseVariation ); | ||
| processedStyleVariations = processedStyleVariations.map( |
There was a problem hiding this comment.
To check with @scruffian if this achieves the original function's intention:
I'm not "removing" the property from the base variation, only allowing it to be overwritten.
...ges/edit-site/src/hooks/use-theme-style-variations/use-theme-style-variations-by-property.js
Show resolved
Hide resolved
ramonjd
requested review from
scruffian,
noisysocks,
apeatling,
tellthemachines and
andrewserong
|
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. |
| Object.keys( object ).forEach( ( key ) => { | ||
| if ( key === property ) { | ||
| newObject[ key ] = object[ key ]; | ||
| } else if ( typeof object[ key ] === 'object' ) { |
There was a problem hiding this comment.
Do we need to guard against object[ key ] being null here, too? Or do a truthy check + typeof object[ key ] === 'object' ?
There was a problem hiding this comment.
Good point. Wouldn't hurt, I've only tested with object keys so far, e.g., typography, layout etc
There was a problem hiding this comment.
Actually, it's only checking for existing keys in the object:
Object.keys( object ).forEach( ( key )
There was a problem hiding this comment.
Oh, sorry, it's a little nuanced this one, so I might've gotten it wrong. When we're looking at object[ key ], key is always a value, but object[ key ] could be a null value, and null unfortunately evaluates to an object type in the typeof object[ key ] === 'object' check.
For a more concrete example, what happens if there is a legal value for a property in theme.json of null? One example would be settings.spacing.blockGap, though it mightn't be a particularly useful example 😄
There was a problem hiding this comment.
if there is a legal value for a property in theme.json of null? One example would be settings.spacing.blockGap
I'm pretty sure it'll be assigned whatever the value, so long as the property keys match.
if ( key === property ) {
newObject[ key ] = object[ key ];I'll add a test to cover null as well. I thought I did but missed it. 👍🏻
There was a problem hiding this comment.
Okay, great. Thanks for confirming!
There was a problem hiding this comment.
I appreciate the line of inquiry!!! 🙇🏻
| */ | ||
| export const filterObjectByProperty = ( object, property ) => { | ||
| if ( ! object ) { | ||
| return {}; |
There was a problem hiding this comment.
Should this be returning a new object, or return object as is? Apologies if this is already covered!
Edit: oh wait, I see further down you're calling Object.keys(), so if this was updated to reflect object back as-is, then there'd need to be another check there 🤔
There was a problem hiding this comment.
Probably object so the recursion works.
I'm adding unit tests for this function so we can check these scenarios.
Thank you for checking it out! 🙇🏻
There was a problem hiding this comment.
Probably object so the recursion works.
I'm adding unit tests for this function so we can check these scenarios.
Thanks! The main thing to check is that we don't accidentally coerce any falsy leaves in a tree to empty object values 🙂
I don't think that's happening, but just thought I'd check!
There was a problem hiding this comment.
Nice work @ramonjd, this is looking very cool!
A couple of questions from reading through the code:
filterObjectByProperty recursively filters a nested object, but I wasn't too clear on the expected behaviour as we go down a tree. A few questions:
- How deep is the filtering meant to go? It sounds like it'll return any branch if anywhere along its branch there is a
propertythat matches. If so, can we update the doc comment to clarify, as it's a slightly unusual kind of object filtering, as it's a bit different to picking just one property out of an object. (Very cool feature, but it's quite specific). - Is
filterObjectByPropertycopying everything where it needs to? Currently a key in the new object is being set to a key in the old object, or to the result of callingfilterObjectByProperty... from reading through, that sounds like it'll always be new objects all the way down the tree, except for when it hits a match onpropertywhere it's then pointing to a reference in the old object? Please forgive me if I'm overthinking this! But my main question was if it matters if the returned object fromfilterObjectByPropertyisn't guaranteed to be a true clone. If it is a problem, then I suppose we could ensure thatcloneDeepis called before the first call tofirstObjectByProperty.
I'm not sure how important the above point is, but just thought I'd ask, as it's sometimes a bit more convenient to ask the esoteric questions up-front than when we're debugging after the fact!
One last question: the function currently supports only filtering by a single property. Is that the main use case, or would there be times when folks would want to grab two things at once? I suppose in that case, they could do two separate calls?
...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
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
Pulling out the utils from #56622 so we test separately
Removes extra functions - now internal to the hook
…eme-style-variations-by-property.js Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
ramonjd
force-pushed
the
try/global-styles-theme-variation-filters
branch
from
89f403c
to
ee36d45
Compare
andrewserong
approved these changes
Feb 13, 2024
There was a problem hiding this comment.
Thanks for all the back-and-forth here! This is looking good to me, and thanks for getting me up to scratch with understanding the intent 👍
There's nice test coverage, and the hook isn't being used anywhere yet and isn't being exported, so this looks like a great place to start to me, and we can always tweak further in follow-ups. LGTM! ✨
|
Thanks for the 👀 @andrewserong and for probing around the object filtering.
There's no limit to the depth. It'll dig down into, for example, I'll update the docs as suggested, thank you!
The unit tests suggest "yes", but I think we should write some more tests around this. Running
That's the main case as far as I'm aware. This PR has really been driven by #56622 I did consider making |
Sounds good to me. The scope in this PR feels like a great start, and it'd be easy enough to change in follow-ups as needs change, too. |
aristath
added a commit
that referenced
this pull request
Feb 13, 2024
* trunk: (273 commits) Remove preffered style variations legacy support (#58930) Style theme variations: add property extraction and merge utils (#58803) Migrate `change-detection` to Playwright (#58767) Update Changelog for 17.6.6 Docs: Clarify the status of the wp-block-styles theme support, and its intent (#58915) Use `data_wp_context` helper in core blocks and remove `data-wp-interactive` object (#58943) Try double enter for details block. (#58903) Template revisions API: move from experimental to compat/6.4 (#58920) Editor: Remove inline toolbar preference (#58945) Clean up link control CSS. (#58934) Font Library: Show error message when no fonts found to install (#58914) Block Bindings: lock editing of blocks by default (#58787) Editor: Remove the 'all' rendering mode (#58935) Pagination Numbers: Add `data-wp-key` to pagination numbers if enhanced pagination is enabled (#58189) Close link preview if collapsed selection when creating link (#58896) Fix incorrect useAnchor positioning when switching from virtual to rich text elements (#58900) Upgrade Floating UI packages, fix nested iframe positioning bug (#58932) Site editor: fix start patterns store selector (#58813) Revert "Rich text: pad multiple spaces through en/em replacement (#56341)" (#58792) Documentation: Clarify the performance reference commit and how to pick it (#58927) ...
ramonjd
added a commit
that referenced
this pull request
Feb 14, 2024
…serConfig()` Replace usage with utils created in #58803
ramonjd
added a commit
that referenced
this pull request
Feb 19, 2024
…serConfig()` Replace usage with utils created in #58803
scruffian
pushed a commit
that referenced
this pull request
Feb 22, 2024
…serConfig()` Replace usage with utils created in #58803
scruffian
pushed a commit
that referenced
this pull request
Feb 27, 2024
…serConfig()` Replace usage with utils created in #58803
scruffian
added a commit
that referenced
this pull request
Feb 28, 2024
* Add colors and typography to the styles tab * refactor * refactor * Add font family names * add heading and body previews * use palettes * make typograpgy a grid * make the styles look nicer * move component to its own file * move color and type variations to different locations in the UI * revert * revert * revert * revert * code dedupe * Add typesets * reorg typogrpahy * tidy up * tidy up code * restyle color palettes * restyle color palettes * remove typesets * add padding back * refactor * Show theme_name + style where there is no typography font family info. Remove unused file. * Consolidating logic into a hook phase 1 * 2022 borks getFamilyPreviewStyle because `face.fontStyle` doesn't exist * comments * Filtering results * Create new convenience hook `useCurrentMergeThemeStyleVariationsWithUserConfig()` Replace usage with utils created in #58803 * add a todo * A bit of clean up. Remove comments, adjust margins. * move variation to a new component * make variation a composible component * use the variation component for typography * Update typography preview * Add animation * move variation to a component * move preset to the higher level * Remove unneeded CSS * updawte the design * remove unsed css * Ensuring the cursor for the color previews is a pointer, which is the same as the typography preview Reducing the vheight of the color panel to match typography Some light refactoring * Reinstate removing property from user config before merging with variation. We do this in order that any user config changes with the same property don't "bleed" into variations. The variation should be "pure". For example, if the user has defined a background color, but the variation has not, we shouldn't merge the two and allow the user background color to persist. Rather the variation's color properties should take the place of all color properties. * i18n for letters * Refactor unique font calculation * move color and type variations higher * Use same box shadow as ColorIndicator so that there's a border around colors This is so that theme colors that are the same value as the background are more noticeable --------- Co-authored-by: ramon <ramonjd@gmail.com>
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 PR extracts and refactors utils from:
The motivation is to provide a hook that:
So, for example:
Why?
See:
Testing Instructions
Run the tests.
npm run test:unit packages/edit-site/src/hooks/use-theme-style-variations/test/use-theme-style-variations-by-property.js