-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] feat(MenuItem): new prop 'roleStructure' #5377
[core] feat(MenuItem): new prop 'roleStructure' #5377
Conversation
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.
When would you use an alternative role for the <li>
? The child element is already getting role="menuitem"
.
Also what other attributes would you imagine passing along? Again I must point you to the comment I made on another PR: #5356 (comment) -- if this PR is to be merged, it must add an example usage of the new feature to either docs-app or demo-app.
The usage for an alternative role for the As you can see in all these examples: I'm not sure how this would be represented in any of the docs or examples, it's just used to add additional props if the user wants/needs to, but it doesn't affect anything functionally or visually |
@adidahiya I completely revamped this PR, changed the description. Same goal as before. Please re-review and let me know what you think. I think these changes indicate what I'm going for here. |
@adidahiya so basically our a11y improvements to Select2/MultiSelect2 (ie adding role="combobox", aria-controls="listbox", etc.) are still kindof useless until we make this a11y change. Please review and consider this PR |
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.
overall this looks good, just some minor code style concerns
* | ||
* @default "menuitem" | ||
*/ | ||
roleConfig?: "menuitem" | "listoption"; |
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.
prefer to avoid names like "config" in prop / variable names, as this is all essentially "configuration". instead we use "kind" in other places in Blueprint, and I think it could work well here:
roleConfig?: "menuitem" | "listoption"; | |
roleKind?: "menuitem" | "listoption"; |
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.
used roleStructure
, thinking that roleKind
is a little too close to having the user think that the value should be an aria role, when that's not the case.
Let me know if you'd still rather have it be roleKind
, no big deal.
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.
Alright, I don't have a strong opinion between the two options, let's go with what you have now 👍🏽
Fixes #0000
Checklist
Changes proposed in this pull request:
allow for setting MenuItem to have proper role structure for when the MenuItem is an item of a "listbox" or "select" element.
Default role structure for a MenuItem is:
which is correct when it is a child of a Menu that has
role="menu"
, which is the default role for the Menu element.However, for situations where we change the
Menu
role torole="listbox"
(such as in Select2, Suggest2, and MultiSelect2), we need a different role structure for its child MenuItems: