Fix #1394 #1395

sgaestel · 2016-11-29T09:13:59Z

This way, the X clear button won't be displayed only for null or undefined values, allowing to use 0 or false as clearable values in select options.

coveralls · 2016-11-29T09:16:31Z

Coverage remained the same at 92.308% when pulling 117f81e on sgaestel:master into ef3a468 on JedWatson:master.

atatakobry · 2016-12-08T10:12:56Z

src/Select.js

@@ -889,7 +889,7 @@ const Select = React.createClass({
 	},

 	renderClear () {
-		if (!this.props.clearable || (!this.props.value || this.props.value === 0) || (this.props.multi && !this.props.value.length) || this.props.disabled || this.props.isLoading) return;
+		if (!this.props.clearable || this.props.value == null || this.props.value === 0 || this.props.multi && !this.props.value.length || this.props.disabled || this.props.isLoading) return;


and what about this.props.value === 0?
sometimes we have to use zero value and clear button is not displayed (

Yes, I guess we could remove this condition, and not display the clear button only when this.props.value is null or undefined

src and tests updated 😉

coveralls · 2016-12-08T10:44:01Z

Coverage remained the same at 92.308% when pulling 919c952 on sgaestel:master into ef3a468 on JedWatson:master.

coveralls · 2017-02-01T08:42:20Z

Coverage remained the same at 92.634% when pulling 98e11d0 on sgaestel:master into 63a59dc on JedWatson:master.

sgaestel · 2017-02-01T08:45:34Z

@JedWatson Any chance for this PR to be merged soon ?

JedWatson

Thanks for this @sgaestel, and cheers for bumping it as well. I'd be very happy to merge this, could you fix the loose equality operator though please?

JedWatson · 2017-02-02T01:30:21Z

src/Select.js

@@ -901,7 +901,8 @@ const Select = React.createClass({
 	},

 	renderClear () {
-		if (!this.props.clearable || (!this.props.value || this.props.value === 0) || (this.props.multi && !this.props.value.length) || this.props.disabled || this.props.isLoading) return;
+
+		if (!this.props.clearable || this.props.value == null || this.props.multi && !this.props.value.length || this.props.disabled || this.props.isLoading) return;


I'm not keen on the loose equality operator - I know how it works, but given the number of people who use / contribute to this project, without an explicit comment it could be misinterpreted.

How about we replace this with a more explicit value === undefined || value === null so that the intended behaviour is explicit?

✅ Done
Replaced as proposed by this.props.value === undefined || this.props.value === null

JedWatson · 2017-02-02T01:30:42Z

test/Select-test.js

@@ -691,6 +691,11 @@ describe('Select', () => {
 			expect(onChange, 'was called with', 0);
 		});

+        it('displays the X button for 0 value', () => {


Thanks for also adding tests for this 😁

You're welcome 😉

coveralls · 2017-02-02T16:45:53Z

Coverage remained the same at 92.634% when pulling 0b2f208 on sgaestel:master into 63a59dc on JedWatson:master.

TrevorBurnham · 2017-03-31T18:30:46Z

src/Select.js

@@ -901,7 +901,8 @@ const Select = React.createClass({
 	},

 	renderClear () {
-		if (!this.props.clearable || (!this.props.value || this.props.value === 0) || (this.props.multi && !this.props.value.length) || this.props.disabled || this.props.isLoading) return;
+
+		if (!this.props.clearable || this.props.value === undefined || this.props.value === null || this.props.multi && !this.props.value.length || this.props.disabled || this.props.isLoading) return;


One small tweak: You could write this.props.value == null instead of this.props.value === undefined || this.props.value === null.

> undefined == null true

TrevorBurnham · 2017-03-31T18:31:30Z

I'd love to see this merged. You can see this bug right on http://jedwatson.github.io/react-select/:

agirton · 2017-05-17T16:22:10Z

Hi @sgaestel this looks good to me, thanks again for your patience and the tests!

@JedWatson

* Fix JedWatson#1394 * Updates following review * Updates following review by @JedWatson

@JedWatson

commit 07dc061 Author: cbergmiller <christian@bergmiller.de> Date: Tue Jun 6 07:04:20 2017 +0200 [ADD] Clarified the onInputChange prop signature in the docs (JedWatson#1773) * [ADD] Clarified the onInputChange prop signature in the docs * [ADD] requested changes commit 40db517 Author: cbergmiller <christian@bergmiller.de> Date: Sat Jun 3 15:53:45 2017 +0200 [FIX] JedWatson#1651 moved option prop sync to componentWillReceiveProps (JedWatson#1765) * [FIX] JedWatson#1651 moved option prop sync to componentWillReceiveProps * [ADD] test for option prop sync by componentWillReceiveProps commit 1d15068 Author: Jed Watson <jed.watson@me.com> Date: Thu May 25 00:25:19 2017 +1000 v1.0.0-rc.5 commit e06ea57 Author: Jed Watson <jed.watson@me.com> Date: Thu May 25 00:25:09 2017 +1000 Updating build commit 7013005 Merge: 9943711 8239201 Author: Jed Watson <jed.watson@me.com> Date: Thu May 25 00:19:49 2017 +1000 Merge pull request JedWatson#1738 from agirton/chore/HISTORY-update Update change log for rc5 release. commit 8239201 Author: Adam Girton <adam.girton@me.com> Date: Sat May 20 10:37:18 2017 -0700 Update changelog commit 9943711 Author: Benjamin Piouffle <Betree@users.noreply.github.com> Date: Fri May 19 07:21:40 2017 +1100 Fix selected option focus when valueKey is not "value" (JedWatson#1733) * Fix selected option focus when valueKey is not "value" * Revert "Fix selected option focus when valueKey is not "value"" This reverts commit 45f5665. * Fix on src/ instead of lib/ commit 196c7bb Author: Max Hubenthal <Max_Hubenthal@carmax.com> Date: Wed May 17 12:47:29 2017 -0400 Fix aria owns value (JedWatson#1556) commit fd19b91 Author: Simon Gaestel <simon.gaestel+git@gmail.com> Date: Wed May 17 18:22:34 2017 +0200 Fix JedWatson#1394 (JedWatson#1395) * Fix JedWatson#1394 * Updates following review * Updates following review by @JedWatson commit 549d20a Author: Jed Watson <jed.watson@me.com> Date: Mon May 15 00:00:05 2017 +1000 v1.0.0-rc.4 commit 5269a85 Merge: af8f58d 75d6372 Author: Jed Watson <jed.watson@me.com> Date: Sun May 14 23:31:30 2017 +1000 Merge pull request JedWatson#1652 from dan-diaz/isClearable Include a className of 'is-clearable' if clearable is true commit af8f58d Merge: 8ff4596 9150107 Author: Jed Watson <jed.watson@me.com> Date: Sun May 14 23:27:19 2017 +1000 Merge pull request JedWatson#1659 from lagun4ik/master Added variables for shadow styles commit 8ff4596 Merge: a0e5855 03b3da2 Author: Jed Watson <jed.watson@me.com> Date: Sun May 14 23:25:48 2017 +1000 Merge pull request JedWatson#1658 from agirton/migrate-to-external-pkgs Migrate to prop-types and create-react-class packages commit 03b3da2 Author: Adam Girton <adam.girton@me.com> Date: Sun Apr 30 20:41:54 2017 -0700 Bumping create-react-class and prop-types commit 387f241 Author: Adam Girton <adam.girton@me.com> Date: Sun Apr 30 20:35:55 2017 -0700 Update react-input-autosize dep commit 9150107 Author: lagun4ik <ivan@lagunovsky.com> Date: Mon Apr 10 21:55:48 2017 +0300 added variables for shadow styles commit d7d2a79 Author: Adam Girton <adam.girton@me.com> Date: Mon Apr 10 09:55:46 2017 -0700 Move over examples to use new pkgs commit feabc4d Author: Adam Girton <adam.girton@me.com> Date: Mon Apr 10 09:55:17 2017 -0700 Migrate rest of src overto createClass pkg commit ace3e08 Author: Adam Girton <adam.girton@me.com> Date: Mon Apr 10 09:44:54 2017 -0700 Migrate src over to creatClass package commit 16ff8e9 Author: Adam Girton <adam.girton@me.com> Date: Mon Apr 10 09:38:37 2017 -0700 Use new prop-types package commit 75d6372 Author: DDiaz <daniel.diaz@isobar.com> Date: Thu Apr 6 10:39:10 2017 -0400 spaces to tabs commit c81b9c9 Author: DDiaz <daniel.diaz@isobar.com> Date: Thu Apr 6 10:37:15 2017 -0400 Include a className of 'is-clearable' if clearable is true

…ate_and_add_creatable_options * commit '26169305a721ec3099a912cea2f6ed38e6dc9c4c': (36 commits) Fix Usage Docs Example (JedWatson#1799) Hide create option after closing menu (JedWatson#1306) Update README.md to include 'valueComponent' prop (JedWatson#1803) Adding merged changes to changelog Fix backspace handling for non-multi select controls. Fixes JedWatson#638 (JedWatson#773) Update Select.js [ADD] Clarified the onInputChange prop signature in the docs (JedWatson#1773) [FIX] JedWatson#1651 moved option prop sync to componentWillReceiveProps (JedWatson#1765) v1.0.0-rc.5 Updating build Update changelog Fix selected option focus when valueKey is not "value" (JedWatson#1733) Fix aria owns value (JedWatson#1556) Fix JedWatson#1394 (JedWatson#1395) v1.0.0-rc.4 Bumping create-react-class and prop-types Update react-input-autosize dep added variables for shadow styles Move over examples to use new pkgs Migrate rest of src overto createClass pkg ... # Conflicts: # dist/react-select.js # dist/react-select.min.js # examples/dist/bundle.js # examples/dist/standalone.js # lib/Async.js # lib/Value.js

Fix JedWatson#1394

117f81e

atatakobry reviewed Dec 8, 2016

View reviewed changes

Updates following review

919c952

Merge branch 'master' into master

98e11d0

JedWatson requested changes Feb 2, 2017

View reviewed changes

Updates following review by @JedWatson

0b2f208

sgaestel mentioned this pull request Feb 20, 2017

Unable to clear selection if value is boolean false #1394

Closed

TrevorBurnham mentioned this pull request Mar 31, 2017

fix value check in renderClear #1146

Merged

TrevorBurnham reviewed Mar 31, 2017

View reviewed changes

agirton mentioned this pull request May 17, 2017

Clearable true doesn't work correctly with value === 0 #1729

Closed

agirton merged commit fd19b91 into JedWatson:master May 17, 2017

billyvg pushed a commit to billyvg/react-select that referenced this pull request May 25, 2017

Fix JedWatson#1394 (JedWatson#1395)

7db9b14

* Fix JedWatson#1394 * Updates following review * Updates following review by @JedWatson

gwyneplaine mentioned this pull request Oct 20, 2017

handle options with value 0 properly #1310

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1394 #1395

Fix #1394 #1395

sgaestel commented Nov 29, 2016

coveralls commented Nov 29, 2016

atatakobry Dec 8, 2016

sgaestel Dec 8, 2016

sgaestel Dec 8, 2016

coveralls commented Dec 8, 2016

coveralls commented Feb 1, 2017

sgaestel commented Feb 1, 2017

JedWatson left a comment

JedWatson Feb 2, 2017

sgaestel Feb 2, 2017

JedWatson Feb 2, 2017

sgaestel Feb 2, 2017

coveralls commented Feb 2, 2017

TrevorBurnham Mar 31, 2017

TrevorBurnham commented Mar 31, 2017

agirton commented May 17, 2017

Fix #1394 #1395

Fix #1394 #1395

Conversation

sgaestel commented Nov 29, 2016

coveralls commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 8, 2016

coveralls commented Feb 1, 2017

sgaestel commented Feb 1, 2017

JedWatson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 2, 2017

Choose a reason for hiding this comment

TrevorBurnham commented Mar 31, 2017

agirton commented May 17, 2017