Fix Screen reader is not announcing the index numbers for the items in V2 dropdown #23613
Conversation
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
docs/src/theme/NavbarItem/DocsVersionDropdownNavbarItem.tsx:72
- Ensure that version.label is properly sanitized before using it in the aria-label to avoid potential issues with screen readers.
"aria-label": `Version ${version.label}, item ${index + 1} of ${versions.length}`,
docs/src/theme/NavbarItem/DocsVersionDropdownNavbarItem.tsx:68
- [nitpick] Consider using aria-label instead of label for accessibility purposes to maintain consistency.
"label": version.label,
| */ | ||
| function getVersionMainDoc(version: GlobalVersion): GlobalDoc { | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| return version.docs.find((doc) => doc.id === version.mainDocId)!; |
There was a problem hiding this comment.
The main doc should always exist right? Should we actually consider adding an assertion here?
There was a problem hiding this comment.
This is a good point. We should explicitly mention the expected behavior in edge cases, such as when no main doc is found
There was a problem hiding this comment.
What method of assertion would you prefer? I used the non-null assertion, we could use another method. should I assign it to a variable and assert if that variable is not unefined?
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
|
Thank you @jikim-msft! |
|
|
||
| /** | ||
| * Wraps the default DocsVersionDropdownNavbarItem to omit the drop-down on non-versioned pages. | ||
| * This module provides a custom implementation of the DropdownNavbarItem to enhance accessibility of the dropdown menu for navigation between doc versions. |
There was a problem hiding this comment.
Nit: this isn't a module, it's a function.
| * @remarks | ||
| * Suggested workaround for lack of version dropdown customization. | ||
| * See {@link https://github.com/facebook/docusaurus/issues/4389}. | ||
| * Context: |
There was a problem hiding this comment.
Nit: since this isn't part of the semantic description of the function, it really belongs under the @remarks block.
| */ | ||
| function getVersionMainDoc(version: GlobalVersion): GlobalDoc { | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion |
There was a problem hiding this comment.
I don't see a non-null assertion on the next line. Is this needed?
Fix Screen reader is not announcing the index numbers for the items in V2 dropdown
How to test