Skip to content

Commit 41d1c20

Browse files
nicolasiensenoliviertassinari
authored andcommitted
[Select] Improve the value comparison function (mui#13408)
* Support type–converting when comparing selected values in SelectInput In this commit, we are changing the comparison strategy of the SelectInput component to match values across different types, for example 10 and "10". Instead of using type–converting operators (== and !=) that would generate linter offenses, we adopted a strategy to parse the values to strings before comparing. This commit addresses the issue mui#12047 * let's merge
1 parent 6cca823 commit 41d1c20

File tree

5 files changed

+92
-8
lines changed

5 files changed

+92
-8
lines changed

.size-limit.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ module.exports = [
2222
name: 'The size of all the material-ui modules.',
2323
webpack: true,
2424
path: 'packages/material-ui/build/index.js',
25-
limit: '93.2 KB',
25+
limit: '93.3 KB',
2626
},
2727
{
2828
name: 'The main docs bundle',

packages/material-ui/src/Select/Select.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,10 @@ Select.propTypes = {
172172
PropTypes.string,
173173
PropTypes.number,
174174
PropTypes.bool,
175-
PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool])),
175+
PropTypes.object,
176+
PropTypes.arrayOf(
177+
PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool, PropTypes.object]),
178+
),
176179
]),
177180
/**
178181
* The variant to use.

packages/material-ui/src/Select/SelectInput.js

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,14 @@ import Menu from '../Menu/Menu';
77
import { isFilled } from '../InputBase/utils';
88
import { setRef } from '../utils/reactHelpers';
99

10+
function areEqualValues(a, b) {
11+
if (typeof b === 'object' && b !== null) {
12+
return a === b;
13+
}
14+
15+
return String(a) === String(b);
16+
}
17+
1018
/**
1119
* @ignore - internal component.
1220
*/
@@ -225,12 +233,12 @@ class SelectInput extends React.Component {
225233
);
226234
}
227235

228-
selected = value.indexOf(child.props.value) !== -1;
236+
selected = value.some(v => areEqualValues(v, child.props.value));
229237
if (selected && computeDisplay) {
230238
displayMultiple.push(child.props.children);
231239
}
232240
} else {
233-
selected = value === child.props.value;
241+
selected = areEqualValues(value, child.props.value);
234242
if (selected && computeDisplay) {
235243
displaySingle = child.props.children;
236244
}
@@ -446,7 +454,10 @@ SelectInput.propTypes = {
446454
PropTypes.string,
447455
PropTypes.number,
448456
PropTypes.bool,
449-
PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool])),
457+
PropTypes.object,
458+
PropTypes.arrayOf(
459+
PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool, PropTypes.object]),
460+
),
450461
]).isRequired,
451462
/**
452463
* The variant to use.

packages/material-ui/src/Select/SelectInput.test.js

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,39 @@ describe('<SelectInput />', () => {
6161
);
6262
});
6363

64+
describe('prop: value', () => {
65+
it('should select the option based on the number value', () => {
66+
const wrapper = shallow(<SelectInput {...defaultProps} value={20} />);
67+
assert.deepEqual(wrapper.find(MenuItem).map(m => m.props().selected), [false, true, false]);
68+
});
69+
70+
it('should select the option based on the string value', () => {
71+
const wrapper = shallow(<SelectInput {...defaultProps} value="20" />);
72+
assert.deepEqual(wrapper.find(MenuItem).map(m => m.props().selected), [false, true, false]);
73+
});
74+
75+
it('should select only the option that matches the object', () => {
76+
const obj1 = { id: 1 };
77+
const obj2 = { id: 2 };
78+
79+
const wrapper = shallow(
80+
<SelectInput {...defaultProps} value={obj1}>
81+
<MenuItem key={1} value={obj1}>
82+
1
83+
</MenuItem>
84+
<MenuItem key={2} value={obj2}>
85+
2
86+
</MenuItem>
87+
</SelectInput>,
88+
);
89+
90+
assert.deepEqual(wrapper.find(MenuItem).map(wrapper2 => wrapper2.props().selected), [
91+
true,
92+
false,
93+
]);
94+
});
95+
});
96+
6497
describe('prop: readOnly', () => {
6598
it('should not trigger any event with readOnly', () => {
6699
const wrapper = shallow(<SelectInput {...defaultProps} readOnly />);
@@ -281,9 +314,7 @@ describe('<SelectInput />', () => {
281314
const wrapper = shallow(<SelectInput {...defaultProps} disabled tabIndex={0} />);
282315
assert.strictEqual(wrapper.find('[data-mui-test="SelectDisplay"]').props().tabIndex, 0);
283316
});
284-
});
285317

286-
describe('prop: multiple', () => {
287318
it('should serialize multiple select value', () => {
288319
const wrapper = shallow(<SelectInput {...defaultProps} value={[10, 30]} multiple />);
289320
assert.strictEqual(wrapper.find('input').props().value, '10,30');
@@ -294,6 +325,45 @@ describe('<SelectInput />', () => {
294325
]);
295326
});
296327

328+
describe('when the value matches an option but they are different types', () => {
329+
it('should select the options based on the value', () => {
330+
const wrapper = shallow(<SelectInput {...defaultProps} value={['10', '20']} multiple />);
331+
assert.deepEqual(wrapper.find(MenuItem).map(wrapper2 => wrapper2.props().selected), [
332+
true,
333+
true,
334+
false,
335+
]);
336+
});
337+
});
338+
339+
describe('when the value is an object', () => {
340+
it('should select only the options that match', () => {
341+
const obj1 = { id: 1 };
342+
const obj2 = { id: 2 };
343+
const obj3 = { id: 3 };
344+
345+
const wrapper = shallow(
346+
<SelectInput {...defaultProps} value={[obj1, obj3]} multiple>
347+
<MenuItem key={1} value={obj1}>
348+
1
349+
</MenuItem>
350+
<MenuItem key={2} value={obj2}>
351+
2
352+
</MenuItem>
353+
<MenuItem key={3} value={obj3}>
354+
3
355+
</MenuItem>
356+
</SelectInput>,
357+
);
358+
359+
assert.deepEqual(wrapper.find(MenuItem).map(wrapper2 => wrapper2.props().selected), [
360+
true,
361+
false,
362+
true,
363+
]);
364+
});
365+
});
366+
297367
it('should throw if non array', () => {
298368
assert.throw(() => {
299369
shallow(<SelectInput {...defaultProps} multiple />);

pages/api/select.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import Select from '@material-ui/core/Select';
3535
| <span class="prop-name">open</span> | <span class="prop-type">bool</span> |   | Control `select` open state. You can only use it when the `native` property is `false` (default). |
3636
| <span class="prop-name">renderValue</span> | <span class="prop-type">func</span> |   | Render the selected value. You can only use it when the `native` property is `false` (default).<br><br>**Signature:**<br>`function(value: any) => ReactElement`<br>*value:* The `value` provided to the component. |
3737
| <span class="prop-name">SelectDisplayProps</span> | <span class="prop-type">object</span> |   | Properties applied to the clickable div element. |
38-
| <span class="prop-name">value</span> | <span class="prop-type">union:&nbsp;string&nbsp;&#124;<br>&nbsp;number&nbsp;&#124;<br>&nbsp;bool&nbsp;&#124;<br>&nbsp;arrayOf<br></span> |   | The input value. This property is required when the `native` property is `false` (default). |
38+
| <span class="prop-name">value</span> | <span class="prop-type">union:&nbsp;string, number, bool, object, arrayOf<br></span> |   | The input value. This property is required when the `native` property is `false` (default). |
3939
| <span class="prop-name">variant</span> | <span class="prop-type">enum:&nbsp;'standard'&nbsp;&#124;<br>&nbsp;'outlined'&nbsp;&#124;<br>&nbsp;'filled'<br></span> |   | The variant to use. |
4040

4141
Any other properties supplied will be spread to the root element ([Input](/api/input/)).

0 commit comments

Comments
 (0)