Skip to content

ReactDOMSelect makeover, fix edge-case inconsistencies and remove state #2241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 6, 2014
Merged

ReactDOMSelect makeover, fix edge-case inconsistencies and remove state #2241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 51 additions & 58 deletions src/browser/ui/dom/components/ReactDOMSelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,14 @@ var assign = require('Object.assign');

var select = ReactElement.createFactory('select');

function updateWithPendingValueIfMounted() {
function updateOptionsIfPendingUpdateAndMounted() {
/*jshint validthis:true */
if (this.isMounted()) {
this.setState({value: this._pendingValue});
this._pendingValue = 0;
if (this._pendingUpdate) {
this._pendingUpdate = false;
var value = LinkedValueUtils.getValue(this);
if (value != null && this.isMounted()) {
updateOptions(this, value);
}
}
}

Expand Down Expand Up @@ -56,40 +59,43 @@ function selectValueType(props, propName, componentName) {
}

/**
* If `value` is supplied, updates <option> elements on mount and update.
* @param {ReactComponent} component Instance of ReactDOMSelect
* @param {?*} propValue For uncontrolled components, null/undefined. For
* controlled components, a string (or with `multiple`, a list of strings).
* @param {*} propValue A stringable (with `multiple`, a list of stringables).
* @private
*/
function updateOptions(component, propValue) {
var multiple = component.props.multiple;
var value = propValue != null ? propValue : component.state.value;
var options = component.getDOMNode().options;
var selectedValue, i, l;
if (multiple) {
var options = component.getDOMNode().options;

if (component.props.multiple) {
selectedValue = {};
for (i = 0, l = value.length; i < l; ++i) {
selectedValue['' + value[i]] = true;
for (i = 0, l = propValue.length; i < l; i++) {
selectedValue['' + propValue[i]] = true;
}
for (i = 0, l = options.length; i < l; i++) {
var selected = selectedValue.hasOwnProperty(options[i].value);
if (options[i].selected !== selected) {
options[i].selected = selected;
}
}
} else {
selectedValue = '' + value;
}
for (i = 0, l = options.length; i < l; i++) {
var selected = multiple ?
selectedValue.hasOwnProperty(options[i].value) :
options[i].value === selectedValue;

if (selected !== options[i].selected) {
options[i].selected = selected;
// Do not set `select.value` as exact behavior isn't consistent across all
// browsers for all cases.
selectedValue = '' + propValue;
for (i = 0, l = options.length; i < l; i++) {
if (options[i].value === selectedValue) {
options[i].selected = true;
return;
}
}
options[0].selected = true;
}
}

/**
* Implements a <select> native component that allows optionally setting the
* props `value` and `defaultValue`. If `multiple` is false, the prop must be a
* string. If `multiple` is true, the prop must be an array of strings.
* stringable. If `multiple` is true, the prop must be an array of stringables.
*
* If `value` is not supplied (or null/undefined), user actions that change the
* selected option will trigger updates to the rendered options.
Expand All @@ -111,22 +117,6 @@ var ReactDOMSelect = ReactClass.createClass({
value: selectValueType
},

getInitialState: function() {
return {value: this.props.defaultValue || (this.props.multiple ? [] : '')};
},

componentWillMount: function() {
this._pendingValue = null;
},

componentWillReceiveProps: function(nextProps) {
if (!this.props.multiple && nextProps.multiple) {
this.setState({value: [this.state.value]});
} else if (this.props.multiple && !nextProps.multiple) {
this.setState({value: this.state.value[0]});
}
},

render: function() {
// Clone `this.props` so we don't mutate the input.
var props = assign({}, this.props);
Expand All @@ -137,16 +127,32 @@ var ReactDOMSelect = ReactClass.createClass({
return select(props, this.props.children);
},

componentWillMount: function() {
this._pendingUpdate = false;
},

componentDidMount: function() {
updateOptions(this, LinkedValueUtils.getValue(this));
var value = LinkedValueUtils.getValue(this);
if (value != null) {
updateOptions(this, value);
} else if (this.props.defaultValue != null) {
updateOptions(this, this.props.defaultValue);
}
},

componentDidUpdate: function(prevProps) {
var value = LinkedValueUtils.getValue(this);
var prevMultiple = !!prevProps.multiple;
var multiple = !!this.props.multiple;
if (value != null || prevMultiple !== multiple) {
if (value != null) {
this._pendingUpdate = false;
updateOptions(this, value);
} else if (!prevProps.multiple !== !this.props.multiple) {
// For simplicity, reapply `defaultValue` if `multiple` is toggled.
Copy link
Contributor

@yungsters yungsters Nov 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm my understanding, this is the main behavioral change caused by this pull request, right?

Copy link
Contributor Author

@syranide syranide Nov 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should be the only behavioral change.

if (this.props.defaultValue != null) {
updateOptions(this, this.props.defaultValue);
} else {
// Revert the select back to its default unselected state.
updateOptions(this, this.props.multiple ? [] : '');
}
}
},

Expand All @@ -157,21 +163,8 @@ var ReactDOMSelect = ReactClass.createClass({
returnValue = onChange.call(this, event);
}

var selectedValue;
if (this.props.multiple) {
selectedValue = [];
var options = event.target.options;
for (var i = 0, l = options.length; i < l; i++) {
if (options[i].selected) {
selectedValue.push(options[i].value);
}
}
} else {
selectedValue = event.target.value;
}

this._pendingValue = selectedValue;
ReactUpdates.asap(updateWithPendingValueIfMounted, this);
this._pendingUpdate = true;
ReactUpdates.asap(updateOptionsIfPendingUpdateAndMounted, this);
return returnValue;
}

Expand Down
77 changes: 71 additions & 6 deletions src/browser/ui/dom/components/__tests__/ReactDOMSelect-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,27 @@ describe('ReactDOMSelect', function() {
expect(node.options[2].selected).toBe(true); // twelve
});

it('should reset child options selected when they are changed and `value` is set', function() {
var stub =
<select multiple={true} value={["a", "b"]}>
</select>
stub = ReactTestUtils.renderIntoDocument(stub);

stub.setProps({
children: [
<option value="a">a</option>,
<option value="b">b</option>,
<option value="c">c</option>
]
})

var node = stub.getDOMNode()

expect(node.options[0].selected).toBe(true); // a
expect(node.options[1].selected).toBe(true); // b
expect(node.options[2].selected).toBe(false); // c
});

it('should allow setting `value` with `objectToString`', function() {
var objectToString = {
animal: "giraffe",
Expand Down Expand Up @@ -181,12 +202,12 @@ describe('ReactDOMSelect', function() {
expect(node.options[1].selected).toBe(true); // giraffe
expect(node.options[2].selected).toBe(false); // gorilla

// When making it multiple, giraffe should still be selected
stub.setProps({multiple: true, defaultValue: null});
// When making it multiple, giraffe and gorilla should be selected
stub.setProps({multiple: true, defaultValue: ['giraffe', 'gorilla']});

expect(node.options[0].selected).toBe(false); // monkey
expect(node.options[1].selected).toBe(true); // giraffe
expect(node.options[2].selected).toBe(false); // gorilla
expect(node.options[2].selected).toBe(true); // gorilla
});

it('should allow switching from multiple', function() {
Expand All @@ -203,13 +224,57 @@ describe('ReactDOMSelect', function() {
expect(node.options[1].selected).toBe(true); // giraffe
expect(node.options[2].selected).toBe(true); // gorilla

// When removing multiple, giraffe should still be selected (but gorilla
// will no longer be)
stub.setProps({multiple: false, defaultValue: null});
// When removing multiple, defaultValue is applied again, being omitted
// means that "monkey" will be selected
stub.setProps({multiple: false, defaultValue: 'gorilla'});

expect(node.options[0].selected).toBe(false); // monkey
expect(node.options[1].selected).toBe(false); // giraffe
expect(node.options[2].selected).toBe(true); // gorilla
});

it('should remember value when switching to uncontrolled', function() {
var stub =
<select value={'giraffe'}>
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
<option value="gorilla">A gorilla!</option>
</select>;
stub = ReactTestUtils.renderIntoDocument(stub);
var node = stub.getDOMNode();

expect(node.options[0].selected).toBe(false); // monkey
expect(node.options[1].selected).toBe(true); // giraffe
expect(node.options[2].selected).toBe(false); // gorilla

stub.setProps({value: null});

expect(node.options[0].selected).toBe(false); // monkey
expect(node.options[1].selected).toBe(true); // giraffe
expect(node.options[2].selected).toBe(false); // gorilla
});

it('should remember updated value when switching to uncontrolled', function() {
var stub =
<select value={'giraffe'}>
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
<option value="gorilla">A gorilla!</option>
</select>;
stub = ReactTestUtils.renderIntoDocument(stub);
var node = stub.getDOMNode();

stub.setProps({value: 'gorilla'});

expect(node.options[0].selected).toBe(false); // monkey
expect(node.options[1].selected).toBe(false); // giraffe
expect(node.options[2].selected).toBe(true); // gorilla

stub.setProps({value: null});

expect(node.options[0].selected).toBe(false); // monkey
expect(node.options[1].selected).toBe(false); // giraffe
expect(node.options[2].selected).toBe(true); // gorilla
});

it('should support ReactLink', function() {
Expand Down