feat(Dropdown): add prop for clearing searchQuery data on item select or on enter #1955
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1955 +/- ##
=========================================
- Coverage 99.8% 99.8% -0.01%
=========================================
Files 148 148
Lines 2589 2568 -21
=========================================
- Hits 2584 2563 -21
Misses 5 5
Continue to review full report at Codecov.
|
src/modules/Dropdown/Dropdown.d.ts
Outdated
| @@ -216,6 +216,9 @@ export interface DropdownProps { | |||
| /** A shorthand for a search input. */ | |||
| searchInput?: any; | |||
|
|
|||
| /** Whether the searchQuery is cleared or not when selecting an item from search. */ | |||
| searchQueryClearOnSelect?: boolean; | |||
There was a problem hiding this comment.
Nit: Could we go with clearSearchQueryOnSelect?
There was a problem hiding this comment.
not a nit at all, was hoping for some specific feedback on the naming anyway. i just put it near the search ones as it felt grouped, but really was unsure about naming convention. will switch to your suggestion.
There was a problem hiding this comment.
I've requested a prop name change above. We just need some tests for this prop:
describe('clearSearchQueryOnSelect', () => {
it('does not clear the query by default on item click', () => {})
it('does not clear the query by default on enter', () => {})
it('clears the query on item click', () => {})
it('clears the query on enter', () => {})
})
layershifter
changed the title
layershifter
changed the title
|
thanks for the feedback. will add the tests and prop name change. |
|
okay should be good to go! |
layershifter
added
ready for review
enhancement
and removed
needs author feedback
labels
| .simulate('click', nativeEvent) | ||
|
|
||
| // cleared search query | ||
| wrapper.should.have.state('searchQuery', searchQuery) |
There was a problem hiding this comment.
Comment needs updated, or removed :)
| domEvent.keyDown(document, { key: 'Enter' }) | ||
|
|
||
| // cleared search query | ||
| wrapper.should.have.state('searchQuery', searchQuery) |
|
Great taken care of. Sorry about the bad comments in the tests. Whoops. |
|
I think that the other PR that was merged probably solves for the use case, where the search query should be cleared if it is the available options === 1. I am likely going to close this, unless you feel that it still makes sense to do. |
|
Fewer options in the Dropdown, the happy I am 😄! This component is quite complex and getting hairy to work on. Assuming we're all good here, I'll take your suggestion and close this. Let me know if we need to reopen it. |
I wanted to add a prop to the dropdown where the search query could be cleared after selection on the multiple select, if desired.
This maintains the current behavior of how everything works today, but if the prop is added then the search query will clear after item selection.
I asked this question on issue #1954 but just figured it was easy enough to PR before waiting for an answer.