Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block API: Allow "array of attributes to be compared" for isActive property in block variations #30913

Merged
merged 19 commits into from
Apr 22, 2021
Merged

Block API: Allow "array of attributes to be compared" for isActive property in block variations #30913

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
e732e8a
Add selector
david-szabo97 Apr 16, 2021
a6ba309
Replace usages
david-szabo97 Apr 16, 2021
456263c
Add tests
david-szabo97 Apr 16, 2021
2f84704
Add doc
david-szabo97 Apr 16, 2021
dc2fb96
Revert unwanted changes
david-szabo97 Apr 16, 2021
0ed0178
Remove docs
david-szabo97 Apr 19, 2021
9210ab4
Fix arguments order in function calls
david-szabo97 Apr 19, 2021
c5fcb45
Update doc
david-szabo97 Apr 19, 2021
bad88e0
Use select instead of creating a wrapper function
david-szabo97 Apr 19, 2021
f82dc48
Make it stable
david-szabo97 Apr 19, 2021
bcc9124
Filter undefined attributes
david-szabo97 Apr 19, 2021
e836034
Use test.each
david-szabo97 Apr 20, 2021
66af876
Fix incorrect return type
david-szabo97 Apr 21, 2021
637b62b
Validate variations on register
david-szabo97 Apr 21, 2021
66c4201
Update WPBlockVariation type
david-szabo97 Apr 21, 2021
146d392
Update docs
david-szabo97 Apr 21, 2021
0e2c363
Update docs
david-szabo97 Apr 21, 2021
1ca52fc
Revert "Validate variations on register"
david-szabo97 Apr 21, 2021
21328a9
Bail out if no attributes to compare
david-szabo97 Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions packages/blocks/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,19 @@ export function getActiveBlockVariation( state, blockName, attributes, scope ) {
const variations = getBlockVariations( state, blockName, scope );

const match = variations?.find( ( variation ) => {
if ( typeof variation.isActive === 'function' ) {
return variation.isActive( attributes, variation.attributes );
}

if ( Array.isArray( variation.isActive ) ) {
david-szabo97 marked this conversation as resolved.
Show resolved Hide resolved
return variation.isActive.every(
( attribute ) =>
attributes[ attribute ] ===
variation.attributes[ attribute ]
);
const blockType = getBlockType( state, blockName );
const attributeKeys = Object.keys( blockType.attributes || {} );
return variation.isActive
.filter( ( attribute ) => attributeKeys.includes( attribute ) )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Would it be worth validating this earlier during block registration? It might be nice for folks to know that something isn't quite right when developing blocks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that's what I had in mind too in my first comment. This way we will avoid calling this check so many times.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! First iteration is here: 637b62b

.every(
david-szabo97 marked this conversation as resolved.
Show resolved Hide resolved
( attribute ) =>
attributes[ attribute ] ===
variation.attributes[ attribute ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll still want to return false if either of these values is undefined. Blocks and variations may have optional attributes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't come up with a case for this 🤔 It seems to be working as expected:

  1. If both of them are undefined, then that's correct and should match.
  2. If variation.attributes[ attribute ] = undefined and attributes[ attribute ] != undefined then it won't match
  3. If variation.attributes[ attribute ] != undefined and attributes[ attribute ] = undefined then it won't match

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwwar can you expand on the use case you have in mind with an example?

Copy link
Contributor

@gwwar gwwar Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed 👀 that we were already iterating over variations instead of all possible block attributes, so only the left side may be undefined, eg attributes[ attribute ]. Code here is fine 👍

btw I personally find that code reuse in tests isn't that useful. What's nicer for me is being able to read a single test case without needing to skip to different definitions. (Fixtures/snapshots can also help when things go crazy). Curious if folks had thoughts on that.

Here's a free test case, where I verified this:

it( 'should handle optional attributes', () => {
				const testLinkName = 'test/link-block';
				const state = {
					blockTypes: {
						[ testLinkName ]: {
							name: testLinkName,
							attributes: {
								url: { type: 'string' },
								type: { type: 'string' },
								id: { type: 'number' },
							},
						},
					},
					blockVariations: {
						[ testLinkName ]: [
							{
								attributes: { url: 'https://wordpress.org' },
								isActive: [ 'url' ],
							},
							{
								attributes: { type: 'foo' },
								isActive: [ 'type' ],
							},
							{
								attributes: {
									type: 'bar',
									url: 'https://example.com',
								},
								isActive: [ 'type', 'url' ],
							},
						],
					},
				};

				expect(
					getActiveBlockVariation( state, testLinkName, {
						type: 'foo',
					} )
				).toEqual( {
					attributes: { type: 'foo' },
					isActive: [ 'type' ],
				} );

				expect(
					getActiveBlockVariation( state, testLinkName, {
						url: 'https://wordpress.org',
					} )
				).toEqual( {
					attributes: { url: 'https://wordpress.org' },
					isActive: [ 'url' ],
				} );

				expect(
					getActiveBlockVariation( state, testLinkName, {
						type: 'bar',
						url: 'https://example.com',
					} )
				).toEqual( {
					attributes: {
						type: 'bar',
						url: 'https://example.com',
					},
					isActive: [ 'type', 'url' ],
				} );

				expect(
					getActiveBlockVariation( state, testLinkName, {
						id: 1234,
					} )
				).toEqual( undefined );
			} );

Copy link
Member Author

@david-szabo97 david-szabo97 Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had that in mind too. 😄 That's why the last tests are totally separate. Definitely needs some cleaning. A little bit of reuse doesn't hurt though 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit of reuse doesn't hurt though

Certainly, I wanted to leave a note since I was having a bit of trouble reading what the state shape was without inspecting tests.

);
}

return false;
return variation.isActive?.( attributes, variation.attributes );
} );

return match;
Expand Down
88 changes: 85 additions & 3 deletions packages/blocks/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,14 @@ describe( 'selectors', () => {
} );
} );
describe( 'getActiveBlockVariation', () => {
const blockTypeWithTestAttributes = {
name: 'block/name',
attributes: {
testAttribute: {},
firstTestAttribute: {},
secondTestAttribute: {},
},
};
const FIRST_VARIATION_TEST_ATTRIBUTE_VALUE = 1;
const SECOND_VARIATION_TEST_ATTRIBUTE_VALUE = 2;
const UNUSED_TEST_ATTRIBUTE_VALUE = 5555;
Expand Down Expand Up @@ -320,12 +328,21 @@ describe( 'selectors', () => {
},
isActive: [ 'testAttribute' ],
};
const stateFunction = createBlockVariationsState( [
const createBlockVariationsStateWithTestBlockType = (
variations
) =>
deepFreeze( {
...createBlockVariationsState( variations ),
blockTypes: {
[ blockTypeWithTestAttributes.name ]: blockTypeWithTestAttributes,
},
} );
const stateFunction = createBlockVariationsStateWithTestBlockType( [
firstActiveBlockVariationFunction,
secondActiveBlockVariationFunction,
thirdBlockVariation,
] );
const stateArray = createBlockVariationsState( [
const stateArray = createBlockVariationsStateWithTestBlockType( [
firstActiveBlockVariationArray,
secondActiveBlockVariationArray,
thirdBlockVariation,
Expand Down Expand Up @@ -416,7 +433,9 @@ describe( 'selectors', () => {
},
];

const state = createBlockVariationsState( variations );
const state = createBlockVariationsStateWithTestBlockType(
variations
);

expect(
getActiveBlockVariation( state, blockName, {
Expand All @@ -437,6 +456,69 @@ describe( 'selectors', () => {
} )
).toEqual( variations[ 2 ] );
} );
it( 'should ignore attributes that are not defined in the block type', () => {
const variations = [
{
name: 'variation-1',
attributes: {
firstTestAttribute: 1,
secondTestAttribute: 10,
undefinedTestAttribute: 100,
},
isActive: [
'firstTestAttribute',
'secondTestAttribute',
'undefinedTestAttribute',
],
},
{
name: 'variation-2',
attributes: {
firstTestAttribute: 2,
secondTestAttribute: 20,
undefinedTestAttribute: 200,
},
isActive: [
'firstTestAttribute',
'secondTestAttribute',
'undefinedTestAttribute',
],
},
];

const state = createBlockVariationsStateWithTestBlockType(
variations
);

expect(
getActiveBlockVariation( state, blockName, {
firstTestAttribute: 1,
secondTestAttribute: 10,
undefinedTestAttribute: 100,
} )
).toEqual( variations[ 0 ] );
expect(
getActiveBlockVariation( state, blockName, {
firstTestAttribute: 1,
secondTestAttribute: 10,
undefinedTestAttribute: 1234,
} )
).toEqual( variations[ 0 ] );
expect(
getActiveBlockVariation( state, blockName, {
firstTestAttribute: 2,
secondTestAttribute: 20,
undefinedTestAttribute: 200,
} )
).toEqual( variations[ 1 ] );
expect(
getActiveBlockVariation( state, blockName, {
firstTestAttribute: 2,
secondTestAttribute: 20,
undefinedTestAttribute: 2345,
} )
).toEqual( variations[ 1 ] );
} );
} );
describe( 'getDefaultBlockVariation', () => {
it( 'should return the default variation when set', () => {
Expand Down