From ec6a1c6e0b0f08fbab9035c14c0ca71ee339238e Mon Sep 17 00:00:00 2001 From: Bernie Reiter Date: Thu, 30 May 2024 16:20:18 +0200 Subject: [PATCH 1/2] Block Variations: Have getActiveBlockVariation return variation with highest specificity --- .../block-api/block-variations.md | 30 ++-- packages/blocks/src/store/selectors.js | 71 +++++++--- packages/blocks/src/store/test/selectors.js | 131 ++++++++++++++++++ 3 files changed, 198 insertions(+), 34 deletions(-) diff --git a/docs/reference-guides/block-api/block-variations.md b/docs/reference-guides/block-api/block-variations.md index 0790b7f02641e..ffd3cc49adda8 100644 --- a/docs/reference-guides/block-api/block-variations.md +++ b/docs/reference-guides/block-api/block-variations.md @@ -129,9 +129,9 @@ While the `isActive` property is optional, it's recommended. This API is used by If `isActive` is not set, the Editor cannot distinguish between an instance of the original block and your variation, so the original block information will be displayed. -The property can be set to either a function or an array of strings (`string[]`). +The property can be set to either an array of strings (`string[]`), or a function. It is recommended to use the string array version whenever possible. -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. As an example, in the core Embed block, the `providerNameSlug` attribute is used to determine the embed provider (e.g. 'youtube' or 'twitter'). The variations may be declared like this: @@ -162,28 +162,32 @@ const variations = [ ] ``` - The `isActive` function can compare the block instance value for `providerNameSlug` to the value declared in the variation's declaration (the values in the code snippet above) to determine which embed variation is active: +The `isActive` property would then look like this: ```js -isActive: ( blockAttributes, variationAttributes ) => - blockAttributes.providerNameSlug === variationAttributes.providerNameSlug, +isActive: [ 'providerNameSlug' ] ``` -The `string[]` version is used to declare which attributes should be compared as a shorthand. Each attribute will be checked and the variation will be active if all of them match. Using the same example for the embed block, the string version would look like this: +This will cause the block instance value for `providerNameSlug` to be compared to the value declared in the variation's declaration (the values in the code snippet above) to determine which embed variation is active. + +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): ```js -isActive: [ 'providerNameSlug' ] +isActive: [ 'query.postType' ] ``` -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). + +Using the same example for the embed block, the function version would look like this: ```js -isActive: [ 'query.postType' ] +isActive: ( blockAttributes, variationAttributes ) => + blockAttributes.providerNameSlug === variationAttributes.providerNameSlug, ``` -### Caveats to using `isActive` +### Specificity of `isActive` matches -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: ```js wp.blocks.registerBlockVariation( @@ -212,6 +216,6 @@ wp.blocks.registerBlockVariation( ); ``` -The `isActive` check on both variations tests the `textColor`, but each variations uses `vivid-red`. Since the `paragraph-red` variation is registered first, once the `paragraph-red-grey` variation is inserted into the Editor, it will have the title `Red Paragraph` instead of `Red/Grey Paragraph`. As soon as the Editor finds a match, it stops checking. +If a block instance has attributes `textColor: vivid-red` and `backgroundColor: cyan-bluish-gray`, both variations' `isActive` criterion will match that block instance. In this case, the more _specific_ match will be determined to be the active variation, where specificity is calculated as the length of each `isActive` array. This means that the `Red/Grey Paragraph` will be shown as the active variation. -There have been [discussions](https://github.com/WordPress/gutenberg/issues/41303#issuecomment-1526193087) around how the API can be improved, but as of WordPress 6.3, this remains an issue to watch out for. +Note that specificity cannot be determined for a matching variation if its `isActive` property is a function rather than a `string[]`. In this case, the first matching variation will be determined to be the active variation. For this reason, it is generally recommended to use a `string[]` rather than a `function` for the `isActive` property. diff --git a/packages/blocks/src/store/selectors.js b/packages/blocks/src/store/selectors.js index 9b2beb0b8369b..a0633cd7ae212 100644 --- a/packages/blocks/src/store/selectors.js +++ b/packages/blocks/src/store/selectors.js @@ -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 ) { + return variations; + } + + const blockType = getBlockType( state, blockName ); + const attributeKeys = Object.keys( blockType?.attributes || {} ); + + const matches = []; + + for ( const variation of variations ) { if ( Array.isArray( variation.isActive ) ) { - const blockType = getBlockType( state, blockName ); - const attributeKeys = Object.keys( blockType?.attributes || {} ); const definedAttributes = variation.isActive.filter( ( attribute ) => { // We support nested attribute paths, e.g. `layout.type`. @@ -251,27 +258,49 @@ export function getActiveBlockVariation( state, blockName, attributes, scope ) { } ); if ( definedAttributes.length === 0 ) { - return false; + continue; } - return definedAttributes.every( ( attribute ) => { - const attributeValue = getValueFromObjectPath( - attributes, - attribute - ); - if ( attributeValue === undefined ) { - return false; - } - return ( - attributeValue === - getValueFromObjectPath( variation.attributes, attribute ) - ); - } ); + if ( + ! definedAttributes.every( ( attribute ) => { + const attributeValue = getValueFromObjectPath( + attributes, + attribute + ); + if ( attributeValue === undefined ) { + return false; + } + return ( + attributeValue === + getValueFromObjectPath( + variation.attributes, + attribute + ) + ); + } ) + ) { + continue; + } + // We assign a specificity score to each variation based on the number of attributes + // that it matches. + matches.push( [ variation, definedAttributes.length ] ); + } else if ( variation.isActive?.( attributes, variation.attributes ) ) { + // If isActive is a function, we cannot know how many attributes it matches. + // This means that we cannot compare the specificity of our matches, + // and simply return the first match we have found. + if ( matches.length > 0 ) { + return matches[ 0 ][ 0 ]; + } + return variation; } + } - return variation.isActive?.( attributes, variation.attributes ); - } ); - - return match; + let candidate = [ undefined, 0 ]; + for ( const [ variation, specificity ] of matches ) { + if ( specificity > candidate[ 1 ] ) { + candidate = [ variation, specificity ]; + } + } + return candidate?.[ 0 ]; } /** diff --git a/packages/blocks/src/store/test/selectors.js b/packages/blocks/src/store/test/selectors.js index 5e3a56c34cf3b..ccbf665dbb83a 100644 --- a/packages/blocks/src/store/test/selectors.js +++ b/packages/blocks/src/store/test/selectors.js @@ -291,6 +291,7 @@ describe( 'selectors', () => { testAttribute: {}, firstTestAttribute: {}, secondTestAttribute: {}, + thirdTestAttribute: {}, }, }; const FIRST_VARIATION_TEST_ATTRIBUTE_VALUE = 1; @@ -510,6 +511,136 @@ describe( 'selectors', () => { } ) ).toEqual( variations[ 2 ] ); } ); + it( 'should return the active variation using the match with the highest specificity for the given isActive array (multiple values)', () => { + const variations = [ + { + name: 'variation-1', + attributes: { + firstTestAttribute: 1, + secondTestAttribute: 2, + }, + isActive: [ + 'firstTestAttribute', + 'secondTestAttribute', + ], + }, + { + name: 'variation-2', + attributes: { + firstTestAttribute: 1, + secondTestAttribute: 2, + thirdTestAttribute: 3, + }, + isActive: [ + 'firstTestAttribute', + 'secondTestAttribute', + 'thirdTestAttribute', + ], + }, + { + name: 'variation-3', + attributes: { + firstTestAttribute: 1, + thirdTestAttribute: 3, + }, + isActive: [ + 'firstTestAttribute', + 'thirdTestAttribute', + ], + }, + ]; + + const state = + createBlockVariationsStateWithTestBlockType( variations ); + + expect( + getActiveBlockVariation( state, blockName, { + firstTestAttribute: 1, + secondTestAttribute: 2, + } ) + ).toEqual( variations[ 0 ] ); + // All variations match the following attributes. Since all matches have an array for their isActive + // fields, we can compare the specificity of each match and return the most specific match. + expect( + getActiveBlockVariation( state, blockName, { + firstTestAttribute: 1, + secondTestAttribute: 2, + thirdTestAttribute: 3, + } ) + ).toEqual( variations[ 1 ] ); + expect( + getActiveBlockVariation( state, blockName, { + firstTestAttribute: 1, + thirdTestAttribute: 3, + } ) + ).toEqual( variations[ 2 ] ); + } ); + it( 'should return the active variation using the first match given the isActive array (multiple values) and function', () => { + const variations = [ + { + name: 'variation-1', + attributes: { + firstTestAttribute: 1, + secondTestAttribute: 2, + }, + isActive: [ + 'firstTestAttribute', + 'secondTestAttribute', + ], + }, + { + name: 'variation-2', + attributes: { + firstTestAttribute: 1, + secondTestAttribute: 2, + thirdTestAttribute: 3, + }, + isActive: [ + 'firstTestAttribute', + 'secondTestAttribute', + 'thirdTestAttribute', + ], + }, + { + name: 'variation-3', + attributes: { + firstTestAttribute: 1, + thirdTestAttribute: 3, + }, + isActive: ( blockAttributes, variationAttributes ) => + blockAttributes.firstTestAttribute === + variationAttributes.firstTestAttribute && + blockAttributes.thirdTestAttribute === + variationAttributes.thirdTestAttribute, + }, + ]; + + const state = + createBlockVariationsStateWithTestBlockType( variations ); + + expect( + getActiveBlockVariation( state, blockName, { + firstTestAttribute: 1, + secondTestAttribute: 2, + } ) + ).toEqual( variations[ 0 ] ); + // All variations match the following attributes. However, since the third variation has a function + // for its isActive field, we cannot compare the specificity of each match, so instead we return the + // first match. + expect( + getActiveBlockVariation( state, blockName, { + firstTestAttribute: 1, + secondTestAttribute: 2, + thirdTestAttribute: 3, + } ) + ).toEqual( variations[ 0 ] ); + expect( + getActiveBlockVariation( state, blockName, { + firstTestAttribute: 1, + thirdTestAttribute: 3, + } ) + ).toEqual( variations[ 2 ] ); + } ); it( 'should ignore attributes that are not defined in the block type', () => { const variations = [ { From bb2bae0c0e8811f309f1a05e4163936bf7b0d166 Mon Sep 17 00:00:00 2001 From: ntsekouras Date: Fri, 31 May 2024 16:46:43 +0300 Subject: [PATCH 2/2] simplify a bit --- packages/blocks/src/store/selectors.js | 61 ++++++++------------- packages/blocks/src/store/test/selectors.js | 4 +- 2 files changed, 25 insertions(+), 40 deletions(-) diff --git a/packages/blocks/src/store/selectors.js b/packages/blocks/src/store/selectors.js index a0633cd7ae212..ecdc82e763304 100644 --- a/packages/blocks/src/store/selectors.js +++ b/packages/blocks/src/store/selectors.js @@ -243,8 +243,8 @@ export function getActiveBlockVariation( state, blockName, attributes, scope ) { const blockType = getBlockType( state, blockName ); const attributeKeys = Object.keys( blockType?.attributes || {} ); - - const matches = []; + let match; + let maxMatchedAttributes = 0; for ( const variation of variations ) { if ( Array.isArray( variation.isActive ) ) { @@ -257,50 +257,35 @@ export function getActiveBlockVariation( state, blockName, attributes, scope ) { return attributeKeys.includes( topLevelAttribute ); } ); - if ( definedAttributes.length === 0 ) { + const definedAttributesLength = definedAttributes.length; + if ( definedAttributesLength === 0 ) { continue; } - if ( - ! definedAttributes.every( ( attribute ) => { - const attributeValue = getValueFromObjectPath( - attributes, - attribute - ); - if ( attributeValue === undefined ) { - return false; - } - return ( - attributeValue === - getValueFromObjectPath( - variation.attributes, - attribute - ) - ); - } ) - ) { - continue; + const isMatch = definedAttributes.every( ( attribute ) => { + const attributeValue = getValueFromObjectPath( + attributes, + attribute + ); + if ( attributeValue === undefined ) { + return false; + } + return ( + attributeValue === + getValueFromObjectPath( variation.attributes, attribute ) + ); + } ); + if ( isMatch && definedAttributesLength > maxMatchedAttributes ) { + match = variation; + maxMatchedAttributes = definedAttributesLength; } - // We assign a specificity score to each variation based on the number of attributes - // that it matches. - matches.push( [ variation, definedAttributes.length ] ); } else if ( variation.isActive?.( attributes, variation.attributes ) ) { // If isActive is a function, we cannot know how many attributes it matches. // This means that we cannot compare the specificity of our matches, - // and simply return the first match we have found. - if ( matches.length > 0 ) { - return matches[ 0 ][ 0 ]; - } - return variation; - } - } - - let candidate = [ undefined, 0 ]; - for ( const [ variation, specificity ] of matches ) { - if ( specificity > candidate[ 1 ] ) { - candidate = [ variation, specificity ]; + // and simply return the best match we have found. + return match || variation; } } - return candidate?.[ 0 ]; + return match; } /** diff --git a/packages/blocks/src/store/test/selectors.js b/packages/blocks/src/store/test/selectors.js index ccbf665dbb83a..1a6e768724acc 100644 --- a/packages/blocks/src/store/test/selectors.js +++ b/packages/blocks/src/store/test/selectors.js @@ -626,14 +626,14 @@ describe( 'selectors', () => { ).toEqual( variations[ 0 ] ); // All variations match the following attributes. However, since the third variation has a function // for its isActive field, we cannot compare the specificity of each match, so instead we return the - // first match. + // best match we've found. expect( getActiveBlockVariation( state, blockName, { firstTestAttribute: 1, secondTestAttribute: 2, thirdTestAttribute: 3, } ) - ).toEqual( variations[ 0 ] ); + ).toEqual( variations[ 1 ] ); expect( getActiveBlockVariation( state, blockName, { firstTestAttribute: 1,