[bug 1115229] Simple snippet template for Hello #29
Conversation
ckprice
force-pushed
the
bug-1115229-hello-simple-template
branch
from
374e511
to
10d6143
Compare
| ;(function($) { | ||
| 'use strict'; | ||
|
|
||
| // create namespace |
There was a problem hiding this comment.
We don't need the entire UITour library included. Take, for example, https://github.com/mozilla/snippets-service/blob/79eaa9aef4caca9329af5780aa1374019e78916c/snippets/base/templates/base/includes/snippet_js.html#L314, where we just create the custom event directly instead of including or relying on the UITour library.
You should be able to create the custom events and add the event listeners you need directly. It'll be less code and easier to read.
There was a problem hiding this comment.
Assuming you were alluding to the fact that I left some UITour declarations here that I wasn't using in the code. It's been cleaned up.
|
You might want to read up on our JS style guide, there's a few things in here that should be fixed to follow it: http://mozweb.readthedocs.org/en/latest/reference/js-style.html |
| return ret; | ||
| }; | ||
|
|
||
| var bindHelloObserver = function() { |
There was a problem hiding this comment.
No need to assign these to variables, you can function bindHelloObserver fine here.
|
Nice first pass! Let me know when you update and I'll go over it again. |
ckprice
force-pushed
the
bug-1115229-hello-simple-template
branch
2 times, most recently
from
b353f9d
to
ae6c6df
Compare
|
@Osmose messaged you in IRC, but adding comment here as well. Updates made. |
| (function() { | ||
| var snippet = document.getElementById('{{ snippet_id }}-hello-simple'); | ||
| snippet.addEventListener('show_snippet', function() { | ||
| Mozilla.UITour.trackHelloStatus (); |
There was a problem hiding this comment.
No space between method name and parenthesis. Bunch of these in the file, this is one of the things I was referring to when I recommended reading the style guide. Style guide issues are me nitpicking though.
ckprice
force-pushed
the
bug-1115229-hello-simple-template
branch
from
ae6c6df
to
ca12352
Compare
|
@Osmose thanks for your help today. Updates are in. |
ckprice
force-pushed
the
bug-1115229-hello-simple-template
branch
from
ca12352
to
b953b0a
Compare
| // bug 1113896 | ||
| document.addEventListener('click', function(e) { | ||
| Mozilla.UITour.hideMenu('loop'); | ||
| document.removeEventListener('click', this); |
There was a problem hiding this comment.
Unless I'm misreading the docs, shouldn't this refer DOM element the listener is attached to? removeEventListener requires you to pass the callback function and the same useCapture value as you used in addEventListener.
There was a problem hiding this comment.
Actually adding the removeEventListener correctly breaks this implementation. Adding the event listener in a callback to the showMenu call above is correct.
| sendMetric('hello-open'); | ||
| }, false); | ||
| } | ||
| }, function () { |
There was a problem hiding this comment.
Tiny readability tip: If you give this function a name, like "loopNotAvailable", it'll be easier for someone reading the code to notice that this is the failure function handler for the isTargetAvailable call. I've never actually done that before myself but I think I'll start!
|
r+wc, this has really come a long way and looks great! Nice work! |
ckprice
force-pushed
the
bug-1115229-hello-simple-template
branch
from
b953b0a
to
936c1fc
Compare
|
@Osmose updates made! |
@Osmose r?
Link to sample: https://snippets.allizom.org/admin/base/snippet/77/