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 5 commits into
base: trunk
Choose a base branch
from
Open

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

Changes from 2 commits
Commits
Show all changes
5 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
1e8a630
Merge branch 'trunk' into group/navigation-ui
t-hamano Feb 4, 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
43 changes: 31 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 @@ -23,13 +23,14 @@ import { htmlElementMessages } from '../utils/messages';
/**
* 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;

return (
<InspectorControls group="advanced">
<SelectControl
Expand All @@ -44,11 +45,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

@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.

Is this expected though? I am torn.

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 @@ -125,10 +146,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