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.

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

Can't set default Group block variation in WordPress 6.3 #53750

Closed
mrwweb opened this issue Aug 16, 2023 · 8 comments
Closed

Can't set default Group block variation in WordPress 6.3 #53750

mrwweb opened this issue Aug 16, 2023 · 8 comments
Labels
[Feature] Block Variations Block variations, including introducing new variations and variations as a feature Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended

Comments

@mrwweb
Copy link

mrwweb commented Aug 16, 2023

Description

Split off #53517 at @ndiego's suggestion.

Update: I was very confused about the "Inherit layout" setting when writing this post, but the behaviors documented are all accurate. The primary issues are: 1) setting a default variation and 2) variation selectors overriding variation attributes.

In WordPress 6.2 (and prior versions), I could use this code to override the default Group block variation so it "inherits" the content layout (very confusing option, btw):

/* Set Group to Inherit Layout by Default */
/* Stopped working in WordPress 6.3. */
/* Results in a duplicate Group block in the inserter and layout attribute is ignored */
wp.blocks.registerBlockVariation('core/group', {
	isDefault: true,
	attributes: {
		layout: {
			inherit: true,
		},
	},
});

The first problem is that the layout attribute was changed. Now, instead of inherit: true, it needs to be type: 'default'.

However, there is a second bigger problem. That code results in a duplicate Group block in the inserter.

Adding name: 'group' to the code gets rid of the duplicate Group block in the inserter but then the attribute changes are ignored. (So my variation is essentially ignored.)

This doesn't work:

/* Set Group to Inherit Layout by Default */
/* Stopped working in WordPress 6.3 */
/* block variation appears to be completely ignored */
wp.blocks.registerBlockVariation('core/group', {
	name: 'group'
	isDefault: true,
	attributes: {
		layout: {
			inherit: true,
		},
	},
});

I can get the variation to work by unregistering the default group variation first:

/* Set Group to Inherit Layout by Default */
/* Working in 6.3, though it seems isDefault should make unregisterBlockVariation unnecessary */
unregisterBlockVariation( 'core/group', 'group' );
registerBlockVariation(
	'core/group', {
		name: 'group',
		isDefault: true,
		attributes: {
			layout: {
				type: 'default',
			},
		},
	}
);

However, that's still not the end of the problems! Even though that inserts a Group block with the "inherit" option toggle off, when the default Group layout (so not row or column) is selected, the Toggle is automatically set back on. My solution was to bypass the Group block variation selector by passing an empty style object to the variation as well:

/* Set Group to Inherit Layout by Default */
/* Working in 6.3, though it seems isDefault should make unregisterBlockVariation unnecessary */
unregisterBlockVariation( 'core/group', 'group' );
registerBlockVariation(
	'core/group', {
		name: 'group',
		isDefault: true,
		attributes: {
			layout: {
				type: 'default',
			},
			style: {},
		},
	}
);

So I think there are a few questions to answer:

  1. Why can't I set a block variation for the default Group block without unregistering it first?
  2. Is there a less-hacky way to bypass the Group block variation selector state so the layout setting is maintained?
  3. Is it supported to register a block variation without a name? Since it worked, I assumed it was intention because isDefault implies it should override the default variation (though that may be a bad assumption).

Finally a few notes:

  1. I tried this both in ES6 compiled via wp-scripts and in vanilla js with the wp global. I got the same results both times.
  2. I couldn't find any difference based on dependencies listed in the script's enqueue argument, the enqueue_block_assets vs. enqueue_block_editor_assets hooks, using DOMContentLoaded/wp.domReady/nothing.

Step-by-step reproduction instructions

See snippets in description.

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 6.3
  • Custom theme
  • Confirmed with a second developer

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

No

@Mamaduka Mamaduka added Needs Technical Feedback Needs testing from a developer perspective. [Feature] Block Variations Block variations, including introducing new variations and variations as a feature labels Aug 17, 2023
@mrwweb
Copy link
Author

mrwweb commented Aug 17, 2023

I'm running into something similar with the cover block now. Wanting to set that to also default to not have the layout toggle on, I seem to have to unregister the default block variation (which I didn't even think existed?) for this to take effect:

/* Adjust default Cover block */
wp.blocks.unregisterBlockVariation( 'core/cover', 'cover' );
wp.blocks.registerBlockVariation(
	'core/cover',
	{
		name: 'cover',
		isDefault: true,
		attributes: {
			layout: {
				type: 'default',
			},
		},
	}
);

I'm still able to set a default Media & Text block without unregistering a block variation ahead of time (and also without a name property).

@ndiego
Copy link
Member

ndiego commented Aug 18, 2023

Wanting to set that to also default to not have the layout toggle on, I seem to have to unregister the default block variation (which I didn't even think existed?) for this to take effect:

I was surprised as well, but it actually does have a default block variation. See here. This was added in 6.2 explicitly due to the layout controls. There is an explanation here.

I need to keep investigating this, but my guess right now is that if a block already has a variation set to isDefault, you much first unregister that variation to then add your own as the default. @tellthemachines would you be able to confirm if this is true?

@tellthemachines
Copy link
Contributor

In WordPress 6.2 (and prior versions), I could use this code to override the default Group block variation so it "inherits" the content layout (very confusing option, btw):

@mrwweb is the intention of this to have all Groups added with content width by default? Or without it? The Group variation was made default when it was decided that all newly added Groups should have content width. That change was made in #42763, which went into core in WP 6.1.

The Cover block change that added layout has only been in core since 6.3.

@ndiego
Copy link
Member

ndiego commented Aug 18, 2023

The Cover block change that added layout has only been in core since 6.3.

Ah thanks for the correction. I saw that PR on the 6.2 project board and didn't investigate further.

After a bit more experimentation with isDefault, I have come up with a reliable understanding of how it works when there are multiple variations with isDefault: true. Also note that while name is not technically required, it adds uniqueness to the variation, and adding name makes these tricky interactions more clear.

Two variations with isDefault: true with the same name or both with no name set

In the example, only the Red Paragraph will display in the inserter. All subsequent variations added with the same name are ignored. The paragraph variation already exists from the perspective of the Editor. If both variations did not have a name set, the result would be the same.

wp.blocks.registerBlockVariation(
	'core/paragraph',
	{
		name: 'paragraph',
		title: 'Red Paragraph',
		isDefault: true,
		attributes: {
			textColor: 'vivid-red',
		},
	}
);

wp.blocks.registerBlockVariation(
	'core/paragraph',
	{
		name: 'paragraph',
		title: 'Purple Paragraph',
		isDefault: true,
		attributes: {
			textColor: 'vivid-purple',
		},
	}
);
image

Two variations with isDefault: true with the different names, or one with no name set

Notice that both variations now have different names. In this case, both variations will show in the inserter because the Editor sees them as being unique. But isDefault fails in this case because there are multiple variations all vying to be the default. This implementation feels correct and leaves it up to the developer to decide which one should actually be the default (solution below).

wp.blocks.registerBlockVariation(
	'core/paragraph',
	{
		name: 'paragraph-red',
		title: 'Red Paragraph',
		isDefault: true,
		attributes: {
			textColor: 'vivid-red',
		},
	}
);

wp.blocks.registerBlockVariation(
	'core/paragraph',
	{
		name: 'paragraph-purple',
		title: 'Purple Paragraph',
		isDefault: true,
		attributes: {
			textColor: 'vivid-purple',
		},
	}
);
image

Solution for overriding previously registered variations set to isDefault: true

So, when there are multiple variations set to isDefault: true and each variation has a unique name, you must unregister any variations set to isDefault: true for yours to be the only variation that displays. This is another very important reason why all variations should have a name. Without the name, there is no way to unregister a variation.

 wp.blocks.registerBlockVariation(
	'core/paragraph',
	{
		name: 'paragraph-red',
		title: 'Red Paragraph',
		isDefault: true,
		attributes: {
			textColor: 'vivid-red',
		},
	}
);

wp.blocks.unregisterBlockVariation( 'core/paragraph', 'paragraph-red' );

wp.blocks.registerBlockVariation(
	'core/paragraph',
	{
		name: 'paragraph-purple',
		title: 'Purple Paragraph',
		isDefault: true,
		attributes: {
			textColor: 'vivid-purple',
		},
	}
);

After unregistering paragraph-red, only paragraph-purple displays in the inserter.

image

@tellthemachines @andrewserong @Mamaduka (and anyone else) I think everything I have outlined above is correct and the expected functionality of variations, but would love a gut check here 😅

I will then update the Block Variations API doc to make all of this much more clear.

@Mamaduka
Copy link
Member

Described behavior makes sense to me. When there's already a registered default, you'll have to unregister it first to override it with yours.

@mrwweb
Copy link
Author

mrwweb commented Aug 18, 2023

@ndiego Thanks for such thorough research! Everything you did aligns with what I saw as well. I think you're right that updating the documentation would help a lot.

I think a lot of my confusion came from the registerBlockVariation documentation of isDefault:

isDefault (optional, type boolean) – Indicates whether the current variation is the default one. Defaults to false.

I think I took that to mean that the "last" variation registered would "win" (as is often the case in other contexts) and replace the first one. It sounds like that isn't the intended behavior, but I do wonder if it would be a good behavior (i.e., if registerBlockVariation is called with the same name as an existing variation and isDefault is true, that becomes the new default).

is the intention of this to have all Groups added with content width by default?

@tellthemachines That option is so confusingly named that I'm still not sure how to say it correctly. This is what I wanted as the default state when selecting "Group" from the inserter:

A friend and I were looking at that toggle a couple days ago, and in both our minds, the toggle behavior is the opposite of what we think it should be. If it's using the content width (toggled on), then the width settings shouldn't be visible (the opposite of the current behavior). It's also confusing since the previous way to get this result in WordPress 6.2 was to set attributes.layout.inherit to true. I assume none of this is going to change, but wanted to just provide my $0.02. I know the naming of that options has been painful and drawn out.

Update: I was completely turned around by this setting. Sorry. It turns out I actually do want the default behavior and I think I got confused by it becoming the default behavior. (Yay!).

@andrewserong
Copy link
Contributor

I think everything I have outlined above is correct and the expected functionality of variations, but would love a gut check here 😅

Yes, that all sounds correct to me. Thanks for digging in to articulate the current behaviour!

@jordesign jordesign added the [Type] Bug An existing feature does not function as intended label Aug 30, 2023
@tellthemachines
Copy link
Contributor

I'm going to go ahead and close this as it seems there isn't anything actionable here. Feel free to reopen if needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Variations Block variations, including introducing new variations and variations as a feature Needs Technical Feedback Needs testing from a developer perspective. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants