Block Variations: Compare objects based on given properties #62272
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
ockham
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. |
ockham
added
Backport to WP 6.7 Beta/RC
|
Size Change: +453 B (+0.03%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
fix/get-active-block-variation-object-comparison
branch
from
8fcc7a6
to
3633a2b
Compare
|
Flaky tests detected in 3633a2b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9368609354
|
ntsekouras
reviewed
Jun 5, 2024
packages/blocks/src/store/utils.js
Outdated
| @@ -18,3 +18,15 @@ export const getValueFromObjectPath = ( object, path, defaultValue ) => { | |||
| } ); | |||
| return value ?? defaultValue; | |||
| }; | |||
|
|
|||
| export function matchesAttributes( blockAttributes, variation ) { | |||
| return Object.entries( variation ).every( ( [ key, value ] ) => { | |||
There was a problem hiding this comment.
I'm wondering if we have a util for this already with all the removal of lodash work.. --cc @tyxla
Maybe isShallowEqual? Additionally it seem we would have the same issue with arrays.
There was a problem hiding this comment.
There was a problem hiding this comment.
(isShallowEqual isn't quite the right fit: We only need the given block to match all the variation object attribute's properties, but it's okay if the block's object attribute has additional properties. E.g. in the example from the PR description, if the given block's metadata.bindings.content object has an additional label property, the variation should still match it.)
There was a problem hiding this comment.
Additionally it seem we would have the same issue with
arrays.
That's a fair point, but I'd rather tackle that separately. It's a bit less clear to me what the desired behavior should be for them, so I think it deserves a bit of dedicated discussion. If a variation has
"attributes": {
"arrayAttr": [ "a", "b", "c" ]
},
"isActive": [ "arrayAttr" ]then which of the following blocks should we match?
[ "a", "b", "c", "d" ] // Variation contains a subset[ "b", "c", "a" ] // Variation matches array items but not array order
There was a problem hiding this comment.
Just to close the loop, based on the need @ockham described, I agree that it's best if we reintroduce that util.
cbravobernal
reviewed
Jun 5, 2024
ockham
commented
Jun 5, 2024
tyxla
reviewed
Jun 5, 2024
packages/blocks/src/store/utils.js
Outdated
Comment on lines
34
to
40
| return Object.entries( variationAttributes ).every( ( [ key, value ] ) => { | ||
| if ( | ||
| typeof value === 'object' && | ||
| typeof blockAttributes[ key ] === 'object' | ||
| ) { | ||
| return matchesAttributes( blockAttributes[ key ], value ); |
There was a problem hiding this comment.
Should we add an exception for null? typeof null is object, but if both value and blockAttributes[ key ] are null, we'll call Object.entries( null ) which will be a TypeError.
There was a problem hiding this comment.
Yes, good spot! I added a check in b69dc0a which I think should be sufficient.
gziolo
reviewed
Jun 5, 2024
| ); | ||
| // If the attribute value is an object, we need to compare its properties. |
There was a problem hiding this comment.
How far is it from isShallowEqual from @wordpress/is-shallow-equal at this point?
gutenberg/packages/is-shallow-equal/src/index.js
Lines 23 to 33 in 06470bb
ockham
force-pushed
the
fix/get-active-block-variation-object-comparison
branch
from
b69dc0a
to
e378dad
Compare
|
@cbravobernal pointed out to me that the example in the PR desc is currently not working (thank you!)
Edit: Ignore, I just missed the
|
ockham
force-pushed
the
fix/get-active-block-variation-object-comparison
branch
from
e378dad
to
75d61fc
Compare
I think I tracked down the error and fixed it (see the last couple of commits). |
|
Checking in on PRs with the Backport to Gutenberg RC. I'll release 18.5 later today, do you think this'll make it in time? |
Depends on whether we'll get approval in time 🤞 |
cbravobernal
approved these changes
Jun 5, 2024
cbravobernal
reviewed
Jun 5, 2024
| @@ -19,6 +19,14 @@ export const getValueFromObjectPath = ( object, path, defaultValue ) => { | |||
| return value ?? defaultValue; | |||
| }; | |||
|
|
|||
| function isObject( candidate ) { | |||
| return ( | |||
| typeof candidate === 'object' && | |||
There was a problem hiding this comment.
We called it item in Interactivity. I like this nomenclature, maybe we should update to keep consistency in the codebase. Ping @DAreRodz @luisherranz
There was a problem hiding this comment.
I need more context. What exactly are we referring to as "item" in the Interactivity API? What is being referred to here?
There was a problem hiding this comment.
In Interactivity API, we check that several "items" are a JSON object in the same file (sources, state, config, proxy handlers, etc). So we call the variable to check "item".
"Candidate" seems also a good general definition, and would be ok to use the same name in all functions that are just checking that an item is an object (there are a few of them across the workspace).
There was a problem hiding this comment.
Oh, ok. You can change that if you want 🙂
vcanales
pushed a commit
that referenced
this pull request
Jun 5, 2024
If a block variation has an isActive property that is an array of strings (attribute names), and one of the attributes referenced therein is an object, compare that attribute based on the object properties specified by the variation, rather than by reference. Co-authored-by: ockham <bernhard-reiter@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org
|
I just cherry-picked this PR to the release/18.5 branch to get it included in the next release: 2f2677a |
vcanales
removed
the
Backport to Gutenberg RC
ellatrix
pushed a commit
that referenced
this pull request
Jun 11, 2024
If a block variation has an isActive property that is an array of strings (attribute names), and one of the attributes referenced therein is an object, compare that attribute based on the object properties specified by the variation, rather than by reference. Co-authored-by: ockham <bernhard-reiter@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org
ellatrix
pushed a commit
that referenced
this pull request
Jun 11, 2024
If a block variation has an isActive property that is an array of strings (attribute names), and one of the attributes referenced therein is an object, compare that attribute based on the object properties specified by the variation, rather than by reference. Co-authored-by: ockham <bernhard-reiter@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org
6 tasks
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…s#62272) If a block variation has an isActive property that is an array of strings (attribute names), and one of the attributes referenced therein is an object, compare that attribute based on the object properties specified by the variation, rather than by reference. Co-authored-by: ockham <bernhard-reiter@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org
ellatrix
removed
the
Backport to WP 6.7 Beta/RC
|
This was cherry-picked to the wp/6.6 branch. |
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?
If a block variation has an
isActiveproperty that is an array of strings (attribute names), and one of the attributes referenced therein is an object, compare that attribute based on the object properties specified by the variation, rather than by reference.Why?
Object attributes of a variation are currently compared by reference to the given block's attributes, meaning that the comparison will never return
true.For example, take the following variation of the Paragraph block:
Prior to this PR,
isActivewould returnfalsefor the given variation, because it would compare the given block'smetadata.bindings.content(nested) attribute by object reference to the variation's.How?
By (recursively) iterating over the object properties specified by the variation and comparing those to the given block's. (In the example above, those properties are
sourceandargs; sinceargsis an object itself, the comparison steps into it recursively and compares itskeyproperty to the given block's as well).Note that this PR re-introduces the
matchesAttributesutil function (albeit in a different place) that was previously removed by #58194.Testing Instructions
Smoke-test block variations and verify that they still work as before.
Optionally, try adding the above variation to the Paragraph block, and verify that the variation is correctly detected (thus fixing the issue reported in this comment).