Refactor experimental dropdown menu usages to latest version #55625
Conversation
ciampo
added
[Feature] UI Components
|
Size Change: +91 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
| // Note: there is currently a limitation from the DropdownMenu | ||
| // component where the radio won't unselect when all related | ||
| // radios are set to false. | ||
| checked={ isChecked } |
There was a problem hiding this comment.
Just flagging that currently it looks like DropdownMenuRadioItem is not able to support unselecting a previously selected radio item. I'm currently looking into it.
There was a problem hiding this comment.
Ok, so I looked a bit more into this, specifically related to the sorting options.
data-view-sorting-dropdown-trunk.mp4
Here are some thoughts:
- I'm not sure that using checkboxes (like currently on
trunk) is a great idea, since users don't expect that checking a checkbox will un-check another checkbox; - using radio seems to be the right choice since we're talking about selecting mutually exclusive options
- however, radio buttons are not really supposed to be de-selectable once selected (see for example this conversation). This also seems to apply to elements with the
menuitemradiorole — for example, see how the expected list of interactions on MDN never mentions unchecking the currently checked item.
A better approach that could improve semantics, accessibility, and ultimately UX may be to add an extra option labeled something like "None" or "Default" (or anything else that conveys the notion to the user that selecting it will remove the ascending/descending sorting). This approach is also used in the ARIA Authoring Practices Guide menubar example:
There was a problem hiding this comment.
Might want to get a design check here. @SaxonF @jameskoster
There was a problem hiding this comment.
Also noting that this implementation changes the design a bit by always keeping the check as a prefix. This affects the sorting submenu.
So that would also need some design input.
There was a problem hiding this comment.
We received design input and I'm in the process of applying it to the component (#56041). After that (and a couple of other PRs) are merged, I should be able to rebase and resume the work on this PR :)
| ); | ||
| } | ||
|
|
||
| export default function ViewActions( { fields, view, onChangeView } ) { | ||
| return ( | ||
| <DropdownMenuV2 | ||
| label={ __( 'Actions' ) } |
There was a problem hiding this comment.
This label prop wasn't actually doing anything. The correct place for it is on the Button component used as a trigger
| const isActive = | ||
| currentSortedField && | ||
| const isChecked = | ||
| currentSortedField !== undefined && |
There was a problem hiding this comment.
Adding an explicit !== undefined check to make sure that this variable is always a boolean (previously, it could also be undefined)
| // We need to handle this on DropDown component probably.. | ||
| event.preventDefault(); |
There was a problem hiding this comment.
While there is a new hideOnClick prop on the new version of DropdownMenuItem, using the DropdownMenuRadioItem component automatically keeps the menu open when clicked
|
I see on your screenshots that the active element is represented by a dot, in my testing (safari) I don't see any. 
Also, I wonder if checkboxes are better design wise. I understand that these are more radio buttons and we want to differentiate to clarify the behavior but they look a bit ugly to me :P |
|
I was trying keyboard navigation within the dropdown menus, for nested dropdown menus (sorting) I was able to go back and forth between levels using arrow keys (left/right) but I was not able to go back to the top level. Also I kind of expected "Escape" to close the menu but in Safari, the window also exits full screen mode, so I think we need a "preventDefault" somewhere. |
| DropdownMenuGroupV2Ariakit, | ||
| DropdownMenuItemV2Ariakit, | ||
| DropdownMenuRadioItemV2Ariakit, | ||
| DropdownMenuCheckboxItemV2Ariakit, |
There was a problem hiding this comment.
It feels a bit weird to me that these components have Ariakit in their names, I understand that they're still private so we can change.
There was a problem hiding this comment.
Absolutely 💯 I plan on renaming those components once the radix-ui version of the dropdownmenu gets removed
|
Thank you for the feedback @youknowriad ! This component is still a bit WIP and PRs like this one can be really helpful in understanding what bugs are left and what tweaks we should apply at this early stage.
I'll take a look, thank you!
The dot is also WIP and needs design validation. I initially went with it since it seemed to me the standard way of representing radio menu items, and I also wanted a way to visually differentiate it from checkboxes since they behave differently. But I'm definitely open to design feedback!
I will also look into this. Out of curiosity, can you also replicate it when the component is rendered in isolation (like in the Storybook example) ?
I'll look into this as well, we may indeed need a |
No, arrow navigation works well on that link. |
Thank you for spotting. I've opened #55964 with the fix.
There should be an updated design spec of the component coming soon, so I guess I'll make any related changes when that comes :) That should also help with this conversation
I see what you mean. Here's what I think is happening:
To sum it up: Ariakit assigns arrow keys based on the For now, in order to at leave have a coherent behaviour when navigating this instance of the menu with keyboard, I've added the I wonder if, at the Ariakit level, the
Well spotted. I opened a separate PR to address this at the component level: #55962 |
|
Thanks for the PR @ciampo! Besides some design input that is needed and a rebase, do you think there are other blockers? I think it would be good to start using the new component. --cc @WordPress/gutenberg-design |
I'd say that we should be able to start using the component after #56041 is merged. I also have a few other open PRs affecting the component (#56213, #56342, #56400) but they shouldn't be blockers for this PR. Being able to use the new component will actually be great as a way to gather lots of feedback and make any additional fixes / tweaks |
|
Marko, this is not the refactor you are waiting for, though I hope that it makes things easier when time comes #56503 This stems from me looking at some consolidation, and realizing we still use the V2 suffix in most places but not all ( |
ciampo
force-pushed
the
refactor/dropdown-v2-radix-to-ariakit
branch
from
73c6759
to
79bff91
Compare
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { unlock } from './lock-unlock'; | ||
| import { VIEW_LAYOUTS, LAYOUT_TABLE } from './constants'; | ||
| import { VIEW_LAYOUTS, LAYOUT_TABLE, SORTING_DIRECTIONS } from './constants'; | ||
| import { DropdownMenuRadioItemCustom } from './dropdown-menu-helper'; |
There was a problem hiding this comment.
@ciampo do we have a v2 radio item we can use here? As far as my understanding goes, we only need a custom radio for cases where we need to be able to uncheck a checked option (only filters that I'm aware of). It's not the case in any of these components.
| isSorted && | ||
| view.sort.direction === direction; | ||
| return ( | ||
| <DropdownMenuItem | ||
| <DropdownMenuRadioItemCustom |
There was a problem hiding this comment.
Same comment as https://github.com/WordPress/gutenberg/pull/55625/files#r1437560831
Why do we need a custom radio here? Since the sorting semantics changed at #56717 we don't need to "uncheck" a checked option.
There was a problem hiding this comment.
Using normal radios would originally cause the bug discussed in this comment for the sorting options
In terms of functionality I spotted a small issue. If you sort by one field, then sort by another, the original sorting remains selected in the menu. See the screenshot below where I sorted by title, then by author. The menu tells me the data is still sorted by title when it is not
That is a consequence of using "real" menu radio items — the value in the radio group can not be reset to having no items selected.
For the time being, I extracted a "custom menu radio item" component based on the existing code, and reverted all menu items to using the custom implementation which should allow for deselecting items (
86748ca(#55625)).
But I think I found a way to still use the actual radio component, testing it in #57505.
What?
Refactor existing usages of
DropdownMenuV2to the newest version of the component. The plan is to delete that older version after this PR is merged (#55626)Why?
There is a new version of the experimental
DropdownMenu"v2" component supposed to replace the previous experimental version.How?
Most refactors were a 1:1 swap between the old and the new version of the component. A few exceptions:
DropdownMenuandDropdownMenuItemand they will work in nested menus automaticallyDropdownMenuItemwere actually fulfilling a radio item role (without the need for "de-selecting" an item by clicking it again), and therefore I refactored them to theDropdownMenuRadioItemcomponent (which removes the need to explicitly add radio semantics and show a check icon)suffixin<span aria-hidden="true" />to hide from assistive technologyTesting Instructions
Screenshots or screencast