Skip to content

Extract components#352

Merged
JedWatson merged 10 commits intoJedWatson:masterfrom
jniechcial:extract-components
Aug 3, 2015
Merged

Extract components#352
JedWatson merged 10 commits intoJedWatson:masterfrom
jniechcial:extract-components

Conversation

@jniechcial
Copy link

New PR due to rebasing and fixing conflicts, but the discussion available in this PR.

@bruderstein
Copy link
Collaborator

Cool, I've just rebased this on my even-more-tests branch (this will be sent as a PR if/when PR #348 is merged, as it relies on it), and one test fails. (The tests for valueRenderer are sadly only in this branch)

It's a pretty easy fix though, you just need to move the

var val = this.state.values[0] || null;

from line 767 to outside the deepest if, otherwise in the case of a single value with a valueRenderer, you pass option={val}, but val isn't defined. After doing that, the test passes, and everything is green.

Tests are nice :)

src/Select.js Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to move this line up

@jniechcial
Copy link
Author

@bruderstein fixed, good to have wide test coverage :)

@JedWatson
Copy link
Owner

#348 is merged :) this looks great @jniechcial, thank you. and thanks to @bruderstein for the review!

JedWatson added a commit that referenced this pull request Aug 3, 2015
@JedWatson JedWatson merged commit f9d5ba1 into JedWatson:master Aug 3, 2015
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.

3 participants