Follow-up of #1664940: [Policy, patch] Decide on JSHint configuration and part of #1415788: [Meta] 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

Files: 
CommentFileSizeAuthor
#17 core-jshint-tabledrag-1684812-17.patch728 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 56,760 pass(es).
[ View ]
#8 core-js-jshint-tabledrag-1684812-8.patch16.73 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 40,706 pass(es).
[ View ]
#7 1684812-jshint-tabledrag-7.patch17.3 KBseutje
PASSED: [[SimpleTest]]: [MySQL] 40,556 pass(es).
[ View ]
#4 tabledrag-jshint.patch15.99 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 37,294 pass(es).
[ View ]

Comments

nod_’s picture

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

Status:Active» Postponed

hold off until rewrite

nod_’s picture

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

Status:Active» Needs review
Issue tags:+Needs manual testing
StatusFileSize
new15.99 KB
PASSED: [[SimpleTest]]: [MySQL] 37,294 pass(es).
[ View ]

Please test it carefully, thanks :)

droplet’s picture

Status:Needs review» Needs work
seutje’s picture

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

StatusFileSize
new17.3 KB
PASSED: [[SimpleTest]]: [MySQL] 40,556 pass(es).
[ View ]

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

Assigned:seutje» Unassigned
Status:Needs work» Needs review
StatusFileSize
new16.73 KB
PASSED: [[SimpleTest]]: [MySQL] 40,706 pass(es).
[ View ]

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

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

will review after some shut-eye

seutje’s picture

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

+++ 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

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

nod_’s picture

catch’s picture

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

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

Status:Needs work» Needs review
StatusFileSize
new728 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,760 pass(es).
[ View ]
droplet’s picture

Status:Needs review» Reviewed & tested by the community

Tested, no errors.

alexpott’s picture

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.