New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/droprdown accessibility #48499
base: master
Are you sure you want to change the base?
Feat/droprdown accessibility #48499
Conversation
Run & review this pull request in StackBlitz Codeflow. |
👁 Visual Regression Report for PR #48499 Failed ❌
Check Full Report for details |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that okay that dropdown need additional tab press to focus on first menu element ?
@@ -43,7 +43,8 @@ export type DropdownArrowOptions = { | |||
}; | |||
|
|||
export interface DropdownProps { | |||
menu?: MenuProps; | |||
menu: MenuProps; | |||
menuLabel?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can i make this prop as required ? Each menu must have unique label, as i use it to bound menu btn and menu itself
components/dropdown/dropdown.tsx
Outdated
@@ -238,7 +263,9 @@ const Dropdown: CompoundedComponent = (props) => { | |||
|
|||
let overlayNode: React.ReactNode; | |||
if (menu?.items) { | |||
overlayNode = <Menu {...menu} />; | |||
overlayNode = ( | |||
<Menu ref={menuRef} aria-labelledby={`${menuLabel}-button`} id={`${menuLabel}-menu`} {...menu} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for ref
components/dropdown/dropdown.tsx
Outdated
const handleKeyPress = (e: KeyboardEvent) => { | ||
if (e.key === 'Enter' || e.code === 'Space') { | ||
targetElement.removeEventListener('keypress', handleKeyPress); | ||
onOpenChange?.(true, { source: 'menu' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is better to use onInnerOpenChange
then this hardcode
🤔 This is a ...
🔗 Related issue link
Fix #48132
📝 Changelog
☑️ Self-Check before Merge