-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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.
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
Ignore symbols and functions in select tag #13389
Ignore symbols and functions in select tag #13389
Conversation
@@ -21,7 +23,7 @@ if (__DEV__) { | |||
|
|||
type SelectWithWrapperState = HTMLSelectElement & { | |||
_wrapperState: { | |||
initialValue: ?string, | |||
initialValue: ToStringValue | void, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ToStringValue
is an opaque type I wasn't sure what to do here.
expect(selectNode.value).toBe(''); | ||
}); | ||
|
||
it('treats controlled function value as an empty string', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seemed to be a additional case that I found failing before this PR. I don't know if this is the best way to test it, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this case is probably fine as is, since the browser defaults to using the element's children as a value if the attribute value
is not provided.
<select id="test">
<option>hi</option>
</select>
console.log(document.getElementById('test').value) // 'hi'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some style stuff. This looks great though.
I've left a note to myself to file an issue for my initialValue
comment, but if you wanted to go ahead and investigate, it's all yours :)
<option value="monkey">A monkey!</option> | ||
<option value="giraffe">A giraffe!</option> | ||
</select>, | ||
)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: but I think the implicit return with an assignment distracts from the test. Could you remove them, like:
expect(() => {
node = ReactTestUtils.renderIntoDocument(
<select onChange={noop} value={Symbol('foobar')}>
<option value={Symbol('foobar')}>A Symbol!</option>
<option value="monkey">A monkey!</option>
<option value="giraffe">A giraffe!</option>
</select>
)
}).toWarnDev('Invalid value for prop `value`');
@@ -21,7 +23,7 @@ if (__DEV__) { | |||
|
|||
type SelectWithWrapperState = HTMLSelectElement & { | |||
_wrapperState: { | |||
initialValue: ?string, | |||
initialValue: ?ToStringValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's curious that this is nullable and it's not clear to me where initialValue
is even used.
Not important for this PR, but I can't help but wonder if initialValue
can go away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I'll try and play around with it and see what I find out 🙂Feel free to report an issue, regardless.
Later today I'll do some manual testing just for good measure. Looks great though, 👍 |
Checks out. Awesome work! |
Related to #11734 and #13362.
As expected, the same cases that fail for textarea seem to fail for select as well.