-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
onSelectResetsInput regression fixed #2215
Conversation
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.
Hi,
Good catch and please take a look on my comments.
test/Select-test.js
Outdated
@@ -2129,6 +2129,66 @@ describe('Select', () => { | |||
|
|||
}); | |||
|
|||
describe('with multi=true and onSelectResetsInput=false', () => { |
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.
more accurately describe('with multi=true and different onSelectResetsInput', () => {
test/Select-test.js
Outdated
@@ -2129,6 +2129,66 @@ describe('Select', () => { | |||
|
|||
}); | |||
|
|||
describe('with multi=true and onSelectResetsInput=false', () => { | |||
it('should have retained inputValue after accepting selection', () => { |
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('should have retained inputValue after accepting selection if onSelectResetsInput=false ', () => {
test/Select-test.js
Outdated
expect(instance.state.inputValue, 'to equal', 'two'); | ||
}); | ||
|
||
it('should have reset the inputValue after accepting selection', () => { |
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('should have reset the inputValue after accepting selection if onSelectResetsInput= true or not set', () => {
As suggested by @yuri-sakharov
As suggested by @yuri-sakharov
Thanks for the fix @dehamilton and for the review, @yuri-sakharov. I'm about to publish a new version so I've made the changes you suggested directly, so I can get this merged 👍 |
@@ -102,7 +102,7 @@ class Select extends React.Component { | |||
this.setState({ required: false }); | |||
} | |||
|
|||
if (this.state.inputValue && this.props.value !== nextProps.value) { | |||
if (this.state.inputValue && this.props.value !== nextProps.value && this.props.onSelectResetsInput) { |
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.
I think better if (this.state.inputValue && this.props.value !== nextProps.value && nextProps.onSelectResetsInput) {
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.
Hi, @JedWatson
Sorry I didn't see the merge. Can you please also add this change?
Thank you.
Recent changes to componentWillReceiveProps prevented the input from retaining values even if onSelectResetsInput was false. This PR resolves that regression and adds tests around onSelectResetsInput, which I didn't notice anywhere. Also updated package.json to use cross-env for all scripts.