Conversation
Codecov Report
@@ Coverage Diff @@
## master #4435 +/- ##
=======================================
Coverage 94.72% 94.72%
=======================================
Files 261 261
Lines 22847 22847
Branches 1673 1673
=======================================
Hits 21642 21642
Misses 981 981
Partials 224 224 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of items. Mostly nits.
mdn.Notifier.growl('You can now use ' + matchStored + ' to sign in to this MDN profile.', { duration: 0, closable: true }).success(); | ||
}); | ||
localStorage.removeItem(matchKey); | ||
catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did eslint
no complain about the unused variable e
? Safe to remove as we are not using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we don't use it it will bubble up. The linter did not object.
var matchStored = localStorage.getItem(matchKey); | ||
|
||
// The user is on the registration page and has been notified that | ||
// there is an MDN profile with a matching email address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: For multi line comments we should use the /** The name of the module. */
JSDoc standard. This will then also match how comments are done on Bedrock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write or edit the comment, not sure why it shows up as a change. I'd like to leave it alone and just make the linter happy.
kuma/static/js/auth.js
Outdated
var serviceCurrent = $(doc.body).data(serviceKey); | ||
|
||
try { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: extra empty line
else if(!serviceCurrent && serviceStored) { | ||
localStorage.removeItem(serviceKey); | ||
$doc.trigger('mdn:logout', [ serviceStored ]); | ||
catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unused parameter e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we don't use it it will bubble up. The linter did not object.
if(!isRemove) listItems.push(li); | ||
if(!isRemove) { | ||
listItems.push(li); | ||
} | ||
|
||
// Cycling through each list item, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could this comment be more descriptive? Perhaps we simply do not need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't write or edit the comment, not sure why it shows up as a change. I'd like to leave it alone and just make the linter happy.
f1cc362
to
87708a8
Compare
Updated. I'd like to merge to make the linter happy without updating the comments, they can be addressed later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+ ⌨️
No description provided.