Replace block variation buttons with ToggleGroupControl

[TimBroddin]

What?

The buttons to transform blocks to different variantions should be replaced with the new icon button variant of ToggleGroupControl.

Why?

Because the ToggleGroupControl is better suited for this. This will improve consistency, reduce code duplication, and add visual/semantic indications that the options are mutually exclusive.

How?

I replaced the buttons with ToggleGroupControl/ToggleGroupControlOptionIcon

Testing Instructions

1. Create a new post or page
2. Add a Group, Row or Column block
3. Verify that the block variations picker still works.

Screenshots or screencast

Before	After

[codesandbox]

  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

[noisysocks]

It might look a bit closer to the original if we add size="__unstable-large" to <ToggleGroupControl>. This makes the control 40px high which will eventually be the default.

[TimBroddin]

It might look a bit closer to the original if we add size="__unstable-large" to <ToggleGroupControl>. This makes the control 40px high which will eventually be the default.

Thanks for pointing this out! I'll change it!

[andrewserong]

Thanks for looking into this one! Unfortunately switching over to ToggleGroupControl seems to introduce an issue if we have a large number of variations (they overflow to the right in the inspector):

While the Group block currently only has a few variations, it's possible that there are custom blocks out in the wild with larger numbers of variations with the transform scope.

One way to test this manually is to hard-code the social link/icon block to expose the block variations as transforms. To do this, add the following line to the variations.forEach in block-library/src/social-link/variations.js in your local environment:

variation.scope = [ 'transform' ];

All up that area in my local environment looks like:

variations.forEach( ( variation ) => {
	if ( variation.isActive ) return;
	variation.isActive = ( blockAttributes, variationAttributes ) =>
		blockAttributes.service === variationAttributes.service;
	variation.scope = [ 'transform' ];
} );

That then exposes all the social icon variations in the UI for testing.

Would it be possible to use the ToggleGroupControl only if the number of variations is lower than a certain threshold, and if it's over that amount, perhaps use the existing buttons?

[TimBroddin]

Thanks for pointing this out @andrewserong. I'll add a check!

[TimBroddin]

I've updated the PR.

Five or less options	More than five options

[andrewserong]

Thanks for the follow-ups!

Are these style removals intentional? Most of these appear to affect the dropdown list version of the list of variations, which is used when not all of the variations have unique icons.

To test this, with the changes applied to scope the social icons for transform, in packages/block-library/src/social-link/variations.js also set any of the social icon blocks to use the same icon as one of the other variations. E.g. set any of them to use the YouTubeIcon icon.

Here's a quick view of trunk versus this PR:

Trunk	This PR

[TimBroddin]

Oops, I need more coffee. I forgot to push the changes 😄 Thanks for your patience!

[andrewserong]

Oops, I need more coffee.

No worries, I have the opposite problem of having drunk too much coffee!

I've re-pulled the changes, and it looks like there are still some differences in the dropdown popover for the transform to variation list. Visually it looks like there's now additional padding / a larger width of the popover, and it overflows the area so there's a horizontal scrollbar:

Trunk	This PR

[andrewserong]

Nice work @TimBroddin, thanks for all the back-and-forth here! This is testing well for me in all three cases:

✅ Group block's variations use the ToggleGroupControl and toggle as expected
✅ A block with a large number of variations, each with a unique icon, correctly renders the buttons approach
✅ A block with a large number of variations, where there are not unique icons, renders the drop down list approach

This LGTM! ✨ It might be worth also getting an approval from a designer before merging, too

[andrewserong]

@jasmussen just copying you in on this one as I think from memory you put together the original designs for this area?

[jasmussen]

Hey, thanks for the PR, thanks for the ping, conceptually seems just right.

Just to be sure I get the context right: when there are more than a rows worth of transforms, they still become a dropdown, correct?

This is what I'm seeing in the branch:

There should be no enclosing border around these controls. I thought actually that was removed as part of #45492, but it seems to be there after a rebase. Is there a prop that needs to be set/unset? CC: @mirka @ciampo

[mirka]

Regarding @jasmussen point about the border: The isBlock prop is what controls the border visibility for the moment. It looks like the Flex component is being used here to undo what the isBlock prop is for anyway, so I think all we need to do is remove both the Flex and isBlock prop 🙂

[mirka]

Is this fieldset necessary? The ToggleGroupControl alone is semantically grouped and labeled. Maybe swap with a div if it's just for the wrapper classes?

[mirka]

@TimBroddin Are you available to address the remaining feedback? If not, let us know so we can find someone to take over 🙂

[TimBroddin]

@TimBroddin Are you available to address the remaining feedback? If not, let us know so we can find someone to take over 🙂

If you could find someone to take over, that would be great. Sorry for not addressing this earlier.