-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Conversation
Size Change: +4.04 kB (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @david-szabo97! 💯
In general this looks good. I haven't checked the tests you wrote for now, but I will after addressing the main thing about attributes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty close @david-szabo97! I think this should be good to go once PR checks are ✅ and folks' feedback is addressed.
const blockType = getBlockType( state, blockName ); | ||
const attributeKeys = Object.keys( blockType.attributes || {} ); | ||
return variation.isActive | ||
.filter( ( attribute ) => attributeKeys.includes( attribute ) ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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( | ||
( attribute ) => | ||
attributes[ attribute ] === | ||
variation.attributes[ attribute ] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- If both of them are
undefined
, then that's correct and should match. - If
variation.attributes[ attribute ] = undefined
andattributes[ attribute ] != undefined
then it won't match - If
variation.attributes[ attribute ] != undefined
andattributes[ attribute ] = undefined
then it won't match
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 );
} );
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks solid to me @david-szabo97 ✨ Thanks for taking this one on!
I found another area where we should update before landing, which is the type definition in
gutenberg/packages/blocks/src/api/registration.js
Lines 104 to 111 in 3e14c5e
* @property {Function} [isActive] A function that accepts a block's attributes | |
* and the variation's attributes and determines | |
* if a variation is active. This function doesn't | |
* try to find a match dynamically based on all | |
* block's attributes, as in many cases some | |
* attributes are irrelevant. An example would | |
* be for `embed` block where we only care about | |
* `providerNameSlug` attribute's value. |
🤔 Optional/followup: I wonder if there's a way to get this typedef to help generate the block-variations.md. We do have support for autogenerating other examples with <!-- START TOKEN(Autogenerated API docs) -->
and npm run docs:build
@gziolo @ntsekouras were there any other blocking changes needed here?
@gwwar, I will do a cross-check shortly. There is one follow-up that I can think of, it's changing API schema for block types in WordPress core to account for isActive as an array. In addition to that, we should be able to know to define |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @david-szabo97 ! A last change that is needed and let's land.
dispatch( blocksStore ).addBlockTypes( settings ); | ||
|
||
return settings; | ||
} | ||
|
||
function validateVariations( settings ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure where to put this. 😄 I'm also wondering why don't we dispatch( blocksStore ).addBlockVariation( settings.variations )
instead of handling variations here:
...get( blockType, [ 'variations' ], [] ), |
For this reason we need to do the variations validation in two functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract it to another PR? It isn't a blocker really and it requires some more thinking because you can register block variations also through the store directly. I don't remember exactly but most likely they are stored in two places in the store. It's similar to how block styles can be registered, so plugin authors could register styles or variations before the block gets registered so plugins wouldn't have to declare dependencies on each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I didn't know it meant to be that way - registering variation even without an existing block type. This comes with its own challenges 😄
I revert the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double-checked, it's stored in one reducer but you can add variations with ADD_BLOCK_TYPES
or ADD_BLOCK_VARIATIONS
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's 🚢 💯
Instead of a callback, you may pass an array of attributes which are checked against the block to see if this variation is active. WordPress/gutenberg#30913
Fixes #30739
Description
Extends the variations API by adding one more possible way to define how active variations are recognized.
The only way currently in
trunk
is to use anisActive
function
. In this PR we introducearray of strings
forisActive
property.Most of our variations use the following pattern:
gutenberg/packages/block-library/src/embed/variations.js
Lines 347 to 352 in 4b14897
As you can see, we are just creating a function for each variation and check for a specific attribute whether it matches or not. Basically, this is what we do for (almost) all of our variations so far.
To reduce boilerplate code, we can now use an array of strings to do the same check behind the scenes:
Every element in the array represents an attribute to be compared. If each attribute matches then it is considered active. For example:
translates to
How has this been tested?
Tests should pass
Smoke test, make sure all blocks using variations are still working as before
Replace
gutenberg/packages/block-library/src/social-link/variations.js
Line 314 in 4b14897
with
Make sure Social Link block still works the same way as before.
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.js
files for terms that need renaming or removal).