[Do not merge][Fivel] Keyboard interactions plugin #315
Conversation
| sequences[ 2 ] = sequences[ 2 ] || []; | ||
|
|
||
| var container = _rootElement.querySelector( ".editor-options" ), | ||
| pluginOptions = {}, |
There was a problem hiding this comment.
I think it's confusing to have both _pluginOptions (line 45) and pluginOptions in the same scope. Can one of these be renamed?
| importMousetrap.type = "text/javascript"; | ||
|
|
||
| document.head.appendChild( importMousetrap ); | ||
| importMousetrap.onload = function() { |
There was a problem hiding this comment.
I agree. Let's not wrap this entire block into an onload script.
A better way to do this would be like our GoogleMaps plugin.
It first has checks to see if the script is loaded, like so https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/plugins/googlemap/popcorn.googlemap.js?source=c#L207-L209. It waits for the document to have a <body> and then adds the maps api script.
Afterwards it uses a polling modules type system seen https://github.com/mozilla/popcorn.webmaker.org/blob/master/public/templates/assets/plugins/googlemap/popcorn.googlemap.js?source=c#L166-L191 to finish the setup once it knows the maps script is present.
There was a problem hiding this comment.
You said, "It waits for the document to have a <body>..." and in the code you link to, it appears to be a googlemaps specific requirement to have the body loaded first. Is that a check I should be making?
There was a problem hiding this comment.
No. Correct me if I'm wrong but this script probably puts something on window correct? Check there and against that value.
| } | ||
|
|
||
| function bindAll( callback ) { | ||
| if ( !callback ){ |
There was a problem hiding this comment.
You don't really need this check do you? You control how/when this function get's called. You only call it in one place and it passes a callback.
ref: https://bugzilla.mozilla.org/show_bug.cgi?id=892815