Skip to content

Conversation

@hanwencheng
Copy link
Contributor

I assume that it is common case that some one use options array with value started from 0:

Then we need to render clear button when the 0 value is chosen.
e.g.

[
  {value:0,label:'Aachen [Aix-la-Chapelle]',state:'NW'},
  {value:1,label:'Aalen',state:'BW'},
]

@JedWatson
Copy link
Owner

Thanks @hanwencheng!

@JedWatson JedWatson merged commit d7e4d11 into JedWatson:master Aug 30, 2016
@JedWatson
Copy link
Owner

I just realised you've applied this change to lib, I've patched it in src as well

JedWatson added a commit that referenced this pull request Aug 30, 2016
@hanwencheng
Copy link
Contributor Author

Thanks, @JedWatson I just find that the lib file has not been changed.

@kevinzwhuang
Copy link
Contributor

kevinzwhuang commented Dec 8, 2016

@JedWatson @hanwencheng

Just looking back at this PR, it doesn't seem to have changed anything nor is it necessary. The behavior without this works normally event with a value of 0.

Let's take a look at what actually changed (this exists in master) -

-		!this.props.value
+		(!this.props.value || this.props.value === 0)

Source from master: https://github.com/JedWatson/react-select/blob/master/src/Select.js#L892

means that we're checking the whole object ( this.props.value ) to see if it is 0, such that the original example is checked like so:
{value:0,label:'Aachen [Aix-la-Chapelle]',state:'NW'} === 0

This does not make sense - since the real value to check is on this.props.value[this.props.valueKey] - but we don't even need to do that, since !this.props.value should pass just fine since the object exists.

The this.props.value === 0 check does not achieve its original goal - in fact, this check already worked previously on its own. !this.props.value should never evaluate to false if such an object existed.

The original check for !this.props.value was sufficient on its own.

@hanwencheng
Copy link
Contributor Author

hanwencheng commented Dec 9, 2016

@kevinzwhuang Hi, the source code in master does not apply the origin commit

@TrevorBurnham
Copy link

PR to actually fix this: #1395

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants