search

fmoo / react-typeahead

Pure react-based typeahead and typeahead-tokenizer
ISC License
677 stars 232 forks source link

Make Typeahead a controlled component. Fixes #166 #167

Open nosilleg opened 8 years ago

nosilleg commented 8 years ago

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.

fmoo commented 8 years ago

@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 commented 8 years ago

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.

nosilleg commented 8 years ago

@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?