From 6c37ecee3f3b4f72196fae8614afca8011e35f73 Mon Sep 17 00:00:00 2001 From: Yuri S Date: Fri, 15 Dec 2017 15:02:27 +0200 Subject: [PATCH 1/9] Fixed bug for single and onSelectResetsInput=false. --- src/Select.js | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/Select.js b/src/Select.js index 022004763f..f8c38da6f5 100644 --- a/src/Select.js +++ b/src/Select.js @@ -33,6 +33,14 @@ const stringOrNumber = PropTypes.oneOfType([ let instanceId = 1; +function shouldShowInputValue(state, props, isOpen){ + if (!state.inputValue) return true; + if (!props.onSelectResetsInput && !isOpen){ + return !(!state.isFocused && state.isPseudoFocused); + } + return false; +} + class Select extends React.Component { constructor (props) { super(props); @@ -347,7 +355,7 @@ class Select extends React.Component { let toOpen = this.state.isOpen || this._openAfterFocus || this.props.openOnFocus; toOpen = this._focusAfterClear ? false : toOpen; //if focus happens after clear values, don't open dropdown yet. - + if (this.props.onFocus) { this.props.onFocus(event); } @@ -791,7 +799,7 @@ class Select extends React.Component { ); }); - } else if (!this.state.inputValue) { + } else if (shouldShowInputValue(this.state, this.props, isOpen)) { if (isOpen) onClick = null; return ( this.input = ref, required: this.state.required, - value: this.state.inputValue, + value, }; if (this.props.inputRenderer) { From 94b3081001fd0c32fee1bcbc92eedc21b8d16053 Mon Sep 17 00:00:00 2001 From: Yuri S Date: Fri, 15 Dec 2017 17:17:32 +0200 Subject: [PATCH 2/9] Fixed placeholder and improved if statement. --- src/Select.js | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/Select.js b/src/Select.js index f8c38da6f5..ce2c12d7e2 100644 --- a/src/Select.js +++ b/src/Select.js @@ -33,13 +33,25 @@ const stringOrNumber = PropTypes.oneOfType([ let instanceId = 1; -function shouldShowInputValue(state, props, isOpen){ - if (!state.inputValue) return true; - if (!props.onSelectResetsInput && !isOpen){ - return !(!state.isFocused && state.isPseudoFocused); +const shouldShowValue = (state, props, isOpen) => { + const { inputValue, isPseudoFocused, isFocused } = state; + const { onSelectResetsInput } = props; + + if (!inputValue) return true; + + if (!onSelectResetsInput && !isOpen){ + return !(!isFocused && isPseudoFocused || isFocused && !isPseudoFocused); } + return false; -} +}; + +const shouldShowPlaceholder = (state, props, isOpen) => { + const { inputValue, isPseudoFocused, isFocused } = state; + const { onSelectResetsInput } = props; + + return !inputValue || !onSelectResetsInput && !isOpen && !isPseudoFocused && !isFocused; +}; class Select extends React.Component { constructor (props) { @@ -779,7 +791,8 @@ class Select extends React.Component { let renderLabel = this.props.valueRenderer || this.getOptionLabel; let ValueComponent = this.props.valueComponent; if (!valueArray.length) { - return !this.state.inputValue ?
{this.props.placeholder}
: null; + const showPlaceholder = shouldShowPlaceholder(this.state, this.props, isOpen); + return showPlaceholder ?
{this.props.placeholder}
: null; } let onClick = this.props.onValueClick ? this.handleValueClick : null; if (this.props.multi) { @@ -799,7 +812,7 @@ class Select extends React.Component { ); }); - } else if (shouldShowInputValue(this.state, this.props, isOpen)) { + } else if (shouldShowValue(this.state, this.props, isOpen)) { if (isOpen) onClick = null; return ( Date: Sat, 16 Dec 2017 14:45:18 +0200 Subject: [PATCH 3/9] Updated readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 236e89a495..d5319ecef3 100644 --- a/README.md +++ b/README.md @@ -381,7 +381,7 @@ function onInputKeyDown(event) { | `onInputKeyDown` | function | undefined | input keyDown handler; call `event.preventDefault()` to override default `Select` behaviour: `function(event) {}` | | `onMenuScrollToBottom` | function | undefined | called when the menu is scrolled to the bottom | | `onOpen` | function | undefined | handler for when the menu opens: `function () {}` | -| `onSelectResetsInput` | boolean | true | whether the input value should be reset when options are selected, for `multi` +| `onSelectResetsInput` | boolean | true | whether the input value should be reset when options are selected. Also input value will be set to empty if 'onSelectResetsInput=true' and Select will get new value that not equal previous value. | | `onValueClick` | function | undefined | onClick handler for value labels: `function (value, event) {}` | | `openOnClick` | boolean | true | open the options menu when the control is clicked (requires searchable = true) | | `openOnFocus` | boolean | false | open the options menu when the control gets focus | From 14e7b18a3fad6171cdc6aa632006e41f0257c072 Mon Sep 17 00:00:00 2001 From: Yuri S Date: Sun, 17 Dec 2017 02:48:39 +0200 Subject: [PATCH 4/9] Added tests and another minor fixes --- src/Select.js | 8 +- test/Select-test.js | 282 +++++++++++++++++++++++++------------------- 2 files changed, 165 insertions(+), 125 deletions(-) diff --git a/src/Select.js b/src/Select.js index ce2c12d7e2..83411ca5a1 100644 --- a/src/Select.js +++ b/src/Select.js @@ -33,13 +33,13 @@ const stringOrNumber = PropTypes.oneOfType([ let instanceId = 1; -const shouldShowValue = (state, props, isOpen) => { +const shouldShowValue = (state, props) => { const { inputValue, isPseudoFocused, isFocused } = state; const { onSelectResetsInput } = props; if (!inputValue) return true; - if (!onSelectResetsInput && !isOpen){ + if (!onSelectResetsInput){ return !(!isFocused && isPseudoFocused || isFocused && !isPseudoFocused); } @@ -812,7 +812,7 @@ class Select extends React.Component { ); }); - } else if (shouldShowValue(this.state, this.props, isOpen)) { + } else if (shouldShowValue(this.state, this.props)) { if (isOpen) onClick = null; return ( { onChange={onChange} onInputChange={onInputChange} {...props} - /> + /> ); if (options.initialFocus !== false) { findAndFocusInputControl(); @@ -177,7 +178,7 @@ describe('Select', () => { onChange={onChange} onInputChange={onInputChange} {...props} - /> + /> ); instance = wrapper.getChild(); @@ -275,7 +276,7 @@ describe('Select', () => { expect(ReactDOM.findDOMNode(selectInputElement).name, 'to equal', 'form-field-name'); }); - it('should show the options on mouse click', function () { + it('should show the options on mouse click', () => { TestUtils.Simulate.mouseDown(ReactDOM.findDOMNode(instance).querySelector('.Select-control'), { button: 0 }); var node = ReactDOM.findDOMNode(instance); expect(node, 'queried for', '.Select-option', 'to have length', 3); @@ -348,14 +349,14 @@ describe('Select', () => { }); - it('should display the options menu when tapped', function() { + it('should display the options menu when tapped', () => { TestUtils.Simulate.touchStart(getSelectControl(instance)); TestUtils.Simulate.touchEnd(getSelectControl(instance)); var node = ReactDOM.findDOMNode(instance); expect(node, 'queried for', '.Select-option', 'to have length', 3); }); - it('should not display the options menu when touched and dragged', function() { + it('should not display the options menu when touched and dragged', () => { TestUtils.Simulate.touchStart(getSelectControl(instance)); TestUtils.Simulate.touchMove(getSelectControl(instance)); TestUtils.Simulate.touchEnd(getSelectControl(instance)); @@ -721,7 +722,6 @@ describe('Select', () => { }); it('selects the initial value', () => { - expect(instance, 'to contain',
Two
@@ -868,7 +868,7 @@ describe('Select', () => { typeSearchText('1'); expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', - 'to satisfy', [ + 'to satisfy', [ expect.it('to have text', 'One'), expect.it('to have text', 'Ten'), expect.it('to have text', 'Twenty-one') @@ -1016,18 +1016,18 @@ describe('Select', () => { }); it('set the initial value of the hidden input control', () => { - expect(ReactDOM.findDOMNode(wrapper).querySelector(FORM_VALUE_SELECTOR).value, 'to equal', 'true' ); + expect(ReactDOM.findDOMNode(wrapper).querySelector(FORM_VALUE_SELECTOR).value, 'to equal', 'true'); }); it('updates the value when the value prop is set', () => { wrapper.setPropsForChild({ value: false }); expect(ReactDOM.findDOMNode(instance), 'queried for first', DISPLAYED_SELECTION_SELECTOR, - 'to have text', 'No'); + 'to have text', 'No'); }); it('updates the value of the hidden input control after new value prop', () => { wrapper.setPropsForChild({ value: false }); - expect(ReactDOM.findDOMNode(wrapper).querySelector(FORM_VALUE_SELECTOR).value, 'to equal', 'false' ); + expect(ReactDOM.findDOMNode(wrapper).querySelector(FORM_VALUE_SELECTOR).value, 'to equal', 'false'); }); it('calls onChange with the new value as a boolean', () => { @@ -1040,7 +1040,7 @@ describe('Select', () => { it('supports setting the value via prop', () => { wrapper.setPropsForChild({ value: false }); expect(ReactDOM.findDOMNode(instance), 'queried for first', DISPLAYED_SELECTION_SELECTOR, - 'to have text', 'No'); + 'to have text', 'No'); }); it('displays the X button for false value', () => { @@ -1068,7 +1068,7 @@ describe('Select', () => { it('selects the initial value', () => { expect(instance, 'to contain', - +
Yes
No
); @@ -1088,7 +1088,7 @@ describe('Select', () => { }); expect(instance, 'to contain', - +
No
); }); @@ -1100,7 +1100,7 @@ describe('Select', () => { }); expect(instance, 'to contain', - +
Yes
); }); @@ -1113,7 +1113,7 @@ describe('Select', () => { }); expect(instance, 'to contain', - +
No
); }); @@ -1153,18 +1153,18 @@ describe('Select', () => { typeSearchText('fal'); expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', - 'to satisfy', [ - expect.it('to have text', 'No'), - ]); + 'to satisfy', [ + expect.it('to have text', 'No'), + ]); }); it('finds text at end', () => { typeSearchText('se'); expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', - 'to satisfy', [ - expect.it('to have text', 'No'), - ]); + 'to satisfy', [ + expect.it('to have text', 'No'), + ]); }); }); @@ -1182,18 +1182,18 @@ describe('Select', () => { typeSearchText('fa'); expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', - 'to satisfy', [ - expect.it('to have text', 'No') - ]); + 'to satisfy', [ + expect.it('to have text', 'No') + ]); }); it('does not match text at end', () => { typeSearchText('se'); expect(ReactDOM.findDOMNode(instance), 'to contain elements matching', - '.Select-noresults'); + '.Select-noresults'); expect(ReactDOM.findDOMNode(instance), 'to contain no elements matching', - '.Select-option'); + '.Select-option'); }); }); @@ -1210,19 +1210,19 @@ describe('Select', () => { typeSearchText('al'); expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', - 'to satisfy', [ - expect.it('to have text', 'No'), - ]); + 'to satisfy', [ + expect.it('to have text', 'No'), + ]); }); it('finds text at end', () => { typeSearchText('e'); expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', - 'to satisfy', [ - expect.it('to have text', 'Yes'), - expect.it('to have text', 'No') - ]); + 'to satisfy', [ + expect.it('to have text', 'Yes'), + expect.it('to have text', 'No') + ]); }); }); @@ -1240,18 +1240,18 @@ describe('Select', () => { typeSearchText('tr'); expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', - 'to satisfy', [ - expect.it('to have text', 'Yes') - ]); + 'to satisfy', [ + expect.it('to have text', 'Yes') + ]); }); it('does not match text at end', () => { typeSearchText('e'); expect(ReactDOM.findDOMNode(instance), 'to contain elements matching', - '.Select-noresults'); + '.Select-noresults'); expect(ReactDOM.findDOMNode(instance), 'to contain no elements matching', - '.Select-option'); + '.Select-option'); }); }); }); @@ -1298,7 +1298,7 @@ describe('Select', () => { value: 'three' }); - expect(ReactDOM.findDOMNode(wrapper).querySelector(FORM_VALUE_SELECTOR).value, 'to equal', 'three' ); + expect(ReactDOM.findDOMNode(wrapper).querySelector(FORM_VALUE_SELECTOR).value, 'to equal', 'three'); }); it('display the raw value if the option is not available', () => { @@ -1361,7 +1361,7 @@ describe('Select', () => { expect(ReactDOM.findDOMNode(instance).querySelectorAll('.Select-option')[1], 'to have attributes', { class: 'is-disabled' - }); + }); }); it('is not selectable by clicking', () => { @@ -1488,7 +1488,7 @@ describe('Select', () => { expect(onChange, 'was not called'); // And the menu is still open expect(ReactDOM.findDOMNode(instance), 'to contain no elements matching', DISPLAYED_SELECTION_SELECTOR); - expect(ReactDOM.findDOMNode(instance), 'queried for' , '.Select-option', + expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', 'to satisfy', [ expect.it('to have text', 'Two') ]); @@ -1501,7 +1501,7 @@ describe('Select', () => { expect(onChange, 'was not called'); // And the menu is still open expect(ReactDOM.findDOMNode(instance), 'to contain no elements matching', DISPLAYED_SELECTION_SELECTOR); - expect(ReactDOM.findDOMNode(instance), 'queried for' , '.Select-option', + expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', 'to satisfy', [ expect.it('to have text', 'Two') ]); @@ -1514,7 +1514,7 @@ describe('Select', () => { expect(onChange, 'was not called'); // And the menu is still open expect(ReactDOM.findDOMNode(instance), 'to contain no elements matching', DISPLAYED_SELECTION_SELECTOR); - expect(ReactDOM.findDOMNode(instance), 'queried for' , '.Select-option', + expect(ReactDOM.findDOMNode(instance), 'queried for', '.Select-option', 'to satisfy', [ expect.it('to have text', 'Two') ]); @@ -1848,7 +1848,7 @@ describe('Select', () => { it('filters the existing selections from the options', () => { - setValueProp(['four','three']); + setValueProp(['four', 'three']); typeSearchText('o'); @@ -1861,7 +1861,7 @@ describe('Select', () => { it('removes the last selected option with backspace', () => { - setValueProp(['four','three']); + setValueProp(['four', 'three']); onChange.reset(); // Ignore previous onChange calls pressBackspace(); expect(onChange, 'was called with', [{ label: 'Four', value: 'four' }]); @@ -1887,7 +1887,7 @@ describe('Select', () => { it('removes the last selected option with delete', () => { - setValueProp(['four','three']); + setValueProp(['four', 'three']); onChange.reset(); // Ignore previous onChange calls pressDelete(); expect(onChange, 'was called with', [{ label: 'Four', value: 'four' }]); @@ -2129,63 +2129,103 @@ describe('Select', () => { }); - describe('with multi=true different onSelectResetsInput', () => { - it('should have retained inputValue after accepting selection with onSelectResetsInput=false', () => { - options = [ - { value: 'one', label: 'One' }, - { value: 'two', label: 'Two' }, - { value: 'three', label: 'Three' }, - { value: 'four', label: 'Four' } - ]; + describe('onSelectResetsInput', () => { + options = [ + { value: 'one', label: 'One' }, + { value: 'two', label: 'Two' }, + { value: 'three', label: 'Three' }, + { value: 'four', label: 'Four' } + ]; + + describe('with single select', () => { + it('should have retained inputValue after accepting selection with onSelectResetsInput=false', () => { + // Render an instance of the component + wrapper = createControlWithWrapper({ + value: '', + options: options, + onSelectResetsInput: false, + onCloseResetsInput: false, + onBlurResetsInput: false, + }); - // Render an instance of the component - wrapper = createControlWithWrapper({ - value: '', - options: options, - multi: true, - closeOnSelect: false, - removeSelected: false, - onSelectResetsInput: false - }); + clickArrowToOpen(); + typeSearchText('tw'); + pressEnterToAccept(); + setValueProp('two'); // trigger componentWillReceiveProps - instance.setState({ - isFocused: true + expect(instance.state.inputValue, 'to equal', 'tw'); + expect(instance, 'to contain',
Two
); + expect(instance, 'to contain', ); + + instance.setState({ + isFocused: false, + isPseudoFocused: false, + }); + + expect(instance, 'to contain', ); + expect(instance, 'to contain',
Two
); + + instance.setState({ + isFocused: true, + }); + + expect(instance, 'to contain', ); + expect(instance, 'not to contain',
Two
); }); - clickArrowToOpen(); - typeSearchText('two'); - pressEnterToAccept(); - setValueProp('two'); // trigger componentWillReceiveProps + it('should have reset the inputValue after accepting selection when onSelectResetsInput= true or not set', () => { + // Render an instance of the component + wrapper = createControlWithWrapper({ + value: '', + options: options, + }); + + clickArrowToOpen(); + typeSearchText('tw'); + expect(instance.state.inputValue, 'to equal', 'tw'); + pressEnterToAccept(); + setValueProp('two'); // trigger componentWillReceiveProps - expect(instance.state.inputValue, 'to equal', 'two'); + expect(instance.state.inputValue, 'to equal', ''); + expect(instance, 'to contain',
Two
); + expect(instance, 'to contain', ); + }); }); - it('should have reset the inputValue after accepting selection when onSelectResetsInput= true or not set', () => { - options = [ - { value: 'one', label: 'One' }, - { value: 'two', label: 'Two' }, - { value: 'three', label: 'Three' }, - { value: 'four', label: 'Four' } - ]; + describe('with multi select', () => { + it('should have retained inputValue after accepting selection with onSelectResetsInput=false', () => { + // Render an instance of the component + wrapper = createControlWithWrapper({ + value: '', + options: options, + multi: true, + onSelectResetsInput: false + }); - // Render an instance of the component - wrapper = createControlWithWrapper({ - value: '', - options: options, - multi: true, - closeOnSelect: false, - removeSelected: false - }); + clickArrowToOpen(); + typeSearchText('two'); + pressEnterToAccept(); + setValueProp('two'); // trigger componentWillReceiveProps - instance.setState({ - isFocused: true + expect(instance.state.inputValue, 'to equal', 'two'); }); - clickArrowToOpen(); - typeSearchText('two'); - pressEnterToAccept(); + it('should have reset the inputValue after accepting selection when onSelectResetsInput= true or not set', () => { + // Render an instance of the component + wrapper = createControlWithWrapper({ + value: '', + options: options, + multi: true, + }); + + clickArrowToOpen(); + typeSearchText('two'); + expect(instance.state.inputValue, 'to equal', 'two'); + pressEnterToAccept(); + setValueProp('two'); // trigger componentWillReceiveProps - expect(instance.state.inputValue, 'to equal', ''); + expect(instance.state.inputValue, 'to equal', ''); + }); }); }); @@ -2717,16 +2757,16 @@ describe('Select', () => { instance = ReactDOM.render(, node); - expect(instance.state.isFocused, 'to equal', false); + expect(instance.state.isFocused, 'to equal', false); + findAndFocusInputControl(); + expect(instance.state.isFocused, 'to equal', true); + ReactDOM.render(