feat(Tabs): add tabListAriaLabel prop #12193
Conversation
WalkthroughAdds two optional props, Changes
Sequence Diagram(s)(Pipeline changes are small, single-component prop passthroughs; no sequence diagram generated.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-core/src/components/Tabs/Tabs.tsx (1)
64-65: Consider clarifying the documentation.The documentation "Aria-label for the list element" could be more specific. Consider updating it to explicitly mention that this labels the
<ul role="tablist">element to help developers understand its purpose better.📝 Suggested documentation improvement
- /** Aria-label for the list element */ + /** Aria-label for the tab list element (the <ul> with role="tablist"). When multiple tab sets exist on a page, provide a unique label to differentiate them for screen readers. */ tabListAriaLabel?: string;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/components/Tabs/Tabs.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (2)
packages/react-core/src/components/Tabs/Tabs.tsx (2)
505-505: LGTM!The prop is correctly extracted from props and properly positioned in the destructuring statement.
632-638: LGTM!The implementation correctly applies the
aria-labelto the tab list element with an appropriate fallback. The approach is consistent with similar props in the component (e.g.,addButtonAriaLabelat line 658).
|
Preview: https://pf-react-pr-12193.surge.sh A11y report: https://pf-react-pr-12193-a11y.surge.sh |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-core/src/components/Tabs/Tabs.tsx (1)
64-65: Consider clarifying the prop documentation.The comment "Aria-label for the list element" could be more specific. Consider updating it to indicate this applies specifically to the
<ul role="tablist">element, and provide guidance on when consumers should set this prop.🔎 Suggested documentation improvement
- /** Aria-label for the list element */ + /** Aria-label for the tab list element (<ul role="tablist">). Provide a descriptive label to identify the purpose of the tab set for assistive technologies. */ tabListAriaLabel?: string;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/components/Tabs/Tabs.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
|
Hey @thatblindgeye, I think this Closes #12128 , unless you'd rather have like a whole |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Thanks for making this update! In addition to the comments here, there's a couple file comments below as well. Let me know if you have any questions!
- Would you feel comfortable adding some unit tests for the new prop?
- We should also make updates to any necessary examples/demos using Tabs. Ideally I think what we would want to do is only make updates where subtabs are being used: wwe could add a tabsListAriaLabel of "Primary" to the main/outermost tabs, and depending how the subtabs are setup giving the Tabs with
isSubTaban accessible name that matches the currently selected main tab (so for example, if "Users" is the main selected tab, giving the subtabtabsListAriaLabel="Users", or using aria-labelledby instead that references the "Users" tab text.
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
Sure, I'll add tests, and update the examples |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-core/src/components/Tabs/Tabs.tsx (2)
64-65: Consider clarifying the JSDoc to distinguish from the outeraria-labelprop.The component has two aria-label props: one for the outer wrapper (line 93:
aria-label) and this new one for the inner tablist element. Consider updating the JSDoc to clarify this relationship and when to use each. For example:- /** A readable string to create an accessible name for the tablist element. This can be used to differentiate multiple tablists on a page, and should be used for subtabs. */ + /** A readable string to create an accessible name for the tablist element (the ul with role="tablist"). Use this to differentiate multiple tablists on a page or to label subtabs. This is separate from the component's aria-label prop, which labels the outer wrapper. */ tabListAriaLabel?: string;
632-638: Reconsider the default aria-label value for better accessibility.The current implementation always applies
aria-label="Tab List"whentabListAriaLabelis not provided. This generic default may reduce accessibility in scenarios with multiple tablists on a page, as they would all have identical labels.Per WAI-ARIA best practices:
- If there's only one tablist on the page, an aria-label may not be necessary
- If there are multiple tablists, each should have a unique, descriptive label
Consider making the aria-label conditional:
♻️ Proposed refactor
<ul - aria-label={tabListAriaLabel || 'Tab List'} + {...(tabListAriaLabel && { 'aria-label': tabListAriaLabel })} className={css(styles.tabsList)} ref={this.tabList} onScroll={this.handleScrollButtons} role="tablist" >Alternatively, if a default is preferred, consider logging a warning when
isSubtabis true buttabListAriaLabelis not provided, similar to the existing warning pattern at lines 186-193.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/components/Tabs/Tabs.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
…s, and examples Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/react-core/src/components/Tabs/__tests__/Tabs.test.tsx (3)
746-759: Good test coverage; consider adding explicit assertion.The snapshot test provides coverage for the
tabListAriaLabelprop. However, for accessibility features, an explicit assertion would make the test more robust and clearly document the expected behavior.💡 Suggested enhancement with explicit assertion
test('should render tablist aria-label when provided', () => { const { asFragment } = render( <Tabs id="tabListLabelTabs" tabListAriaLabel="Primary tab list"> <Tab id="tab1" eventKey={0} title={<TabTitleText>"Tab item 1"</TabTitleText>}> Tab 1 section </Tab> <Tab id="tab2" eventKey={1} title={<TabTitleText>"Tab item 2"</TabTitleText>}> Tab 2 section </Tab> </Tabs> ); + expect(screen.getByRole('tablist')).toHaveAttribute('aria-label', 'Primary tab list'); expect(asFragment()).toMatchSnapshot(); });
761-777: Good test coverage; consider adding explicit assertion.The snapshot test provides coverage for the
tabListAriaLabelledByprop. An explicit assertion would make the relationship between the heading and tablist clearer.💡 Suggested enhancement with explicit assertion
test('should render tablist aria-labelledby when provided', () => { const { asFragment } = render( <> <h2 id="tablistHeading">My tabs heading</h2> <Tabs id="tabListLabelledByTabs" tabListAriaLabelledBy="tablistHeading"> <Tab id="tab1" eventKey={0} title={<TabTitleText>"Tab item 1"</TabTitleText>}> Tab 1 section </Tab> <Tab id="tab2" eventKey={1} title={<TabTitleText>"Tab item 2"</TabTitleText>}> Tab 2 section </Tab> </Tabs> </> ); + expect(screen.getByRole('tablist')).toHaveAttribute('aria-labelledby', 'tablistHeading'); expect(asFragment()).toMatchSnapshot(); });
779-792: Good default-case coverage; consider adding explicit assertion.The snapshot test verifies the default behavior when neither prop is provided. An explicit assertion would clearly document that the tablist should not have these aria attributes by default.
💡 Suggested enhancement with explicit assertion
test('should not render tablist aria-label or aria-labelledby when neither is provided', () => { const { asFragment } = render( <Tabs id="noTabListLabelTabs"> <Tab id="tab1" eventKey={0} title={<TabTitleText>"Tab item 1"</TabTitleText>}> Tab 1 section </Tab> <Tab id="tab2" eventKey={1} title={<TabTitleText>"Tab item 2"</TabTitleText>}> Tab 2 section </Tab> </Tabs> ); + const tablist = screen.getByRole('tablist'); + expect(tablist).not.toHaveAttribute('aria-label'); + expect(tablist).not.toHaveAttribute('aria-labelledby'); expect(asFragment()).toMatchSnapshot(); });Additionally, you may want to add an edge-case test to verify behavior when both props are provided (aria-labelledby should take precedence per ARIA specification):
test('should prioritize aria-labelledby over aria-label when both are provided', () => { render( <> <h2 id="tablistHeading">My tabs heading</h2> <Tabs id="bothPropsProvidedTabs" tabListAriaLabel="Label text" tabListAriaLabelledBy="tablistHeading" > <Tab id="tab1" eventKey={0} title={<TabTitleText>"Tab item 1"</TabTitleText>}> Tab 1 section </Tab> </Tabs> </> ); const tablist = screen.getByRole('tablist'); expect(tablist).toHaveAttribute('aria-labelledby', 'tablistHeading'); expect(tablist).toHaveAttribute('aria-label', 'Label text'); });packages/react-core/src/components/Tabs/examples/TabsSubtabs.tsx (1)
45-45: LGTM! Consider demonstratingaria-labelledbyas an alternative.The
tabListAriaLabel="Users"correctly matches the parent tab title and follows the reviewer's guidance. Since the commit messages indicatetabListAriaLabelledBywas also implemented, you might consider demonstrating that alternative approach as well—either here or in a separate example—to showcase the full API surface and dynamic labeling patterns.packages/react-core/src/components/Tabs/examples/TabsNavSubtab.tsx (1)
39-39: Subtab labeling is correct; consider demonstratingaria-labelledbyas well.The
tabListAriaLabel="Users"correctly matches the parent tab title as requested.Since the commit indicates
tabListAriaLabelledBywas also implemented, consider demonstrating that alternative approach in this or another example by referencing the parent tab's title element directly, rather than duplicating the string. This would showcase both new props and improve maintainability.💡 Alternative approach using aria-labelledby
If the parent tab title element has an ID, you could use:
- tabListAriaLabel="Users" + tabListAriaLabelledBy="users-tab-title-id"This would reference the actual tab title element and demonstrate the
tabListAriaLabelledByprop that was also added in this PR.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/Tabs/__tests__/__snapshots__/Tabs.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
packages/react-core/src/components/Tabs/Tabs.tsxpackages/react-core/src/components/Tabs/__tests__/Tabs.test.tsxpackages/react-core/src/components/Tabs/examples/Tabs.mdpackages/react-core/src/components/Tabs/examples/TabsNavSubtab.tsxpackages/react-core/src/components/Tabs/examples/TabsSubtabs.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/react-core/src/components/Tabs/examples/Tabs.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/components/Tabs/Tabs.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/components/Tabs/__tests__/Tabs.test.tsx (2)
packages/react-core/src/components/Tabs/Tabs.tsx (2)
render(474-688)Tabs(691-691)packages/react-core/src/components/Tabs/Tab.tsx (1)
Tab(149-149)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (2)
packages/react-core/src/components/Tabs/examples/TabsSubtabs.tsx (1)
36-36: LGTM! Correctly demonstrates the new prop.The addition of
tabListAriaLabel="Primary"properly labels the main tab list and aligns with the reviewer's request. The label clearly distinguishes the primary navigation from the subtabs.packages/react-core/src/components/Tabs/examples/TabsNavSubtab.tsx (1)
30-30: LGTM! Outer tabs correctly labeled.The
tabListAriaLabel="Primary"appropriately labels the primary tab list as requested in the review feedback.
3 participants
What
Add
tabListAriaLabelprop to set a custom aria-label on the tab list (Closes #12128)Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.