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: tabledrag.js

CommentFileSizeAuthor
#17 core-jshint-tabledrag-1684812-17.patch728 bytesnod_
#8 core-js-jshint-tabledrag-1684812-8.patch16.73 KBnod_
#7 1684812-jshint-tabledrag-7.patch17.3 KBseutje
#4 tabledrag-jshint.patch15.99 KBdroplet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented 
core/misc/tabledrag.js: line 23, col 8, Don't make functions within a loop.
core/misc/tabledrag.js: line 18, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 62, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 61, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 129, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 139, col 9, 'hidden' used out of scope.
core/misc/tabledrag.js: line 139, col 19, 'cell' used out of scope.
core/misc/tabledrag.js: line 142, col 25, 'cell' used out of scope.
core/misc/tabledrag.js: line 142, col 58, 'cell' used out of scope.
core/misc/tabledrag.js: line 163, col 8, Don't make functions within a loop.
core/misc/tabledrag.js: line 127, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 250, col 7, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 245, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 417, col 29, 'groupHeight' is already defined.
core/misc/tabledrag.js: line 544, col 11, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 541, col 7, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 571, col 5, '$droppedRow' used out of scope.
core/misc/tabledrag.js: line 626, col 21, 'rowHeight' is already defined.
core/misc/tabledrag.js: line 630, col 22, 'rowHeight' used out of scope.
core/misc/tabledrag.js: line 630, col 50, 'rowHeight' used out of scope.
core/misc/tabledrag.js: line 633, col 20, 'n' is already defined.
core/misc/tabledrag.js: line 672, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/misc/tabledrag.js: line 702, col 19, 'sourceRow' is already defined.
core/misc/tabledrag.js: line 727, col 22, '$previousRow' is already defined.
core/misc/tabledrag.js: line 728, col 21, 'previousRow' is already defined.
core/misc/tabledrag.js: line 735, col 7, 'sourceRow' used out of scope.
core/misc/tabledrag.js: line 743, col 7, 'sourceRow' used out of scope.
core/misc/tabledrag.js: line 744, col 11, 'sourceRow' used out of scope.
core/misc/tabledrag.js: line 745, col 9, 'sourceRow' used out of scope.
core/misc/tabledrag.js: line 753, col 24, 'sourceRow' used out of scope.
core/misc/tabledrag.js: line 758, col 7, 'useSibling' used out of scope.
core/misc/tabledrag.js: line 769, col 40, 'sourceRow' used out of scope.
core/misc/tabledrag.js: line 951, col 10, Don't make functions within a loop.
core/misc/tabledrag.js: line 1120, col 39, 'checkRowIndentation' used out of scope.
core/misc/tabledrag.js: line 1123, col 18, 'checkRowIndentation' used out of scope.
core/misc/tabledrag.js: line 1147, col 3, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.

See #1524414: Rewrite tabledrag.js to use jQuery UI as a proper way of addressing the architecture issues on top of cleaning up.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Active » Postponed

hold off until rewrite

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Postponed » Active
Issue tags: +js-novice

Actually no, that's not a bad thing to do in the meantime. there are a lot of things to fix but they should be pretty easy.

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Active » Needs review
Issue tags: +Needs manual testing
FileSize
tabledrag-jshint.patch15.99 KB

Please test it carefully, thanks :)

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Needs work
seutje’s picture

seutje CreditAttribution: seutje commented
Assigned: Unassigned » seutje

Firefox breaks on TypeError: $indentation.get(1) is undefined line 94
Chrome on Uncaught TypeError: Cannot read property 'offsetLeft' of undefined line 94

investigating...

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
1684812-jshint-tabledrag-7.patch17.3 KB

I fixed a few scoping issues created by abstracting anonymous, but for some reason self.table ends up being a jQuery collection instead of a reference to an HTML element which screws up the call to tBodies on line 652, but I can't figure out why (or where) it's being turned into a jQuery collection.

After staring at it for almost 2 hours now, I'm gonna let it rest for a while. Feel free to pick this up further, if nobody does, I'll look into this again in a day or so, cause right now, I'm blind-staring and I have to head home in a minute.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Assigned: seutje » Unassigned
Status: Needs work » Needs review
FileSize
core-js-jshint-tabledrag-1684812-8.patch16.73 KB

umm don't add the tabledragHandle to the behavior object I'd like to avoid having anything beside attach/detach in there.

Check what you send to Drupal.tableDrag in attachBehaviors should not be a jQuery object :D

seutje’s picture

seutje CreditAttribution: seutje commented

oh god, of course... sleep-deprivation--

will review after some shut-eye

seutje’s picture

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

Manually tested on chrome, ff and a (real) IE8, functionality works as expected.

Good to go, if you ask me!

droplet’s picture

droplet CreditAttribution: droplet commented 
+++ b/core/misc/tabledrag.jsundefined
@@ -1156,11 +1199,13 @@ Drupal.tableDrag.prototype.row.prototype.findSiblings = function (rowSettings) {
+        .removeClass('tree-child')
+        .removeClass('tree-child-first')
+        .removeClass('tree-child-last')
+        .removeClass('tree-child-horizontal');

It can be oneline.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

yeah but that's not necessary to fix the jshint errors. We can do that in the rewrite issue.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

#8: core-js-jshint-tabledrag-1684812-8.patch queued for re-testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Thanks for the manual testing . Committed/pushed to 8.x.

Status: Fixed » Closed (fixed)

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

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Closed (fixed) » Needs work

New JSHint config #1995996: Update JSHint configuration.

core/misc/tabledrag.js: line 525, col 2, Missing semicolon.
core/misc/tabledrag.js: line 674, col 19, 'indentDiff' is defined but never used.
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
FileSize
core-jshint-tabledrag-1684812-17.patch728 bytes
droplet’s picture

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

Tested, no errors.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed e52b50f and pushed to 8.x. Thanks!

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