FEATURE: Bookmark keyboard shortcuts (PR #9318)

GitHub

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 ? :stuck_out_tongue: and not just this.set("name", null)

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 event over e as it’s way easier to grep for it in the codebase.

Wondering, does it work:

 GLOBAL_SHORTCUTS_TO_PAUSE.forEach(Mousetrap.unbind)

?

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:

Object.keys(bindings).forEach(this.mouseTrap.unbind);

It should

The only thing change required here is the : setTimeout

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.

For example:

KeyboardShortcuts.pause(GLOBAL_SHORTCUTS_TO_PAUSE);
KeyboardShortcuts.addBindings(BOOKMARK_BINDINGS);

// and then to restore:

KeyboardShortcuts.removeBindings(BOOKMARK_BINDINGS);
KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE);