fix picker multi value #1649
fix picker multi value #1649
Conversation
src/components/picker/index.js
Outdated
|
|
||
| if (renderCustomModal) { | ||
| const modalProps = { | ||
| visible: showExpandableModal, | ||
| toggleModal: this.toggleExpandableModal, | ||
| onSearchChange: this.onSearchChange, | ||
| children, | ||
| onDone: () => this.onDoneSelecting(value), | ||
| onDone: () => this.onDoneSelecting(multiDraftValue), |
There was a problem hiding this comment.
Why is this multiDraftValue for all types? It seems to be working, but the code is not clear (it is not actually used in single mode). There are other places set up this way.
Not sure we want to "fix" this, did you talk about it with Ethan?
There was a problem hiding this comment.
What do you mean by "all types"?
This state value is not used in single mode, therefore its name is multiDraftValue
There was a problem hiding this comment.
That's what I've written, but will a future developer know that? - I feel the code is (still) unclear about the fact it's only for MULTI.
There was a problem hiding this comment.
I don't have a simple solution, maybe adding a comment for now.
There was a problem hiding this comment.
Approved, merge with\without adding a comment.
There was a problem hiding this comment.
@lidord-wix
I agree this specific use case worth a comment.
Let's add a comment and merge
ethanshar
added
the
Important for Next Release
This reverts commit 8dc646f.
Description
Fix bug with picker, now the single picker value is controlled by the user (with the value prop) and the multi picker is controlled by the user (with the value prop) and controlled by the component (with multiDraftValue state and multiFinalValue)
added TODO task to add an initialValue prop in the future to make it more clear to the user in multi mode.
Fix #1615
Changelog
Picker - fix multi mode value