Fix icon size regression in Switcher

[jasmussen]

In #12901, a small margin was introduced to the IconButton component when text was present. This caused an issue with block icons, scaling them down when they shouldn't be. A 20x20px icon should show as 20x20, and a 24x24px icon should show as 24x24px.

The additional margin was applied even when the IconButton didn't actually have text. It simply needed children, and in the case of the Switcher, it has a dropdown arrow. Combined with the fixed width of the switcher, that meant a 24x24 icon would be rendered as 20x24.

This PR adds a length check so if your IconButton includes more than 2 letters, it's considered to have a text label. This fixes the issue. But as you can tell, maybe it's not the best way to check whether there's text or not. Suggestions most welcome.

Before:

After:

A good way to test this is to insert an Image placeholder block. The "Upload" button is actually an IconButton, so it should have margin, whereas the Switcher should not.

[ZebulanStanphill]

Would it be possible to check if the second child is an SVG element, and if so, not add the has-text class? That seems like a somewhat smarter way to fix the issue.

[aduth]

children is an opaque object type, and length is not intended to be reliable.

See also: https://reactjs.org/docs/react-api.html#reactchildren
Specifically: https://reactjs.org/docs/react-api.html#reactchildrencount

import { Children } from '@wordpress/element';

// ...

'has-text': Children.count( children ) > 2

[jasmussen]

Thanks for the code improvement suggestion, much appreciated. Though counting the children to be more than 2 on its own feels hacky — do you have any suggestions for how to make sure that the "has-text" returns true when the IconButton has [Icon] Label, but false in cases of [Icon] [Icon] in a more reliable way?

[aduth]

As I see it, BlockSwitcher is using IconButton incorrectly. If children are passed to IconButton, it should be assumed to be a text label. The icon should be passed using the icon prop, even when those icons are not referenced by name, or when it's some custom "aggregate" icon (like with the BlockSwitcher's __transform sibling).

In other words, I think if we change this:

gutenberg/packages/editor/src/components/block-switcher/index.js

Lines 111 to 123 in 1c583b2

	<IconButton
	className="editor-block-switcher__toggle"
	onClick={ onToggle }
	aria-haspopup="true"
	aria-expanded={ isOpen }
	label={ label }
	tooltip={ label }
	onKeyDown={ openOnArrowDown }
	>
	<BlockIcon icon={ icon } showColors />
	<SVG className="editor-block-switcher__transform" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><Path d="M6.5 8.9c.6-.6 1.4-.9 2.2-.9h6.9l-1.3 1.3 1.4 1.4L19.4 7l-3.7-3.7-1.4 1.4L15.6 6H8.7c-1.4 0-2.6.5-3.6 1.5l-2.8 2.8 1.4 1.4 2.8-2.8zm13.8 2.4l-2.8 2.8c-.6.6-1.3.9-2.1.9h-7l1.3-1.3-1.4-1.4L4.6 16l3.7 3.7 1.4-1.4L8.4 17h6.9c1.3 0 2.6-.5 3.5-1.5l2.8-2.8-1.3-1.4z" /></SVG>
	</IconButton>
	</Toolbar>

...to something more like:

<IconButton
	{ /* ... */ }
	icon={ (
		<Fragment>
			<BlockIcon />
			<SVG />
		</Fragment>
	) }
/>

...then we no longer need to deal with the problem of trying to figure out what sort of children are passed to IconButton, because we should be able to assume that children, if any exist, is the text label.

[jasmussen]

This was awesome, thank so much for the feedback. Implemented it verbatim and it seems to work:

[aduth]

Note to self: If we can consider an IconButton element which lacks an icon prop to be an error case, we could implement some custom ESLint rules to protect against it:

https://astexplorer.net/#/gist/de497f17dc6651842880ebd86c2aeb9b/c161a04f0d1e1fe0dc9c22e268fae3a9febe7fb8

May not be worth the effort, depending on #7534 (comment).

[aduth]

The previous build failed, but it looked to be an intermittent end-to-end test failure. I've restarted it.

Code-wise LGTM 👍 (I may be biased 😄 )

[gziolo]

The idea is to have two icons combined so this proposal makes sense 👍