Block Variations: Have getActiveBlockVariation return variation with highest specificity
#62031
Conversation
ockham
added
[Feature] Block Variations
|
Size Change: +295 B (+0.02%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
ockham
changed the title
ockham
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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
[Type] Enhancement
|
Thanks for the PR! I commented on the Group block variations issue and it seems really related:
This approach still has some shortcomings with the Sorry if I'm focusing more on examples of Query Loop and object attributes, but that's the use case I've discussed/explored most.. |
No, that's fine! It's actually helpful to see other use cases that I hadn't taken into account 🙂
So basically a "silent" attribute that's used to differentiate variations, but doesn't have any other effect on the block? During our explorations related to #43743, @tjcafferkey and I considered adding a Now while this PR cannot detect the active variation in the situation you're describing, I believe it would cover the specific use case described in #41303 (if we also implement the idea in #62068) and AFAICS, there wouldn't be any adverse side effects. Do you think it's viable to land those two things before we consider adding a "silent" attribute (like the |
ockham
force-pushed
the
fix/get-active-block-variation-return-match-with-highest-specificity
branch
2 times, most recently
from
e8bd468
to
58ce75c
Compare
|
|
||
| ```js | ||
| isActive: [ 'providerNameSlug' ] | ||
| isActive: [ 'query.postType' ] |
There was a problem hiding this comment.
@justintadlock and @bacoords, I think you will like these change in relation to Block Bindings 😄
There was a problem hiding this comment.
This is a fantastic PR overall. 🙌
…highest specificity
ntsekouras
force-pushed
the
fix/get-active-block-variation-return-match-with-highest-specificity
branch
from
58ce75c
to
ec6a1c6
Compare
| } ); | ||
|
|
||
| return match; | ||
| let candidate = [ undefined, 0 ]; |
There was a problem hiding this comment.
This part of the code is hard to follow because of the tuple-like array. However, it works as expected.
| @@ -237,10 +237,17 @@ export const getBlockVariations = createSelector( | |||
| export function getActiveBlockVariation( state, blockName, attributes, scope ) { | |||
| const variations = getBlockVariations( state, blockName, scope ); | |||
|
|
|||
| const match = variations?.find( ( variation ) => { | |||
| if ( ! variations ) { | |||
There was a problem hiding this comment.
I wasn't sure about this like but getBlockVariations returns a non-empty array or undefined after converting the array into the return value.
|
I'm seeing some small inconsistencies after inserting a variation. It gets recognized correctly upon insertion, but after reloading the page, it no longer can be recognized as a variation. Maybe the comparison is based on the object reference which is the same upon insertion but will differ after the page reloads? Screen.Recording.2024-05-31.at.15.00.59.movThis is the modification that I applied to the Paragraph block: diff --git a/packages/block-library/src/paragraph/block.json b/packages/block-library/src/paragraph/block.json
index 6f11baf838..8bd350fb1e 100644
--- a/packages/block-library/src/paragraph/block.json
+++ b/packages/block-library/src/paragraph/block.json
@@ -70,5 +70,25 @@
}
},
"editorStyle": "wp-block-paragraph-editor",
- "style": "wp-block-paragraph"
+ "style": "wp-block-paragraph",
+ "variations": [
+ {
+ "name": "warning",
+ "title": "Warning",
+ "icon": "warning",
+ "attributes": {
+ "metadata": {
+ "bindings": {
+ "content": {
+ "source": "core/post-meta",
+ "args": {
+ "key": "text_field"
+ }
+ }
+ }
+ }
+ },
+ "isActive": [ "metadata.bindings.content" ]
+ }
+ ]
} |
|
|
||
| The function version of this property accepts a block instance's `blockAttributes` as the first argument, and the `variationAttributes` declared for a variation as the second argument. These arguments can be used to determine if a variation is active by comparing them and returning a `true` or `false` (indicating whether this variation is inactive for this block instance). | ||
| The `string[]` version is used to declare which of the block instance's attributes should be compared to the given variation's. Each attribute will be checked and the variation will be active if all of them match. |
There was a problem hiding this comment.
| The `string[]` version is used to declare which of the block instance's attributes should be compared to the given variation's. Each attribute will be checked and the variation will be active if all of them match. | |
| The `string[]` version is used to declare which of the block instance's attributes should be compared to each variation's attributes. Each attribute will be checked and the variation will be active if all of them match. |
| ``` | ||
|
|
||
| Nested object paths are also supported. For example, consider a block variation that has a `query` object as an attribute. It is possible to determine if the variation is active solely based on that object's `postType` property (while ignoring all its other properties): | ||
| The function version of this property accepts a block instance's `blockAttributes` as the first argument, and the `variationAttributes` declared for a variation as the second argument. These arguments can be used to determine if a variation is active by comparing them and returning a `true` or `false` (indicating whether this variation is inactive for this block instance). |
There was a problem hiding this comment.
| The function version of this property accepts a block instance's `blockAttributes` as the first argument, and the `variationAttributes` declared for a variation as the second argument. These arguments can be used to determine if a variation is active by comparing them and returning a `true` or `false` (indicating whether this variation is inactive for this block instance). | |
| The function version of this property accepts a block instance's `blockAttributes` as the first argument, and the `variationAttributes` declared for a variation as the second argument. These arguments can be used to determine if a variation is active by comparing them and returning `true` or `false` (indicating whether this variation is inactive for this block instance). |
|
|
||
| The `isActive` property can return false positives if multiple variations exist for a specific block and the `isActive` checks are not specific enough. To demonstrate this, consider the following example: | ||
| If there are multiple variations whose `isActive` check matches a given block instance, and all of them are string arrays, then the variation with the highest _specificity_ will be chosen. Consider the following example: |
There was a problem hiding this comment.
Nit: Maybe make bold the all of them?
|
I'm trying to test this variation added to the Paragraph block: "variations": [
{
"name": "warning-with-color",
"title": "Warning with color",
"icon": "warning",
"attributes": {
"content": "This is a warning message.",
"backgroundColor": "luminous-vivid-amber"
},
"isActive": [ "content", "backgroundColor" ]
}
]It isn't recognized as active for some reason I don't fully understand for both "isActive": [ "backgroundColor" ]but not "isActive": [ "content" ]In effect, I think we need to fix it separately for attributes represented as RichText objects and cast them to string values when comparing. |
There was a problem hiding this comment.
Very excited about this functionality, we will have to follow-up with some small fixes to ensure that objects are compared by value, and RichText value properly casted to string value. The same issues existed before so it isn't directly related to this improved functionality but rather related to the way I'm testing it.
|
Hope you don't mind, but pushed a commit that I think it simplifies things a bit and also keeps the best match if a variation has defined an |
|
@ntsekouras, feel free to land it when CI passes so we have it ready on time for WP 6.6 Beta 1 😄 |
ntsekouras
deleted the
fix/get-active-block-variation-return-match-with-highest-specificity
branch
I wanted to report back on my findings. I did some more testing, and everything works as in "isActive": [
"backgroundColor",
"metadata.bindings.content.source",
"metadata.bindings.content.args.key"
]This is how it looks in action for a few variations correctly defined: Screen.Recording.2024-05-31.at.16.49.19.mov |
|
Thank you both for approving and landing this! 🎉
@ntsekouras I was initially considering a similar approach but ultimately decided against. IIUC, your change returns the best It seemed to me that we couldn't make that assumption, as an Granted, it is probably an edge case to have a mix of |
|
@gziolo I'll file issues and/or PRs to address object and RichText comparison in the next couple of days! |
…h highest specificity (WordPress#62031) Co-authored-by: ockham <bernhard-reiter@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: ndiego <ndiego@git.wordpress.org>
…h highest specificity (WordPress#62031) Co-authored-by: ockham <bernhard-reiter@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: ndiego <ndiego@git.wordpress.org>
What?
Change
getActiveBlockVariationsuch that if multiple variations'isActivearrays match the given block's attributes, the variation with the highest specificity (i.e. matching the largest number of attributes) is returned.Why?
See the "Caveats to using isActive" section of the Block Variation docs.
Fixes part of #41303 (see #41303 (comment)).
How?
By assigning a specificity score to each match, corresponding to the number of attributes specified by
isActive, and returning the match with the highest specificity. Take the example from the docs:The specificity (i.e.
isActive.length) of theparagraph-redvariation is1, whereas for theparagraph-red-greyvariation, it is 2. If the current block matches both variations, the one with the higher specificity (here:paragraph-red-grey) is returned.Note that if any matching variation uses a function rather than an array for its
isActivefield, a specificity is not assigned; as a result, the match cannot be compared to the other matches. In this case, the first matching variation will be returned instead (much likegetActiveBlockVariationdid anyway, prior to this PR). (See #62068 for a related issue; fixing it should make it possible to use arrays rather than functions for in most cases.)Testing Instructions
Do some some testing with block variations in the editor.
Other than that, refer to automated tests.