Add ability to remove dropdown items on backspace #283
Conversation
Current coverage is 85.67%
|
| @@ -335,6 +340,22 @@ export default class Dropdown extends Component { | |||
| } | |||
| } | |||
|
|
|||
| removeItemOnBackspace = (e) => { | |||
| debug('remoteItemOnBackspace()') | |||
|
Thanks for the feedback so far. I'm pretty new to React and ES2015, so my code quality might not be the best :) |
|
We'll need some coverage in order to merge this. Could you write some tests? We can add a new section to describe('removing items on backspace', () => {
it('removes the last item when there is no search query', () => {
// ...
})
it('does not remove the last item when there is a search query', () => {
// ...
})
it('does not remove items for multiple dropdowns without search', () => {
//...
})
})Something like this to get started. describe.only('removing items on backspace', () => {
... |
No worries, thoroughly pleased to see this PR. I knew this feature was missing but just didn't have the time to implement it. Will gladly walk you through. |
| domEvent.keyDown(document, { key: 'Backspace' }) | ||
|
|
||
| spy.should.have.been.calledOnce() | ||
| spy.firstCall.args[1].should.deep.equal(expected) |
There was a problem hiding this comment.
Looks good, can we add a state assertion to ensure the component is being self controlled:
wrapper.should.have.state('value', expected)There was a problem hiding this comment.
Hm, I wonder if it is a deep equality issue. You could try using the getter form of state and then deep equal:
wrapper
.state('value')
.should.deep.equal(expected)If this still gives you troubles, don't worry about adding it, we can iterate on this.
There was a problem hiding this comment.
That did the trick.
|
This is lookin superb, just the one request for a state assertion on the test here https://github.com/TechnologyAdvice/stardust/pull/283#discussion_r68460171 and I think we can merge this guy. Thanks much! |
| const { multiple, search } = this.props | ||
| const { searchQuery, value } = this.state | ||
|
|
||
| if (searchQuery || !search || !multiple || !value) return |
There was a problem hiding this comment.
Found a little bug here. When there are not items left and you hit backspace, this method still calls onChange and setValue. This is because !value here is still true since it is an empty array.
Being that multiple dropdowns have arrays for value we should use _.isEmpty(value) here instead. This will be true if empty, false if there are any items in the array.
|
👻 looks great |
|
Thanks much for the PR @ianunruh. Hope to see more of your great work here. |
Fairly simple change that only affects dropdowns with
searchandmultiple. If the dropdown is focused, the search query is empty and backspace is pressed, the most recently selected item will be removed.