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.

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

Merged
merged 4 commits into from
Aug 14, 2018
Merged

Ignore symbols and functions in select tag #13389

merged 4 commits into from
Aug 14, 2018

Conversation

raunofreiberg
Copy link
Contributor

Related to #11734 and #13362.

As expected, the same cases that fail for textarea seem to fail for select as well.

@raunofreiberg raunofreiberg changed the title wip: ignore symbols and functions in select tag Ignore symbols and functions in select tag Aug 14, 2018
@raunofreiberg
Copy link
Contributor Author

I'm not sure whether we need to sanitize the value here since the initialValue seems to be set to undefined later on.

@@ -21,7 +23,7 @@ if (__DEV__) {

type SelectWithWrapperState = HTMLSelectElement & {
_wrapperState: {
initialValue: ?string,
initialValue: ToStringValue | void,
Copy link
Contributor Author

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', () => {
Copy link
Contributor Author

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.

Copy link
Contributor Author

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'

Copy link
Contributor

@nhunzaker nhunzaker left a 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>,
)),
Copy link
Contributor

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,
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@nhunzaker
Copy link
Contributor

Later today I'll do some manual testing just for good measure. Looks great though, 👍

@nhunzaker
Copy link
Contributor

Checks out. Awesome work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants