-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Dropdown: add focusOnMount prop to pass onto Popover component #12855
Dropdown: add focusOnMount prop to pass onto Popover component #12855
Conversation
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.
My first thought is that the example provided seems a bit confusing; the popover is displayed on... focus of the input but the keyboard isn't moved to it. I guess I'm a bit lost as to how that works for keyboard users having not tested it and just having glanced at the code.
But I'm fine with exposing this property. I think you'll want to update the package's CHANGELOG though as this is an API change and we'll need to publish a new version of it. Our packages are handled via lerna and there are old PRs you can reference for this, but let me know if the package updates aren't clear 😅
5e36710
to
7b6164f
Compare
Thanks for the review @tofumatt !
I added a gif and explanation to illustrate how it works for keyboard users. By avoiding bringing focus to the popover contents, keyboard users aren't taken out of a form's sequence and can use the input as intended. I also added to the CHANGELOG with a minor bump (new backwards-compatible feature) |
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.
Ah okay, I getcha. Looks good then; it would probably be good to have an end-to-end test to verify tabbing works as expected here, but if you want to do that in a follow-up PR that's okay with me 😄
(You'll need to fix the conflict in the CHANGELOG
but then it's good to merge, sorry I didn't get to this for forever, I think I missed it in my issue queue. My bad.)
Would you mind rebasing this? |
a46ec07
to
8175d62
Compare
I think the error in the unit tests here is already fixed in master and will be gone if we rebase this branch. |
8175d62
to
241b081
Compare
Thanks @youknowriad ! Another rebase did the trick, thanks for taking a look. |
Description
I'd like to use the functionality of the
<Dropdown />
component, but not bring focus to the dropdown as it gets mounted by making use of<Popover />
propfocusOnMount
.This PR adds a
focusOnMount
prop to<Dropdown />
and simple passes it down to<Popover />
.How has this been tested?
I've tested via UI implementation, woocommerce/woocommerce-admin#1069. Changes in this PR will not affect other areas of the codebase.
Screenshots
Use case involving keyboard Interactions
The option to have the popover NOT take focus on mounting is useful in the case of the
<Dropdown />
button being an input. The contents of the<Popover />
might be for information purposes, or in this case, provide additional methods of date selection beyond an input.As seen below, tabbing to the input opens the popover, but by not stealing focus, allows keyboard-only users to remain in the input with focus.
Types of changes
New feature: A non-breaking change that exposes
<Popover />
's functionality.Checklist: