Blocks: Make it more flexible to match the currently applied block variation

[gziolo]

What problem does this address?

It is possible to define multiple variations of the same block. Users can see in the UI a title, description, or icon if the currently applied block variation implements isActive callback and it returns true.

• isActive (optional, type Function) - 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.

It's worth mentioning that setting the isActive property can be useful for cases you want to use information from the block variation, after a block's creation. For example, this API is used in useBlockDisplayInformation hook to fetch and display proper information on places like the BlockCard or Breadcrumbs components.

See more in: https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-variations.md

However, it looks like the majority of current usage loops over the list of variations and dynamically injects the isActive callback. Some examples:

gutenberg/packages/block-library/src/embed/variations.js

Lines 342 to 352 in bbd3333

	/**
	* Add `isActive` function to all `embed` variations, if not defined.
	* `isActive` function is used to find a variation match from a created
	* Block by providing its attributes.
	*/
	variations.forEach( ( variation ) => {
	if ( variation.isActive ) return;
	variation.isActive = ( blockAttributes, variationAttributes ) =>
	blockAttributes.providerNameSlug ===
	variationAttributes.providerNameSlug;
	} );

gutenberg/packages/block-library/src/social-link/variations.js

Lines 307 to 316 in bbd3333

	/**
	* Add `isActive` function to all `social link` variations, if not defined.
	* `isActive` function is used to find a variation match from a created
	* Block by providing its attributes.
	*/
	variations.forEach( ( variation ) => {
	if ( variation.isActive ) return;
	variation.isActive = ( blockAttributes, variationAttributes ) =>
	blockAttributes.service === variationAttributes.service;
	} );

With #29095, it is also possible to register block variations on the server. It is impossible to define isActive in PHP as long as it needs to be represented as a JavaScript callback. It ends up being dynamically injected in JavaScript with the WordPress hook that creates another level of complexity.

gutenberg/packages/block-library/src/navigation-link/hooks.js

Lines 47 to 66 in b620c92

	if ( settings.variations ) {
	const isActive = ( blockAttributes, variationAttributes ) => {
	return blockAttributes.type === variationAttributes.type;
	};
	const variations = settings.variations.map( ( variation ) => {
	return {
	...variation,
	...( ! variation.icon && {
	icon: getIcon( variation.name ),
	} ),
	...( ! variation.isActive && {
	isActive,
	} ),
	};
	} );
	return {
	...settings,
	variations,
	};
	}

What is your proposed solution?

Option 1: isActive allows arrays

This is a simple idea where isActive could be represented as an array in the case where block variations are registered on the server, e.g.:

array(
	'name'     => 'category', 
	'isActive' => array( 'type' ),
	// ... other settings
)`

type is the value of a block attribute in the editor and the type set as an attribute for the variation. You can think of it as the following condition:

isActive = ( blockAttributes, variationAttributes ) => blockAttributes.type === variationAttributes.type; 

Options 2: top-level function on the client

Another option is that we allow defining the top-level API method that returns the name of the currently applied variation. In fact, this is what @ntsekouras originally proposed in #27469 when working on the API, but I convinced him to start with a callback per block variation so we keep the list of top-level settings small.

registerBlockType( 'core/navigation-link', {
	getCurrentlyAplliedVariation: ( blockAttributes, blockVariations ) => {
		const { name } = find( blockVariations,  ( { attributes } ) => blockAttributes.type === attributes.type );
		return name;
 	},
	// ... other settings
} );

We could ensure backward compatibility by giving the higher priority the isActive matches defined on individual block variations.

[gwwar]

One thing I found a bit awkward was needing to define isActive per variation. Do we have an existing use case where we needed a different check on one variation but not another in the list? By default I think most variations in a list would use the same comparison. Since variations is an array, perhaps this was to avoid another top level item in settings?

Option 1: isActive allows arrays

Do you mean that if we're passed an array of [ 'type', 'slug' ], the isActive function would then compare blockAttributes.type === variationAttributes.type && blockAttributes.slug === variationAttributes.slug?

This could definitely work and it keeps logic grouped together. One of the tradeoffs would be that it's a bit limiting if someone needs a custom check. I'm not aware of such a use case yet, so I'm not sure how much we need to worry about this.

Options 2: top-level function on the client

This one also works. One thing to be mindful of is making sure folks understand what the function ties to. These are block variations, but we also have style variations, and I think folks might also mix this up with patterns.

If it's a concern to group things together in settings and keep options minimal have we considered an object instead of an array and another top level function? This makes things a bit more difficult for the server definition, but I was curious if this was considered already. For example:

{
//...
variations: { items:[
    {
		name: 'twitter',
		title: 'Twitter',
		icon: embedTwitterIcon,
		keywords: [ 'tweet', __( 'social' ) ],
		description: __( 'Embed a tweet.' ),
		patterns: [ /^https?:\/\/(www\.)?twitter\.com\/.+/i ],
		attributes: { providerNameSlug: 'twitter', responsive: true },
  },
  //... 
], getActiveVariation: ( blockAttributes, blockVariations ) => { ... }  }`

Sidenote: To fully remove the hooks.js file from navigation-link, 🤔 thinking through icons would be another one. One straightforward approach would be passing a string matching the icon name from the package. Is there any background on why we may have avoided this?

[ntsekouras]

Do you mean that if we're passed an array of [ 'type', 'slug' ], the isActive function would then compare blockAttributes.type === variationAttributes.type && blockAttributes.slug === variationAttributes.slug?

Yes, by providing an array of attributes is proposed to make equality checks for each of them.

One of the tradeoffs would be that it's a bit limiting if someone needs a custom check. I'm not aware of such a use case yet, so I'm not sure how much we need to worry about this.

A custom check is used in Template Parts for now, but still an API that supports two types (array of attributes and function) seems worthy of exploration.

Sidenote: To fully remove the hooks.js file from navigation-link, 🤔 thinking through icons would be another one.

Correct. @gziolo had noted that a layer which translates strings into @wordpress/icons should be provided.

[gziolo]

Sidenote: To fully remove the hooks.js file from navigation-link, 🤔 thinking through icons would be another one.
Correct. @gziolo had noted that a layer which translates strings into @wordpress/icons should be provided.

Yes, it would be great to add a way to be able to pass a string that resolves into a matching icon from @wordpress/icons package. @youknowriad, does it go along with the vision you had for the package? What could be the place where we introducing the mapping layer (at least) for WordPress core blocks so we could put strings in block.json.

Options 2: top-level function on the client
This one also works. One thing to be mindful of is making sure folks understand what the function ties to. These are block variations, but we also have style variations, and I think folks might also mix this up with patterns.

Yes, in general, it seems disconnected when put as a top-level setting. It was one of the reasons I was advocating against it initially.

If it's a concern to group things together in settings and keep options minimal have we considered an object instead of an array and another top level function? This makes things a bit more difficult for the server definition, but I was curious if this was considered already. For example:

I don't remember if we discussed this option, but it's definitely a good option to consider. It doesn't solve the issue with the need for the filter for the Navigation Link block, unless we allow also an array representation as a getActiveVariation matcher in your example shared. It definitely would require some more work to ensure that both old and new formats are supported and well documented.

[gwwar]

If folks don't have strong feelings, I'd say option 1 is likely most viable since it's the least amount of work + would match well if we added a string icon mapping.

[gziolo]

Yes, I agree it is a relatively simple change. We can always do both options discussed 😄

[david-szabo97]

Taking this 😄

[gziolo]

@gwwar and @ntsekouras, should we open a new issue that makes variations setting more flexible? Example:

variations: {
    items,
    getActiveVariation,
},

[ntsekouras]

We could but I don't think that is needed (at least for now) and this involves lots of changes and keeping backwards compatibility. Do you think it would provide value if we implemented this now?

[gziolo]

It's mostly optimization as developers wouldn't have to inject isActive to every block variation if they are the same. It can wait for sure. We have the discussion documented so we can get back to it in the future if necessary 👍🏻

[gwwar]

@gwwar and @ntsekouras, should we open a new issue that makes variations setting more flexible?

@gziolo I think it'd be worthwhile to create a new issue, if it's something we'd still like. (fine to mark as low priority). It can be a decent amount of work for someone to reason about what still needs to be done from a closed issue discussion

[gziolo]

We can keep it closed and see how quickly we run into a blocker that requires changes to the shape of variations. It looks like we should focus more on better handling for icons so we could define them on the server more often.