Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

Typeahead: Prevent lookup until user finishes typing #594

Closed
wants to merge 1 commit into from

Conversation

jroxendal
Copy link
Contributor

A fix for bug #573, though not through modifying typeahead-wait-ms. Instead, I've added typeahead-throttle-ms, so that:

<input typeahead="item in getAsync($viewValue) | filter:$viewValue" typahead-throttle-ms="500">

will only send one request if the user types a string in under 500 milliseconds. this should work in conjunction with wait-ms, for what it's worth.

Thoughts/suggestions?

@pkozlowski-opensource
Copy link
Member

@jroxendal Thnx for the PR - I must say that I really do appreciate bug-fix PR, this is amazing. Having said this I think that it adds way too much code for the functionality. I hope that we can fix it with the wait-ms only. Would be awesome if you could give it a try!

@jroxendal
Copy link
Contributor Author

@pkozlowski-opensource sure thing, shouldn't be too much work. So you'd prefer the behavior to follow that of underscore/lodash debounce? From what I understood of the semantics of wait-ms, the countdown starts after all of the input is finished, then the lookup is evaluated at the end of countdown. throttle-ms has slightly different semantics, where the countdown length is equal to input time - delay. I don't think it matters too much, we just have to pick one, so my suggestion is:

  1. at first input, start countdown.
  2. if more input comes before end of countdown, restart countdown. ( <- my throttle doesn't do this)
  3. at end of countdown, fire lookup.

come to think of it, this seems better :) assuming this is what you mean, i'll get right on it.

@pkozlowski-opensource
Copy link
Member

@jroxendal What you've listed in (1) - (3) is exactly what I've meant. So it currently works like this the only problem being that (2) queues lookups and fires them all at (3). Instead we should fire only one look-up corresponding to the last input.

@pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource
Copy link
Member

@jroxendal I think that this is going in the right direction but will require more work - I've left some comments in the PR. Thnx!

@jroxendal
Copy link
Contributor Author

that should do it. we'll have to see what travis has to say, but i pass all the tests here at least.

@pkozlowski-opensource
Copy link
Member

@jroxendal This looks much better, we are almost here! Could you just please rebase it on the current master and squash the 2 commits into one? Thank you!

@jroxendal
Copy link
Contributor Author

@pkozlowski-opensource I believe that did the trick.

@pkozlowski-opensource
Copy link
Member

@jroxendal Finally I've decided to merge #607 as it was less code changes. Thnx for your contribution though, very much appreciated.

Could you check if 90a8aa7 fixes things for you?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants