Support controlling open/closed state for Dropdown and DropdownMenu #54257
Conversation
|
Unit tests are broken, so we;d need to look into that and see if this approach is still feasible without introducing breaking changes. |
| useEffect( | ||
| () => () => { | ||
| if ( onToggle && isOpen ) { | ||
| onToggle( false ); | ||
| } | ||
| }, | ||
| [ onToggle, isOpen ] | ||
| ); |
There was a problem hiding this comment.
Nice one, this is the removal that I was missing from my experiment 🎉
There was a problem hiding this comment.
Nice one, this PR unblocks #54083 for me!
Unit tests are broken, so we;d need to look into that and see if this approach is still feasible without introducing breaking changes.
I've pulled in most of the changes here into #54083 where the unit tests are passing. Are they failing in this PR because of the temporarily commented out close() call in closeIfFocusOutside()?
Edit: the unit tests are passing over in #54083, but the e2es are failing, so there's likely still a bit to fix up. Still, I'm feeling confident about the direction, now!
Edit 2: I think I found the source of the e2e failures on the other PR #54083 — turns out I needed to explicitly clear the state in a couple of places, and in one e2e wait for the menu to be open before firing a keyboard shortcut. So, all up, looking pretty good so far 🙂
|
Cool! In that case, we can close this PR as it served its purpose :) |
|
Just seen #54083 (comment) — I'm going to reopen, so that we can use this PR to make changes to the components and keep the rest of the change sin #54083 |
ciampo
changed the title
ciampo
force-pushed
the
feat/dropdown-controlled
branch
2 times, most recently
from
15b31be to
6ea9b0a
Compare
ciampo
force-pushed
the
feat/dropdown-controlled
branch
from
6ea9b0a to
ab6e115
Compare
| @@ -2,16 +2,13 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Breaking changes | |||
There was a problem hiding this comment.
Moved this entry to the previous release section, since I realised that the corresponding PR was merged before August 31st
There was a problem hiding this comment.
Thanks again for all the work here, this looks great to me!
✅ The structure of the new controlled props matches what I need to get #54083 working correctly
✅ Code looks good, and documentation changes read well to me
✅ Smoke tested the Dropdown and DropdownMenu components in Storybook and across usage in the post and site editors
LGTM! ✨
ciampo
added
the
has dev note
What?
Add support to control the open state of
DropdownandDropdownMenucomponentsWhy?
To give more choice to consumers of the components on how to use such components (see for example #54083)
How?
By adding
open/onToggle/defaultOpenprops, using theuseControlledValuehook and refactoring as needed.Testing Instructions
DropdownandDropdownMenuin controlled mode (see d8379d9 and 2d35f36 to see the required changes)npm run storybook:dev) and make sure that the components continue to work as expected✍️ Dev note
The open/closed state of the
DropdownandDropdownMenucomponents can now be controlled by their consumers via theopen,onToggleanddefaultOpenprops, allowing more flexibility and more advanced behaviors when using these components.