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

Fix Typeahead focus out #210

Merged
merged 1 commit into from
Apr 20, 2015
Merged

Fix Typeahead focus out #210

merged 1 commit into from
Apr 20, 2015

Conversation

diogotrentini
Copy link
Contributor

Hey guys!

I found a bug on the typeahead directive: it's not updating the loading callback if the input field loses focus. You can test it on http://plnkr.co/edit/VZOjJLO4nZ5HMWXrl9Wc?p=preview, just type something and immediately focus out. I just changed when the directive updates the loading callback and it's working. 😄

@diogotrentini diogotrentini changed the title Fix Typeahead Fix Typeahead focus out Apr 13, 2015
@jbrowning
Copy link
Member

Thanks @diogotrentini. What's the use case for this? I'd argue that the typeahead should be closed if the field loses focus.

@diogotrentini
Copy link
Contributor Author

Hi @jbrowning! It's happening while waiting for a async request (that's why I put a $timeout on the plunker, so I can simulate a async function). So the use case is something like this: I'm fetching via AJAX the items that I want to display on typeahead and using typeahead-loading to show a spinner. If I focus out when the AJAX is still running, the spinner will not hide (typahead-loading will not update, ever).

@jbrowning
Copy link
Member

Got it. Makes sense. I would also test that typeahead-loading is set to false if the promise is rejected because this change causes that too.

@diogotrentini
Copy link
Contributor Author

Sorry for the delay. Actually, isLoadingSetter(originalScope, false); was being called on the promise's rejection, even before this update. Even though I added a test, just to make sure (and because there aren't tests about this case). (:

@jbrowning
Copy link
Member

Looks great. Thanks! 👍

jbrowning added a commit that referenced this pull request Apr 20, 2015
@jbrowning jbrowning merged commit d1a3ae1 into yalabot:master Apr 20, 2015
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