Skip to content
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

Select element update #7054

Closed
wants to merge 18 commits into from
Closed

Select element update #7054

Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
021a6ca
Check for duplicate option values in select element.
Jun 16, 2016
cc7b610
Check for duplicate option values in select element.
Jun 16, 2016
87587e2
Moved to the __DEV__ block.
Jun 16, 2016
dc09140
Update
Jun 21, 2016
0a6cf21
Update
Jun 21, 2016
d092c52
Updates
Jun 21, 2016
dbd4851
Merge branch 'master' of github.com:facebook/react into SelectElement…
Jun 26, 2016
bf9fe29
Check for duplicate values in select element
Jun 27, 2016
01c63d5
Check for duplicate values in select element.
Jun 27, 2016
4c932a0
Fixed warning message
Jun 27, 2016
06aafde
Early return if warning has already been displayed.
Jun 27, 2016
f129956
Put values in a dictionary instead of an array.
Jul 1, 2016
1fd6935
Changed option values dictionary back to an array.
Jul 1, 2016
c172e1c
Put option values in a dictionary.
Jul 1, 2016
03dc300
Updates & removed redundant code
Jul 3, 2016
36524f3
Changed a 'let' to 'const'
Jul 3, 2016
0f38d16
Removed function parameters..
Jul 3, 2016
1445899
Consolidated 'if' statements.
Jul 6, 2016
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
23 changes: 23 additions & 0 deletions src/renderers/dom/client/wrappers/ReactDOMSelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,28 @@ function warnIfValueIsNull(props) {
}
}

function warnIfDuplicateValues(inst) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're trying to put new warnings within devtools. For an example that you can mimic, see: #7040

Copy link
Author

Choose a reason for hiding this comment

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

Should I move it to ReactDOMNullInputValuePropDevtool.js or create a new file? If a new file, what should I name it? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

You would create a new file for this warning. #7040 is just a reference for what it might look like. You can maybe name it something like ReactDOMDuplicateSelectValuesDevtool.js to be consistent with the current naming scheme.

Copy link
Author

Choose a reason for hiding this comment

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

I noticed ReactDOMNullInputValuePropDevtool.js exports ReactDOMUnknownPropertyDevtool. Is that correct or a copy/paste error?

Copy link
Contributor

Choose a reason for hiding this comment

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

@git-richard looks like a copy/paste error to me, but @jimfb can verify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, just copy-paste.

const options = inst._currentElement.props.children;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is legal for this to be a composite component, so the children might not be simple option tags.

Copy link
Contributor

Choose a reason for hiding this comment

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

They can also be nested inside arrays/maps/sets etc, so this does not work reliably.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also not to forget that <optgroup> is a thing.

Copy link
Contributor

@syranide syranide Jun 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Also, children can be null or an element (not wrapped in an array). I'm not sure whether it is supported or not yet, but IIRC the idea is also to support anything that is iterable as valid value for children, so we can't assume this to be an array in that case?


if (options === undefined) {
return;
}

// Displays a warning for all duplicate values in select element. Does not exit after first duplicate is found.
for (var i = 0; i < options.length-1; i++) {
if (options[i].props.value !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!= null I would assume? Or?

Copy link
Contributor

Choose a reason for hiding this comment

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

Children are not required to be only elements. This needs to check the type (also related to composite components as mentioned by @jimfb, shouldn't check the value of those).

for (var j = i + 1; j < options.length; j++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

O(n^2) performance seems far from ideal.

if (options[i].props.value === options[j].props.value) {
warning(
false,
`Select element contains duplicate option value ${options[i].props.value} in options #${i} & #${j}`
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to include a stack trace for new warnings. Again, #7040 is a good example.

);
}
}
}
}
}

var valuePropNames = ['value', 'defaultValue'];

/**
Expand Down Expand Up @@ -169,6 +191,7 @@ var ReactDOMSelect = {
if (__DEV__) {
checkSelectPropTypes(inst, props);
warnIfValueIsNull(props);
warnIfDuplicateValues(inst);
Copy link
Contributor

@syranide syranide Jun 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Duplicate values is valid for uncontrolled components, so we probably shouldn't warn in that case?

}

var value = LinkedValueUtils.getValue(props);
Expand Down