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

Extract components #352

Merged
merged 10 commits into from
Aug 3, 2015
Merged

Extract components #352

merged 10 commits into from
Aug 3, 2015

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 :)

value.push(<Value
key={0}
option={val}
renderer={this.props.valueRenderer}
disabled={this.props.disabled} />);
} else {
value.push(<div className="Select-placeholder" key="placeholder">{this.state.placeholder}</div>);
var val = this.state.values[0] || null;
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