Setting the selectedItem to '' does not clear the inputValue
yp opened this issue · 12 comments
downshiftversion:1.16.0nodeversion:npm(oryarn) version:
What you did:
I have a component that lists several items using several "Downshift-enriched" inputs. At the end of the list there is an empty Downshift-enriched input for adding a new item to the list. As such the components are semi-controlled, in the sense that the selectedItem property is set by the parent component.
What happened:
If I select a new item using the last input component I would expect that a new empty input component appears after the last one.
However, it appears a new input component where the input value is the one selected before.
Reproduction repository:
https://codesandbox.io/s/k5px1z0075
Select a fruit (apple for example) in the only input that appears and see that two components have the same fruit (instead of being one selected and the other one empty).
Problem description:
Maybe setting to null or '' the selectedItem prop does not correctly clear the internal state.
I'm not 100% certain, but I'm thinking that perhaps something's preventing the second autocomplete from re-rendering. Could you do a little more digging?
I think that the component re-renders. Indeed the componentDidUpdate fires. However, inputValue is not cleared sing the selectedItem prop does not change (was '' before, it stays '' after).
If selectedItem is a controlled prop, does it make sense to clear/reset inputValue after a selection is made? As far as I can tell, the componentDidUpdate should then correctly set the inputValue to the string representation of selectedItem (if selectedItem changes).
It should be done inside the component, since I don't think there's an API for clearing inputValue.
If selectedItem is a controlled prop, does it make sense to clear/reset inputValue after a selection is made?
I think it makes more sense that inputValue should be updated anytime there's an update to selectedItem (and it should be updated by calling itemToString). I thought that's what happens. Could you figure out why that's not happening? Thanks!
I think it is related to this line: https://github.com/paypal/downshift/blob/936c8e053dbb2be4b768ad0fd0b7bce21afb4e74/src/downshift.js#L241
in function selectItem.
It seems that Downshift assumes that item will be the "next" selected item, while, when selectedItem is a controlled prop, that assumption may not be true.
Ahhh, yeah, that's tricky... Hmmm... I'm not sure how to solve this honestly. A workaround for you could be to control the inputValue as well. I think that's probably the best course of action actually. Unless you have another suggestion.
Yes, it is a possibility.
Another possibility is to set inputValue using the itemToString prop if selectedItem is not controlled or to '' otherwise. What do you think?
Yeah, I'd considered that. So now we need to make a decision between two "defaults" when you control the selectedItem. In a decision like this, we need to decide which is a more common scenario:
- I'm controlling
selectedItemso I can set it externally, but I still want the user to change the selected item themselves and I want the input value to reflect that change. - I'm controlling
selectedItemso I can set it externally, and if the user changes the selected item, I don't want the item they selected to change the input value.
I'd say that scenario 1 is more common. So I think that we'd be better off leaving things as they are and forcing folks in scenario 2 to control the inputValue.
I see. Please take all the rest of this comment as my respectful opinion.
For better consistency of the design, scenario 1 means that selectedItem is not really a controlled prop since it assumes that the next selectedItem will be exactly the one resulting from the selection. The solution I was proposing has the benefit that if the controlled prop selectedItem changes, then the inputValue will be updated in componentDidUpdate by this line: https://github.com/paypal/downshift/blob/936c8e053dbb2be4b768ad0fd0b7bce21afb4e74/src/downshift.js#L749
Hence the behavior perceived by the user is that of scenario 1, but without forcing to control the inputValue. (BTW, the current implementation resets the inputValue twice.)
Please close the issue if you prefer to leave things as-is.
Ah, so you're saying if we make the suggested change, we'll update the inputValue anyway because of componentDidUpdate?
By code inspection, I would say yes. But I did not check.
If you would be willing, I would appreciate a pull request that demonstrates this approach in source, tests, and a new example. Could you do that? Thanks so much for iterating with me on this!
Just ran into this same problem, and happy to find you guys already figured it out! This is what I ❤️ about open-source. Thanks for all your hard work!