-
Notifications
You must be signed in to change notification settings - Fork 233
Add autoselect feature #54
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
Conversation
|
Looks good to me altho I didn't run it. Might want to update the docs too. By the way, one way to make the maintainers perhaps more comfortable might be to demo the old and new behavior with a screencast http://www.screencast.com/ This seems straightforward enough tho... |
|
I've updated the typeahead docs here I feel this feature is clear enough to not require a screencast, but happy to do so if you feel it's necessary. |
|
@jcrben It has been working as expected in production. |
|
+1 love to see this merged in |
|
actually, @DanielApt, I built this myself and caught a bug. The onblurred event doesn't have any arguments passed in and thus the referenced $e is undefined. Also there are situations when the event is called and the $e argument is undefined. I traced this back to input.js#74: _.mixin(Input.prototype, EventEmitter, {
_onBlur: function onBlur() {
this.resetInputValue();
this.trigger("blurred");
},This event, and really all others, should pass along the event so it doesn't break standard js event model. |
|
|
@synth was the event passing issue introduced by this PR or was it a pre-existing 'feature' of the previous code-base? If it is pre-existing and adding the event argument later will not cause a regression, I think this should be merged in. Or am I misunderstanding the situation? |
|
@MarkHerhold well, this code hasn't changed from twitter's codebase, so I'm pretty sure its always existed this way. 🚀 merge |
|
Sorry, been a bit out of the loop on this one. Would you want me to add @synth's code snippet to my PR? |
|
Moving this to the 0.11.3 milestone, since it's not clear if this is ready to go. @DanielApt and @synth is the onBlur event issue resolved? |
|
Just migrated from the old twitter typeahead for other reasons, but our clients miss this feature. Looking forward to seeing it in the next release. |
|
+1 when can we see this merged? |
|
@JonathanSeguin, When the merge conflicts are resolved. 😉 I likely won't have time to look at this in the near future. If you or @hutch120 think you're up for it, feel free to resolve the conflicts and submit a new PR. I haven't even had a chance to see how extensive the conflicts are (could be a fairly simple fix). |
|
This was recently merged. Move to close. |
|
Thanks guys! Since I added this feature into other repo and haven't check its committed in here... |
Closes #53