fix(ui-tooltip,ui-popover): put aria-expanded on trigger element#1965
Merged
Conversation
HerrTopi
force-pushed
the
add-aria-expanded
branch
from
f21f1f7 to
0057bea
Compare
|
HerrTopi
force-pushed
the
add-aria-expanded
branch
from
0057bea to
3c1a1f9
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new shouldSetARIAExpanded prop to Popover and Drilldown components to control whether aria-expanded is applied to their trigger elements, and disables it for a hidden submenu in the top nav bar.
- Add
shouldSetARIAExpandedprop (defaulttrue) in Popover and Drilldown props - Update Popover and Drilldown render logic to conditionally apply
aria-expanded - Pass
shouldSetARIAExpanded={false}for hidden Drilldown in TopNavBarMenuItems
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-top-nav-bar/src/TopNavBar/TopNavBarMenuItems/index.tsx | Disable aria-expanded on hidden Drilldown trigger |
| packages/ui-popover/src/Popover/props.ts | Define new shouldSetARIAExpanded prop |
| packages/ui-popover/src/Popover/index.tsx | Default prop, destructuring cleanup, conditional aria-expanded |
| packages/ui-drilldown/src/Drilldown/props.ts | Define new shouldSetARIAExpanded prop |
| packages/ui-drilldown/src/Drilldown/index.tsx | Default prop, pass-through to Popover, trigger aria-expanded |
Comments suppressed due to low confidence (1)
packages/ui-popover/src/Popover/index.tsx:472
- [nitpick] Consider adding unit tests or integration tests to verify that
aria-expandedis correctly set or omitted on the trigger based onshouldSetARIAExpandedand the Popover's open/close state.
if (this.props.shouldSetARIAExpanded) {
3c1a1f9 to
623ed76
Compare
matyasf
requested changes
Jun 12, 2025
HerrTopi
force-pushed
the
add-aria-expanded
branch
from
623ed76 to
2ae6f69
Compare
balzss
approved these changes
Jun 13, 2025
matyasf
approved these changes
Jun 13, 2025
matyasf
left a comment
matyasf
left a comment
Collaborator
There was a problem hiding this comment.
Nice work! Please add a more detailed commit message, and then its good to go
…xpanded, allow override with shouldSetAriaExpanded This change adds a new prop (shouldSetAriaExpanded) to Popover and Drilldown as well. It defaults to true. It is needed for setting automatically the aria-expanded attribute on the DOM. This can have a side-effect of duplicating aria-expanded in certain edge cases or putting aria-expanded on triggers where it doesn't need to be there. These are very rare cases so we decided to have "break" these cases and fix all the orher ones where aria-expanded was missing INSTUI-4544
HerrTopi
force-pushed
the
add-aria-expanded
branch
from
2ae6f69 to
abf8598
Compare
This file contains hidden or 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
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.
INSTUI-4544