-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
prevent bubbling of escape key #2713
Conversation
return true | ||
when 9, 38, 40, 16, 91, 17, 18 | ||
break | ||
when 9, 16, 17, 18, 38, 40, 91 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hero.
if @is_multiple and @backstroke_length < 1 and this.choices_count() > 0 | ||
this.keydown_backstroke() | ||
else if not @pending_backstroke | ||
this.result_clear_highlight() | ||
this.results_search() | ||
when 13 | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add all of these explicit break
s here; CoffeeScript handles that.
Docs:
Switch statements in JavaScript are a bit awkward. You need to remember to break at the end of every case statement to avoid accidentally falling through to the default case. CoffeeScript prevents accidental fall-through...
Confirmed with the "Try CoffeeScript" dingus on http://coffeescript.org/ as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but if you don't add them, CoffeeScript returns a value, and I wanted to get rid of the return true
below, so for consistency I added it everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Fair enough.
switch stroke | ||
when 8 # backspace | ||
@backstroke_length = this.get_search_field_value().length | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto on all of these break
s — can be safely removed.
The biggest question I had that came out of reviewing this is WTF a 👍 on this one, though! |
When the results are showing, pressing escape only hides the results, and then prevents bubbling.
Also introduced an extra method to retrieve the search input value, so that the
keydown_checker
can be moved to the abstract class.This method can also be used as an additional solution for #2370, e.g.:
fixes #1256
fixes #2653