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.

Sign up for GitHub

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

Jump to bottom

[AutoComplete] Fix undesired behaviour with controlled searchText #6621

Merged
merged 1 commit into from
Apr 19, 2017
Merged

[AutoComplete] Fix undesired behaviour with controlled searchText #6621

merged 1 commit into from
Apr 19, 2017

Conversation

NickMalt
Copy link

this is resolve #6618

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

oliviertassinari
oliviertassinari requested changes Apr 16, 2017
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

👍 for no using the searchText state when the component is controlled.

src/AutoComplete/AutoComplete.js Outdated
this.setState({
searchText: searchText,
}, () => {
this.props.onUpdateInput(searchText, this.props.dataSource, {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we always call onUpdateInput?

Copy link
Author

Choose a reason for hiding this comment

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

You mean why we don't call onUpdateInput when searchText is controlled? If so, it's seems logical for me to not call it in this case (because we don't change input actually). If user wants to change searchText, he will do it in onNewRequest handler.

Copy link
Member

@oliviertassinari oliviertassinari Apr 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sure but that's a breaking change with the current behavior. Users can use the options argument to filter out that call.
I think that you are right but not introducing a breaking change is even more important.

Copy link
Author

Choose a reason for hiding this comment

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

got your point, done

src/AutoComplete/AutoComplete.js Outdated
source: 'touchTap',
});

this.timerTouchTapCloseId = setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep that DRY?

Copy link
Author

Choose a reason for hiding this comment

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

done

@oliviertassinari oliviertassinari added component: AutoComplete PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 16, 2017
oliviertassinari
oliviertassinari previously requested changes Apr 16, 2017
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Aside from my comment, I think that this PR is going in the right direction.
We still use the searchText state in many locations, like in the handleChange function. But as we plan deep refactorisation for the next branch, it sounds better to do as little change as possible in order to reduce the risk of fixing that issue.

src/AutoComplete/AutoComplete.js Outdated
this.setState({
searchText: searchText,
}, () => {
this.props.onUpdateInput(searchText, this.props.dataSource, {
Copy link
Member

@oliviertassinari oliviertassinari Apr 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Same, can we keep that DRY?

Copy link
Author

Choose a reason for hiding this comment

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

done, sorry for long reply

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed PR: Needs Review labels Apr 16, 2017
@NickMalt NickMalt force-pushed the controlledAutoComplete branch from 55b0448 to 3dad6d5 Compare April 18, 2017 14:03
@oliviertassinari oliviertassinari dismissed their stale review April 18, 2017 18:36

looks good.

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Apr 18, 2017
@oliviertassinari oliviertassinari merged commit 3b36e24 into mui:master Apr 19, 2017
@oliviertassinari
Copy link
Member

Thanks.

@oliviertassinari oliviertassinari changed the title [AutoComplete] fix undesired behaviour with controlled searchText [AutoComplete] Fix undesired behaviour with controlled searchText Apr 30, 2017
@keithamoss keithamoss mentioned this pull request May 24, 2017
Closed
@oliviertassinari oliviertassinari added the component: autocomplete This is the name of the generic UI component, not the React module! label Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[AutoComplete] controlled searchText
3 participants
@NickMalt @oliviertassinari @muibot