-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(Dropdown, DropdownMenu, DropdownToggle): allow Dropdown to have … #2077
feat(Dropdown, DropdownMenu, DropdownToggle): allow Dropdown to have … #2077
Conversation
…listbox or menu role Dropdown Menu should allow for listbox or menu roles. Before this commit, the Dropdown could only have the menu role. After this commit, a user can add a menuRole property to the Dropdown component which will set a corresponding aria-haspopop property on the DropdownToggle, role property on the DropdownMenu, and role property on the Dropdown Items
src/Dropdown.js
Outdated
@@ -35,7 +36,8 @@ const defaultProps = { | |||
active: false, | |||
addonType: false, | |||
inNavbar: false, | |||
setActiveFromChild: false | |||
setActiveFromChild: false, | |||
menuRole: true |
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.
Don't think we need this. Just to make this behavior opt-in
Address all code cleanlieness and backwards compatibility issues brought up in PR
Okay! I made all the suggested changes. Thanks so much for all your suggestions and guidance @kyletsang |
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.
Almost there! Few more nitpicks and we're good to go.
src/Dropdown.js
Outdated
if(this.context.menuRole === 'listbox') { | ||
return 'option' | ||
} | ||
return '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.
return 'menu' | |
return 'menuitem' |
src/Dropdown.js
Outdated
if(this.context.menuRole) { | ||
return [].slice.call(menuContainer.querySelectorAll('[role="' + this.getItemType() + '"]')); | ||
} | ||
return [].slice.call(menuContainer.querySelectorAll('[role="menuitem"]')); |
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.
if(this.context.menuRole) { | |
return [].slice.call(menuContainer.querySelectorAll('[role="' + this.getItemType() + '"]')); | |
} | |
return [].slice.call(menuContainer.querySelectorAll('[role="menuitem"]')); | |
return [].slice.call(menuContainer.querySelectorAll(`[role="${this.getItemType()}"]`)); |
getItemType will return the default menuitem in case when menuRole doesn't exist, so we can simplify this. Also, string interpolation is good to use here too
src/DropdownItem.js
Outdated
const menuRole = this.context.menuRole | ||
if(menuRole === true || menuRole === 'menu' || menuRole === undefined) { | ||
return 'menuitem' | ||
} | ||
if(menuRole === 'listbox') { | ||
return 'option' | ||
} |
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 simplify this function similar to getRole in DropdownMenu
src/DropdownToggle.js
Outdated
render() { | ||
console.log(this.getRole()) |
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.
console.log(this.getRole()) |
gotcha, I'm on it! Thanks so much for all your help. This has been super educational!!! |
… and fix a typo Address all issues mentioned in PR review
Looks good, thanks! |
…listbox or menu role
Dropdown Menu should allow for listbox or menu roles. Before this commit, the Dropdown could only have the menu role. After this commit, a user can add a menuRole property to the Dropdown component which will set a corresponding aria-haspopop property on the DropdownToggle, role property on the DropdownMenu, and role property on the Dropdown Items