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.

Sign up for GitHub

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

Group: Make nav element selectable and add UI for ariaLabel #68611

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Open

Group: Make nav element selectable and add UI for ariaLabel #68611

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8159156
Group: Make nav element selectable and add UI for ariaLabel
t-hamano Jan 11, 2025
fa7b95c
Resolve conflicts
t-hamano Jan 17, 2025
4bad275
Merge branch 'trunk' into group/navigation-ui
t-hamano Jan 18, 2025
79d6d8f
Update navigation label
t-hamano Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
useInnerBlocksProps,
store as blockEditorStore,
} from '@wordpress/block-editor';
import { SelectControl } from '@wordpress/components';
import { SelectControl, TextControl } from '@wordpress/components';
import { useRef } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { View } from '@wordpress/primitives';
Expand All @@ -22,13 +22,13 @@ import GroupPlaceHolder, { useShouldShowPlaceHolder } from './placeholder';
/**
* Render inspector controls for the Group block.
*
* @param {Object} props Component props.
* @param {string} props.tagName The HTML tag name.
* @param {Function} props.onSelectTagName onChange function for the SelectControl.
*
* @return {JSX.Element} The control group.
* @param {Object} props Component props.
* @param {Object} props.attributes Block attributes.
* @param {(attributes: Object) => void} props.setAttributes Callback for updating block attributes.
* @return {JSX.Element} The control group.
*/
function GroupEditControls( { tagName, onSelectTagName } ) {
function GroupEditControls( { attributes, setAttributes } ) {
const { tagName, ariaLabel } = attributes;
const htmlElementMessages = {
header: __(
'The <header> element should represent introductory content, typically a group of introductory or navigational aids.'
Expand All @@ -48,6 +48,9 @@ function GroupEditControls( { tagName, onSelectTagName } ) {
footer: __(
'The <footer> element should represent a footer for its nearest sectioning element (e.g.: <section>, <article>, <main> etc.).'
),
nav: __(
'The <nav> element should represent a section of a page that links to other pages or to parts within the page.'
t-hamano marked this conversation as resolved.
Show resolved Hide resolved
),
};
return (
<InspectorControls group="advanced">
Expand All @@ -63,11 +66,31 @@ function GroupEditControls( { tagName, onSelectTagName } ) {
{ label: '<article>', value: 'article' },
{ label: '<aside>', value: 'aside' },
{ label: '<footer>', value: 'footer' },
{ label: '<nav>', value: 'nav' },
] }
value={ tagName }
onChange={ onSelectTagName }
onChange={ ( value ) => {
setAttributes( {
tagName: value,
ariaLabel: value === 'nav' ? ariaLabel : undefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to override already existing aria labels when the tag name is changed to and from nav

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work like this:

  • When changed TO a nav element: existing aria-Label is maintained
  • When changed FROM a nav element: aria-Label is removed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this expected though? I am torn, it would be good to have some user testing.

If I have a group block and I use the transform, either to another block like the details or to a row or stack, the label is kept.

Copy link
Contributor

@carolinan carolinan Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the UI was always available, I don't think the removal would be expected.
Without the UI, it is more difficult to discover both that the label is removed, or that it is kept.

} );
} }
help={ htmlElementMessages[ tagName ] }
/>
{ tagName === 'nav' && (
<TextControl
label={ __( 'Navigation label' ) }
value={ ariaLabel || '' }
__next40pxDefaultSize
__nextHasNoMarginBottom
onChange={ ( value ) => {
setAttributes( { ariaLabel: value } );
} }
help={ __(
'Add a label to describe the purpose of this navigation element.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to provide a better help text to educate users about the recommended best practices and communicate the concept that If a page includes more than one navigation, each should have a unique label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated by 79d6d8f

) }
/>
) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's just time to add a full standardized UI for the aria label block support 🤔

In reality users can already set this attribute in code today. It just doesn't have a UI... I'm not sure only showing it for the nav tag name makes it more clear 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's just time to add a full standardized UI for the aria label block support

That's certainly true, and maybe we should do so in the future.

Personally, I'm cautious about changing the block support spec right now. For now, I prefer to expose the UI only to the Group block. I'd like to hear other people's opinions on this.

</InspectorControls>
);
}
Expand Down Expand Up @@ -144,10 +167,8 @@ function GroupEdit( { attributes, name, setAttributes, clientId } ) {
return (
<>
<GroupEditControls
tagName={ TagName }
onSelectTagName={ ( value ) =>
setAttributes( { tagName: value } )
}
attributes={ attributes }
setAttributes={ setAttributes }
/>
{ showPlaceholder && (
<View>
Expand Down
Loading