DataViews: set proper semantics for dropdown items #56676
Merged
Conversation
This file contains 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
oandregal
requested review from
ntsekouras,
youknowriad,
andrewhayward,
ciampo and
jorgefilipecosta
oandregal
added
[Type] Experimental
|
Size Change: +49 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Thanks for the PR! I know @ciampo has a PR for replacing the dropdown, but maybe it's better to use the new one and make improvement there if needed? |
No reason not to land this simple fix while the new dropdown is being worked on (I don't know the timeline for that, we may find issues while doing it, etc.). |
ntsekouras
approved these changes
Nov 30, 2023
ciampo
reviewed
Nov 30, 2023
There was a problem hiding this comment.
Thank you for working on this!
Rather than using DropdownMenuItem and manually adding role and aria-checked attributes, we should use the dedicated menu radio item components.
- If we're using the
DropdownMenuV2(the radix-based one, that will soon be deleted), we should use thevalue/onValueChangedprops instead of theonSelectprop (example) - If we were to use the new version of the component (based on
ariakit), we should use thecheckedandonChangeprops on each menu items (you can see how I approach a similar refactor in Refactor experimental dropdown menu usages to latest version #55625)
Hope this makes sense!
oandregal
force-pushed
the
update/menu-item-role
branch
from
fe17240 to
42b7917
Compare
|
@ciampo Thanks for the advice, that's great. There are some subtle differences between the new components and the old ones (focus, for example), so I'd rather update them all together. I've tried using the specific components in other PRs (radio, checkbox) and ran into issues (couldn't uncheck, etc.) — perhaps it was my own fault, I'm not sure, I'll revisit. Anyway, this PR also makes the e2e test more robust (searching string using exact, delete template in |
Sounds good
That is possible, and it would be because radio items are not supposed to be unchecked (see related comment). For posterity, I'm also linking to a related conversation that we've been having about the right semantics / widgets that should be probably used for these components. |
Comment on lines
+84
to
+85
| role="menuitemradio" | ||
| aria-checked={ false } |
There was a problem hiding this comment.
This is a weird one, but I'd probably argue that these should have a role of menuitem. They are not values in and of themselves; instead, they are all actions that create a new filter with a particular value.
Suggested change
| role="menuitemradio" | |
| aria-checked={ false } | |
| role="menuitem" |
Ideally we'd put this in front of actual users, but given the submenu isn't available once the filter has been created, it kind of ceases to matter what their selected state is, or whether they are mutually exclusive.
There was a problem hiding this comment.
(Looks like I was 30 seconds late with feedback 😩 Not a blocker, just something to think about.)
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.
Part of #55083
What?
Set the proper semantics for the existing menu items.
Why?
In some cases, the item was declared as behaving as a checkbox while their behavior was actually like a radio item.
How?
Set the proper
rolefor everyDropdownMenuItem.