-
Notifications
You must be signed in to change notification settings - Fork 55
fix(dropdown): fix tab selection and blur reset #1118
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.
The changes looks good, but I would suggest adding some unit tests for this. Also, please update the changelog as well!
Codecov Report
@@ Coverage Diff @@
## master #1118 +/- ##
==========================================
- Coverage 82.05% 81.99% -0.06%
==========================================
Files 724 724
Lines 8619 8626 +7
Branches 1234 1236 +2
==========================================
+ Hits 7072 7073 +1
- Misses 1530 1536 +6
Partials 17 17
Continue to review full report at Codecov.
|
@@ -185,6 +185,9 @@ export interface DropdownProps extends UIComponentProps<DropdownProps, DropdownS | |||
/** Sets search query value (controlled mode). */ | |||
searchQuery?: string | |||
|
|||
/** When selecting an element with Tab, focus stays on the dropdown by default. If true, the focus will jump to next/previous element in DOM. */ | |||
tabInMultipleSelection?: boolean |
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.
tabInMultipleSelection
is not very descriptive; what about
moveFocusOnTab
moveFocusOnTabKey
moveFocusOnTabPress
moveFocusOnTabKeyPress
Or use other verbs instead of move
, like release
or yield
.
What do you think?
@mnajdova @silviuavram
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.
forgot this is something that needs to get in so I'm not blocking this PR, but pls consider the comments, especially the ones concerning changing the name of the props as we want to avoid having breaking changes
forgot this is something that needs to get in so I'm not blocking this PR, but pls consider the comments, especially the ones concerning changing the name of the props as we want to avoid having breaking changes
bfdbee7
to
3e72de7
Compare
This PR aims to fix 2 issues:
tabInMultipleSelection
is set to true.These 2 issues should also be addressed in Downshift but we need them now, so will patch them now like this and will follow-up in Downshift later. Downshift should make sure that selection on blur works (now it just resets on blur). Also I am planning to add multiple selection to Downshift in the future.
Please provide input about implementation and naming.
Should close #1101.