Skip to content

Conversation

@sophiebits
Copy link
Collaborator

No description provided.

@zpao
Copy link
Member

zpao commented Jan 21, 2014

This is resulting in https://github.com/facebook/react/blob/master/src/dom/components/__tests__/ReactDOMSelect-test.js#L202 failing internally now with jsdom (cc @benjamn)

It's leaving the last option selected, though this doesn't sound like behavior a real browser would have. Previously we would explicitly set node.selected = false all the time, but this diff changes that behavior. That's why we never saw the issue before.

Nothing else for you to do here I don't think. We may have to shim some fixes into jsdom :(

@sophiebits
Copy link
Collaborator Author

I'm not super concerned about the test; we could get rid of it if we wanted to.

Copy link
Member

Choose a reason for hiding this comment

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

REview from @yungsters:

Instead, would this be easier to follow?

updateOptions(this, this.getValue());

@zpao
Copy link
Member

zpao commented Feb 5, 2014

Let's make that change (which gets rid of a boolean param, yay), and rebase. @benjamn is patching jsdom to make the test work internally.

@sophiebits
Copy link
Collaborator Author

Done.

@zpao zpao closed this in 4ebdb2c Feb 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants