react-native-picker-select icon indicating copy to clipboard operation
react-native-picker-select copied to clipboard

Test undefinedness of value in getSelectedItem

Open Bigood opened this issue 4 years ago • 3 comments

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

Bigood avatar Mar 05 '20 09:03 Bigood

not against it, but a few questions:

  1. why use itemKey to select a default -- value will handle that
  2. why use itemKey in your example? all values are unique.
  3. what about just updating the docs to make it clearer that overriding the placeholder requires you to include a value and label?

lfkwtz avatar Mar 05 '20 13:03 lfkwtz

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 :)

Bigood avatar Mar 10 '20 16:03 Bigood

Similar situation here @Bigood.

Any news about this PR?

gsevla avatar Jul 29 '20 14:07 gsevla