Skip to content

Commit f964825

Browse files
committed
Amend tests and move onInputChange call
onInputChange is now called from `handleInputChange`, which is less surprising than `componentDidUpdate`, and should stop the double render, and hence the flash of typed content, as opposed to what is returned from `onInputChange`. Simplified the tests by using the existing `createComponentWithWrapper` function, and using a stub. This should make the tests clearer. Added a couple of tests for returning different things (e.g. null, numbers), and one to check the new value is actually rendered in the DOM.
1 parent 4e87373 commit f964825

File tree

2 files changed

+37
-35
lines changed

2 files changed

+37
-35
lines changed

src/Select.js

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -171,13 +171,6 @@ const Select = React.createClass({
171171
this.hasScrolledToOption = false;
172172
}
173173

174-
if (prevState.inputValue !== this.state.inputValue && this.props.onInputChange) {
175-
var nextState = this.props.onInputChange(this.state.inputValue);
176-
var newInput = (nextState !== undefined) ? nextState : null;
177-
if (newInput !== null) {
178-
this.setState({ inputValue: newInput });
179-
}
180-
}
181174
if (this._scrollToFocusedOptionOnUpdate && this.refs.focused && this.refs.menu) {
182175
this._scrollToFocusedOptionOnUpdate = false;
183176
var focusedDOM = ReactDOM.findDOMNode(this.refs.focused);
@@ -347,10 +340,18 @@ const Select = React.createClass({
347340
},
348341

349342
handleInputChange (event) {
343+
let newInputValue = event.target.value;
344+
if (this.state.inputValue !== event.target.value && this.props.onInputChange) {
345+
let nextState = this.props.onInputChange(newInputValue);
346+
// Note: != used deliberately here to catch undefined and null
347+
if (nextState != null) {
348+
newInputValue = '' + nextState;
349+
}
350+
}
350351
this.setState({
351352
isOpen: true,
352353
isPseudoFocused: false,
353-
inputValue: event.target.value,
354+
inputValue: newInputValue
354355
});
355356
},
356357

test/Select-test.js

Lines changed: 28 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -127,31 +127,6 @@ describe('Select', () => {
127127

128128
};
129129

130-
var createControlWithChange = (props, options) => {
131-
132-
options = options || {};
133-
134-
onChange = sinon.spy();
135-
onInputChange = sinon.spy(function(inputValue) {
136-
if (inputValue === '1') {
137-
return '2';
138-
}
139-
});
140-
// Render an instance of the component
141-
instance = TestUtils.renderIntoDocument(
142-
<Select
143-
onChange={onChange}
144-
onInputChange={onInputChange}
145-
{...props}
146-
/>
147-
);
148-
if (options.initialFocus !== false) {
149-
findAndFocusInputControl();
150-
}
151-
return instance;
152-
153-
};
154-
155130
var setValueProp = value => wrapper.setPropsForChild({ value });
156131

157132
var createControlWithWrapper = (props, options) => {
@@ -408,30 +383,56 @@ describe('Select', () => {
408383
});
409384

410385
describe('with a return from onInputChange', () => {
386+
387+
let onInputChangeOverride;
411388
beforeEach(() => {
412389
options = [
413390
{ value: 'one', label: 'One' },
414391
{ value: 'two', label: 'Two' },
415392
{ value: 'three', label: 'Three' }
416393
];
417394

418-
instance = createControlWithChange({
395+
onInputChangeOverride = sinon.stub();
396+
397+
wrapper = createControlWithWrapper({
419398
name: 'field-onChange',
420399
value: 'one',
421400
options: options,
422401
simpleValue: true,
402+
onInputChange: onInputChangeOverride
423403
});
424404
});
425405

426406
it('should change the value when onInputChange returns a value', () => {
407+
onInputChangeOverride.returns('2');
427408
typeSearchText('1');
428409
expect(instance.state.inputValue, 'to equal', '2');
429410
});
430411

431-
it('should return the input when onInputChange does not return a value', () => {
412+
it('should return the input when onInputChange returns undefined', () => {
413+
onInputChangeOverride.returns(undefined); // Not actually necessary as undefined is the default, but makes this clear
432414
typeSearchText('Test');
433415
expect(instance.state.inputValue, 'to equal', 'Test');
434416
});
417+
418+
it('should return the input when onInputChange returns null', () => {
419+
onInputChangeOverride.returns(null);
420+
typeSearchText('Test');
421+
expect(instance.state.inputValue, 'to equal', 'Test');
422+
});
423+
424+
it('should update the input when onInputChange returns a number', () => {
425+
onInputChangeOverride.returns(5);
426+
typeSearchText('Test');
427+
expect(instance.state.inputValue, 'to equal', '5');
428+
});
429+
430+
it('displays the new value in the input box', () => {
431+
onInputChangeOverride.returns('foo');
432+
typeSearchText('Test');
433+
const displayedValue = ReactDOM.findDOMNode(instance).querySelector('.Select-input input').value;
434+
expect(displayedValue, 'to equal', 'foo');
435+
});
435436
});
436437

437438
describe('with values as numbers', () => {

0 commit comments

Comments
 (0)