Follow-up of #1664940: [Policy, patch] Decide on JSHint configuration and part of #1415788: Javascript winter clean-up

Run jshint on the files with the configuration from the parent issue or use jshint.com with the following options: 

/*jshint forin:true, noarg:true, eqeqeq:true, undef:true, curly:true, browser:true, expr:true, latedef:true, newcap:true, trailing:true */
/*global Drupal, jQuery */

Fix any warnings or errors the tool finds.
Check manually that the fixes did not break any functionalities
Create patch and upload for the testbot.

Files: states.js

CommentFileSizeAuthor
#6 core-js-jshint-states-1684810-6.patch2.77 KBnod_
#4 core-js-jshint-states-1684810-4.patch2.63 KBnod_
#2 core-js-jshint-states.patch2.62 KBnod_

Comments

nod_’s picture

nod_
Primary language French
Location Lille
commented 
core/misc/states.js: line 23, col 7, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/states.js: line 22, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/states.js: line 54, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/states.js: line 113, col 10, Don't make functions within a loop.
core/misc/states.js: line 550, col 27, 'invert' is already defined.
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs review
StatusFileSize
new core-js-jshint-states.patch2.62 KB
droplet’s picture

droplet commented 
// Inverts a (if it's not undefined) when invert is true.
function invert (a, invert) {
  return (invert && typeof a !== 'undefined') ? !a : a;
}

It inverts A. Why we call 2nd parameter "invert"

Shouldn't it:
function invert (val, condition) {

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-jshint-states-1684810-4.patch2.63 KB

it's not really a condition, it's more like the second parameter of the .toggleClass() function of jQuery. They call that stateVal. I went with invertState seems good enough.

droplet’s picture

droplet commented
Status: Needs review » Needs work 
+++ b/core/misc/states.jsundefined
@@ -547,8 +555,8 @@ function ternary (a, b) {
 // Inverts a (if it's not undefined) when invert is true.

when "invert" is true

little problem, other part are fine.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-js-jshint-states-1684810-6.patch2.77 KB

thanks :)

sxnc’s picture

sxnc commented

I ran JSHint locally using SublimeText 2 and had no issues, everything seems to have worked fine otherwise as well :)

pivica’s picture

pivica commented
Assigned: Unassigned » pivica

Taking this one to review.

pivica’s picture

pivica commented
Status: Needs review » Reviewed & tested by the community

jshint runs cleanly now with patch from comment 6 and states in browser works as expected (tested on admin/config/regional/settings checked/visibility states).

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented

Why are we doing this?

+    var state, self = this;
+
+    function stateEventHandler(e) {
+      self.update(e.data.selector, e.data.state, e.value);
+    }

instead of replace the "self" occurrences by "this"?

+    function stateEventHandler(e) {
+      this.update(e.data.selector, e.data.state, e.value);
+    }
nod_’s picture

nod_
Primary language French
Location Lille
commented

Because that won't work. this here resolves to the DOM element it's been bound to, this element won't have the update method available.

pivica’s picture

pivica commented

Yeah you need to use js clousures for this kind of thing, take a look here http://stackoverflow.com/a/111200 for more detail explanation.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs review

Sounds like this still needs more discussion..?

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

nope just some JS weirdness oriol_e9g didn't catch, it's good to go

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ok, committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.