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

Make Typeahead a controlled component. Fixes #166 #167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nosilleg
Copy link
Contributor

In addition to making Typeahead a controlled component (tracking of the value is done in the parent component and passed in) there are additional fixes where state was being mutated; hasCustomValue was being called redundantly; and some style changes that just enabled me to see the setState calls more clearly.

This is a fairly minimal change as there are additional potential bugs with calling setState then reading this.state which the docs say to avoid. I hope to address those in a future PR.

All tests still pass, but I haven't created any new ones.

this.setState(
{
selected: update(this.state.selected, {$splice: [[index, 1]]})
}
Copy link
Owner

Choose a reason for hiding this comment

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

stylewise, prefer

this.setState({
  selected: update(...)
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update.

@fmoo
Copy link
Owner

fmoo commented Mar 22, 2016

@nosilleg - do you think this is a minor version or release version bump?

And any ideas for unittests that we could add to verify this?

@nosilleg
Copy link
Contributor Author

I would hazard a guess that it's major release. The main fix requires that it's a controlled component, so people using Typeahead without Tokenizer will need to start tracking the value (passing it in and out). I would be tempted to release 1.1.7 that reverts the changes in 1.1.6 (putting the old bug back in) and then release this as 2.x. That way people that depended on the broken (in one way) functionality have expected results in the 1.1 upgrade path.

I say "requires" above simply because I haven't invested enough time to make sure all variations work with a smaller change.

I would also be tempted to release a 2.0.0-beta1 just to get some consensus since the tests don't have a lot of coverage.

fmoo added a commit that referenced this pull request Mar 25, 2016
This reverts commit bda8233.  This
change will be reintroduced (along with #167) in 2.0.x
@nosilleg
Copy link
Contributor Author

@fmoo You asked the question about adding unit tests... I'm not sure a unit test would suffice for this issue. I think functional tests would be required in order to get the full range of events happening on the dom (focus, blur) based on other events (click, keyUp, etc) Do you have a preferred approach to this?

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