The title of this pull request changed from “FEATURE: Bookmark keyboard shortcuts and next business day changes” to "FEATURE: Bookmark keyboard shortcuts
never use setTimeoit, prefer Ember.run.later
setTimeout doesnt play well with ember runloop. Also in this case Ember.run.next will be enough probably.
You shouldnt reach container through the global
I have a doubt this will work on IE do we do it at other places?
we have a setting helper for this kind of properties
could be one line
Also why do we need this ? and not just
I guess you tried and it wasn’t working, do you have an explanation of why ?
I know you just hooked into the existing here, but could we set shortcuts from onOpen, and ensure it’s nullified onClose I don’t feel great about this gigantic hash set on the controller
could be one line
could probably use http://api.emberjs.com/ember/3.15/functions/@ember%2Fobject%2Fcomputed/and here
this is personal preference and very minor nitpick but I prefer
e as it’s way easier to grep for it in the codebase.
Wondering, does it work:
is there any chance this doesn’t exist?
might be worth to have a css class here, otherwise targeting anything here will be complicated
same question I wonder if it works:
The only thing change required here is the :
Nitpick: why not use longer lines for the comments here? Editors should be at least 80 chars wide.
Should be BOOKMARK_BINDINGS or something uppercase.
I don’t like using global modules like this. Instead I would prefer if you exposed an API in the
keyboard-shortcuts module that would allow you to do what you need.
KeyboardShortcuts.pause(GLOBAL_SHORTCUTS_TO_PAUSE); KeyboardShortcuts.addBindings(BOOKMARK_BINDINGS); // and then to restore: KeyboardShortcuts.removeBindings(BOOKMARK_BINDINGS); KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE);