fix(a11y-landmark): spread other html props over tabs nav item - #1405
fix(a11y-landmark): spread other html props over tabs nav item#1405Stefan3002 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for passing additional HTML/ARIA attributes to the <nav> element rendered by Tabs, to help consumers provide unique accessible names (avoiding “unique landmark” a11y violations when multiple nav landmarks exist).
Changes:
- Spreads remaining props from
Tabsonto the root<nav>element.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const Tabs = <P,>({ | ||
| className, | ||
| links, | ||
| listClassName, | ||
| ...props |
| }: Props<P>): React.JSX.Element => { | ||
| return ( | ||
| <nav className={classNames("p-tabs", className)}> | ||
| <nav className={classNames("p-tabs", className)} {...props}> |
|
Will check the Copilot comments and address them. |
edlerd
left a comment
edlerd
left a comment
There was a problem hiding this comment.
LGTM, one question on types below.
| export type Props<P = null> = { | ||
| /** | ||
| * Optional classes applied to the parent "nav" element. | ||
| */ | ||
| className?: ClassName; | ||
| /** | ||
| * An array of tab link objects. | ||
| */ | ||
| links: TabLink<P>[]; | ||
| /** | ||
| * Optional classes applied to the "ul" element. | ||
| */ | ||
| listClassName?: string; | ||
| }; |
There was a problem hiding this comment.
Do we need to adjust the type here as well to announce the ones for the nav item that can be passed with a spread?
I suspect in the current form it will work, but TypeScript will not be able to figure out the right available types.
Done
QA
Pinging @canonical/react-library-maintainers for a review.
Make sure that, when you pass in a "aria-label" attribute to the Tabs component, it is actually rendered in the nav element
Storybook
To see rendered examples of all react-components, run:
QA
Fixes
Fixes: #AC-4738