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
DataViews: set proper role for AddFilter's items #56714
Conversation
@@ -81,8 +81,6 @@ export default function AddFilter( { fields, view, onChangeView } ) { | |||
{ filter.elements.map( ( element ) => ( | |||
<DropdownMenuItem | |||
key={ element.value } | |||
role="menuitemradio" |
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.
@andrewhayward Given the component is a DropdownMenuItem
, do I need to set the role explicitly 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.
The e2e test are passing, which means they are able to pick up the menuitem
role for this component.
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 you dig all the way down, it looks like the default role is menuitem
, so no, we don't need to provide it here. There's an argument for being explicit, but I don't think it's really necessary given the component name.
Size Change: -2 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
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.
Looks good to me 🚀
Part of #55083
Follow-up to #56676 (comment)