Heading block: move alignment controls to toolbar (#2)

[gziolo]

Description

An alternative to #16682.

Closes #16648.

Added an alignment toolbar component to the heading block's toolbar.

On the implementation side of things, the biggest difference in comparison to #16682, this PR adds an icon for each heading level which removes the need to drill down subscript prop to other components together with CSS styling based on the data attribute.

TODO

• Provide better icons which have fixed size 20x20

[gziolo]

@mapk or @jasmussen - how do you feel about this approach? We would still have to create icons which fit better to make it look solid.

[youknowriad]

Nit: /> can be moved to the next line

[gziolo]

I missed that, sorry :(

[youknowriad]

Code wise, this looks good. Design-wise, these svgs might need to be tweaked but I leave that for designers.

[jasmussen]

Not able to test this at the moment, but purely judging by the lovely screenshots, this looks good. The hierarchy is right, heading level first. The dropdowns work well for keeping the toolbar compact. The icons look great and is a good alternative to the previous heading icons, more legible. In that vein and as a design sanity check, 👍👍

Now that the heading level is in a drop-down, there is an open question around the heading levels, though. So as to not keep up this PR, this is fine to look at separately, but worth mentioning. And that is: should we show all heading levels, h1 to h6, in the drop-down? Given the space is there, the absence seems confusing.

I realize the reason they are not there is that we don't want to encourage wrongful usage of the heading levels breaking the semantics. In the past we forced users into the sidebar to choose those levels. At the same time if we show all levels here, we don't need to also have the levels in the sidebar, and we do have tools to help you understand wrongful usage of heading levels, so maybe it's okay?

[gziolo]

Now that the heading level is in a drop-down, there is an open question around the heading levels, though. So as to not keep up this PR, this is fine to look at separately, but worth mentioning. And that is: should we show all heading levels, h1 to h6, in the drop-down? Given the space is there, the absence seems confusing.
I realize the reason they are not there is that we don't want to encourage wrongful usage of the heading levels breaking the semantics. In the past we forced users into the sidebar to choose those levels. At the same time if we show all levels here, we don't need to also have the levels in the sidebar, and we do have tools to help you understand wrongful usage of heading levels, so maybe it's okay?

I'm open to iterations here. There is one thing to consider, we want to provide tooling which helps to emphasize issues with the semantics of your content. Heading levels is one of the common issues which impacts accessibility. We have this PR proposed: #14889 which adds heading hierarchy checker:

So, if we were to consolidate those levels in the toolbar, we would have to find a different way to present this message.

[jasmussen]

Excellent point. That is an argument to move forward with this PR as is, and explore the changes discussed separately.

And regarding those changes, it seems like it should be possible to show such a warning directly in the drop-down, no?

[jasmussen]

GIF:

The icons are a bit small

So I increased their size

[jasmussen]

I think we should follow up on showing all heading levels, with warnings for picking the wrong ones, but in a separate PR. As is, this is good from a design perspective. If anyone wants to give additional sanity checks to the code feel free, but otherwise 👍 👍

[jasmussen]

Here's a very rough mockup of what it could look like, to show the warnings for incorrect heading level usage in the menu:

☝️ We would want to either link to more information, or show more information as a tooltip, or rephrase to be more descriptive. Also the h1 icon should be used, obviously. But as an "inspector" mockup, it hopefully gets the point across. What do you think?

[mapk]

As is, it feels good to me too. I agree with @jasmussen that I'd like to see all heading levels in the popover as well. Should we include all headings in this PR right now? And work in the warnings in another PR as mentioned?

[youknowriad]

Should we include all headings in this PR right now? And work in the warnings in another PR as mentioned?

I feel we should do both these things at the same time to avoid the potential "regression" of this message not being seen by people using the new dropdown in the meantime.

[gziolo]

I'm merging this PR as is to avoid lengthy debate about implications of the change in UI. For reference, this is how it would look in the dropdown:

Code:

diff --git a/packages/block-library/src/heading/edit.js b/packages/block-library/src/heading/edit.js
index 3289fb5a7..b591bca52 100644
--- a/packages/block-library/src/heading/edit.js
+++ b/packages/block-library/src/heading/edit.js
@@ -61,16 +61,12 @@ function HeadingEdit( {
 	return (
 		<>
 			<BlockControls>
-				<HeadingToolbar minLevel={ 2 } maxLevel={ 5 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
+				<HeadingToolbar minLevel={ 1 } maxLevel={ 6 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
 				<AlignmentToolbar value={ align } onChange={ ( nextAlign ) => {
 					setAttributes( { align: nextAlign } );
 				} } />
 			</BlockControls>
 			<InspectorControls>
-				<PanelBody title={ __( 'Heading Settings' ) }>
-					<p>{ __( 'Level' ) }</p>
-					<HeadingToolbar isCollapsed={ false } minLevel={ 1 } maxLevel={ 7 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
-				</PanelBody>
 				<HeadingColorUI
 					setTextColor={ setTextColor }
 					textColorValue={ textColor.color }
diff --git a/packages/block-library/src/heading/edit.native.js b/packages/block-library/src/heading/edit.native.js
index 389791663..961f08832 100644
--- a/packages/block-library/src/heading/edit.native.js
+++ b/packages/block-library/src/heading/edit.native.js
@@ -27,8 +27,8 @@ const HeadingEdit = ( {
 	<View onAccessibilityTap={ onFocus }>
 		<BlockControls>
 			<HeadingToolbar
-				minLevel={ 2 }
-				maxLevel={ 5 }
+				minLevel={ 1 }
+				maxLevel={ 6 }
 				selectedLevel={ attributes.level }
 				onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) }
 			/>
diff --git a/packages/block-library/src/heading/heading-toolbar.js b/packages/block-library/src/heading/heading-toolbar.js
index f3a089a8f..fb85152c0 100644
--- a/packages/block-library/src/heading/heading-toolbar.js
+++ b/packages/block-library/src/heading/heading-toolbar.js
@@ -33,7 +33,7 @@ class HeadingToolbar extends Component {
 			<Toolbar
 				isCollapsed={ isCollapsed }
 				icon={ <HeadingLevelIcon level={ selectedLevel } /> }
-				controls={ range( minLevel, maxLevel ).map(
+				controls={ range( minLevel, maxLevel + 1 ).map(
 					( index ) => this.createLevelControl( index, selectedLevel, onChange )
 				) } />
 		);

[gziolo]

Continuing on the previous topic of accessibility violations.

Here's a very rough mockup of what it could look like, to show the warnings for incorrect heading level usage in the menu:

The biggest challenge I see is the discovery of this message. As soon as you change the level of heading, you won't see the dropdown anymore so it'd be very likely you would miss it. I'm wondering whether you see an elegant way to extend block's UI to have a space for such warnings. The other example is the contrast warning which is located in the sidebar. In that case, the feedback loop is much better as you need to perform all interactions in the same place.