Group: Make nav element selectable and add UI for ariaLabel #68611
+33
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Type] Enhancement
|
Size Change: +132 B (+0.01%) Total Size: 1.84 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
fabiankaegy
reviewed
Jan 11, 2025
Comment on lines
+80
to
+93
| { 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.' | ||
| ) } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
fabiankaegy
reviewed
Jan 11, 2025
| onChange={ ( value ) => { | ||
| setAttributes( { | ||
| tagName: value, | ||
| ariaLabel: value === 'nav' ? ariaLabel : undefined, |
There was a problem hiding this comment.
This is going to override already existing aria labels when the tag name is changed to and from nav
There was a problem hiding this comment.
This should work like this:
- When changed TO a
navelement: existing aria-Label is maintained - When changed FROM a
navelement: aria-Label is removed
|
Aside: Do you recall an issue about displaying the tagName in the ListView? Because it feels relevant to this change. But I can't find the discussions. I am saying it is relevant because it can help (advanced) users quickly locate the nav elements or items that need to be updated to navs. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #67407
What?
This PR makes it possible to operate the following two things from the Advanced panel in the Group block:
navelement as an HTML element (tagNameattribute)ariaLabelattribute)Why?
These two are already available as internal attributes. However, the only way to change them is to edit the HTML directly. (See TT5's example).
To help users create more accessible content, I propose adding a UI for these attributes to the Advanced panel.
How?
When an element other than a nav element is selected, the navigation label is removed. This is to prevent HTML like the one below from being saved:
This does not mean that the
aria-labelattribute cannot be used on elements other than thenavelement, but for now I will limit it to thenavelement.Additionally, we need some good help text to prevent users from misusing this UI. I also welcome your feedback on the help text.
Testing Instructions
nav.aria-labelattribute is added.nav.aria-labelattribute is NOT added.Screenshot