Test undefinedness of value in getSelectedItem#286
Open
Bigood wants to merge 3 commits intolawnstarter:masterfrom
Open
Test undefinedness of value in getSelectedItem#286Bigood wants to merge 3 commits intolawnstarter:masterfrom
Bigood wants to merge 3 commits intolawnstarter:masterfrom
Conversation
Using itemKey to select a default value, if using a placeholder without a value (which is intended), internal method getSelectedItem fails as it doesn't test the presence of an item.value before calling isEqual, and in absence of a value prop on Picker, will resolve as isEqual(undefined, undefined).
Thus, this code won't work, but should IMO:
const items = [
{ key: 'a', label: 'a', value: 'a' },
{ key: 'b', label: 'b', value: 'b' },
{ key: 'c', label: 'c', value: 'c' },
]
...
<Picker
placeholder={{
label: "Select a value…"
}}
items={items}
itemKey={"a"}
/>
A simple non-breaking change in getSelectedItem will correct this, as proposed in this pull request.
See this snack for a most comprehensive demonstration of this unexpected behavior: https://snack.expo.io/rJm7QrAEU
Contributor
|
not against it, but a few questions:
|
Author
|
In my real situation, I have values that are mobx-state-tree objects snapshots, and I use their IDs as itemKeys. To provide a default value, it makes more sense to me to use their IDs, versus snapshoting my current object and do a shallow comparison with values, which will be more costly. As for your third question, that might be an option, cause I don't see how forcing to give a placeholder a value to be able to use itemKey is intuitive. But I'd rather see this patched than this weird behavior explained :) |
|
Similar situation here @Bigood. Any news about this PR? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Using itemKey to select a default value, if using a placeholder without a value (which is intended), internal method getSelectedItem fails as it doesn't test the presence of an item.value before calling isEqual, and in absence of a value prop on Picker, will resolve as isEqual(undefined, undefined).
Thus, this code won't work, but should IMO:
A simple non-breaking change in getSelectedItem will correct this, as proposed in this pull request.
See this snack for a most comprehensive demonstration of this unexpected behavior: https://snack.expo.io/rJm7QrAEU