Add new ariakit-based DropdownMenu component
#54939
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
ciampo
added
[Type] Enhancement
|
Flaky tests detected in a5bb3e1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6639667936
|
ciampo
force-pushed
the
feat/dropdown-v2-ariakit
branch
3 times, most recently
from
ee3e5c6
to
1c6171e
Compare
ciampo
force-pushed
the
feat/dropdown-v2-ariakit
branch
from
1c6171e
to
e8138be
Compare
|
Size Change: +7.8 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
feat/dropdown-v2-ariakit
branch
2 times, most recently
from
ca1cf02
to
1725347
Compare
ciampo
commented
Oct 15, 2023
| * External dependencies | ||
| */ | ||
| import { render, screen, waitFor } from '@testing-library/react'; | ||
| import { press, click, hover, type } from '@ariakit/test'; |
There was a problem hiding this comment.
With @diegohaz we came to the conclusion that @testing-library/user-event's way of simulating user events (especially key presses) just doesn't work well with ariakit (likely it's related to flushing microtasks). Using @ariakit/test instead seems to work well and allows us to write unit tests without any hacky workarounds.
There was a problem hiding this comment.
Yes, just bear in mind that @ariakit/test isn't as stable as @ariakit/react. You can expect more breaking changes without any deprecation warnings (but it will be described in the changelog).
ciampo
requested review from
tyxla,
chad1008,
brookewp,
andrewhayward,
afercia,
alexstine,
jameskoster,
SaxonF,
jasmussen,
richtabor,
andrewserong and
aaronrobertshaw
…eyboard and uncontrolled check/radio items
ciampo
force-pushed
the
feat/dropdown-v2-ariakit
branch
from
fc8ef66
to
650152e
Compare
|
@aaronrobertshaw , apart from the more design-related feedback, I should have addressed all of your other points. Regarding design-related feedback, I will discuss further with @jameskoster and other design folks and take it from there. @alexstine , thank you for sharing feedback! A couple of questions:
|
|
Sorry, it's in one of the storybook examples. There are menu options you cannot select with the keyboard. Not sure which ones though. I am traveling this week and cannot review this further until Sunday. |
|
@alexstine , I believe I understand what you meant about items not being clickable — a few items in the slotfill example had the On a separate note, I'd still like to understand better what you meant by "Mounting the menu seems sluggish", so that I can also look at potentially improving things there. Overall, I'd like to propose that we merge this PR as-is given its size, and continue the work with smaller follow-ups as needed. What do folks think? |
diegohaz
approved these changes
Oct 25, 2023
anomiex
added a commit
to anomiex/gutenberg
that referenced
this pull request
Nov 13, 2023
In WordPress#54939 a new test was added that depends on `@ariakit/test`. But, even though that package is not needed in the production code, it was (accidentally?) added to `dependencies` instead of `devDependencies`. This means everyone using `@wordpress/components` winds up having to install this package. Let's move it to devDependencies where it belongs.
ciampo
pushed a commit
that referenced
this pull request
Nov 15, 2023
* Package `@ariakit/test` should be a dev dependency In #54939 a new test was added that depends on `@ariakit/test`. But, even though that package is not needed in the production code, it was (accidentally?) added to `dependencies` instead of `devDependencies`. This means everyone using `@wordpress/components` winds up having to install this package. Let's move it to devDependencies where it belongs. * Add PR link to changelog * Remove extraneous `.`
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 #50459
Requires #55365
What?
Add a new version of the
DropdownMenucomponent based off theariakitlibrary.Note: The component should still be considered a WIP and will be tweaked and refined either in this PR or via follow-up PRs as it receives feedback and as it gets used throughout the codebase
Why?
We found a few blockers when working on the radix-ui based new version of
DropdownMenu, and therefore we're trying a version based onariakitinsteadHow?
Next steps:
DropdownMenuandMenuItemProgress tracker
DropdownMenuDropdownMenuItem)DropdownMenuGroupDropdownMenuGroupLabel(orDropdownMenuLabel)DropdownMenuItemDropdownMenuCheckboxItemDropdownMenuRadioItemDropdownMenuSeparatordirattribute via HTMLhideOnEscape={ ( e ) => { e.stopPropagation(); return true; } }Testing Instructions
npm run distclean && npm ci && npm run storybook:devDropdownMenuV2Ariakitstorybook examplesScreenshots or screencast
(ignore the fact that menu's popover is not aligned with the trigger when Storybook's sidebar is opened, as that's a
floating-uibug not related to this PR)dropdownmenu.mp4