Alasdair/fedx 36 dropdown menu added arrow support - #31
Conversation
d1d638a to
daa51d2
Compare
|
@dsjen if you have a minute could you please review this PR? |
|
|
||
| handlerIsAction: function(key, $el) { | ||
| if (key === constants.keyCodes.up) { | ||
| if (key === constants.keyCodes.up || key === constants.keyCodes.left) { |
There was a problem hiding this comment.
This is just a suggestion. Have you thought about using underscore's contain? e.g. _([constants.keyCodes.up, constants.keyCodes.left]).contains(key). Maybe even creating member variables and using them:
previewKeys = [constants.keyCodes.up, constants.keyCodes.left];
...
if (_(this.previousKeys).contains(key) {...}
It may be a bit overkill for two keys though!
|
I left two questions that are just "nits." I think most code paths will be hit with the current tests. My last suggestion is to add a test that hits the LGTM! 👍 |
e6aa64a to
ad62dca
Compare
| var DropdownMenuView = Backbone.View.extend({ | ||
| tpl: _.template(DropdownTpl), | ||
|
|
||
| // For event handling only instances do I need to scope this somehow? |
There was a problem hiding this comment.
These events will only be scoped to the root element of the view:
http://backbonejs.org/#View-events
I think that's what you want. If so, you can just remove this comment.
|
Some minor nits from me, but looks great overall. 👍 |
fe8c968 to
3573e73
Compare
b986e40 to
eb2e691
Compare
…arrow-support Alasdair/fedx 36 dropdown menu added arrow support
3 participants
@andy-armstrong please review. This PR adds the left and right arrow support to scroll through the dropdown list (as requested 2 hackathons ago) and it adds
space barclose andtab+shiftcapture as per Christine's PR (https://github.com/edx/edx-platform/pull/11393)