JSHint color [#1684830]

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: color/color.js, color/preview.js

Comment	File	Size	Author
#3	interdiff.txt	885 bytes	tim.plunkett
#3	drupal-1684830-3.patch	9.31 KB	tim.plunkett
#3
#2	core-js-jshint-color.patch	8.29 KB	nod_
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

nod_

French

Lille

CreditAttribution: nod_ commented 14 July 2012 at 14:34

core/modules/color/color.js: line 29, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/modules/color/color.js: line 48, col 46, ['direction'] is better written in dot notation.
core/modules/color/color.js: line 37, col 5, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/modules/color/color.js: line 59, col 14, Bad for in variable 'field_name'.
core/modules/color/color.js: line 59, col 9, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/modules/color/color.js: line 108, col 15, 'd' is already defined.
core/modules/color/color.js: line 139, col 67, Expected '{' and instead saw 'break'.
core/modules/color/color.js: line 144, col 59, Expected '{' and instead saw 'break'.
core/modules/color/color.js: line 173, col 19, Possible strict violation.
core/modules/color/color.js: line 180, col 17, Possible strict violation.
core/modules/color/color.js: line 182, col 21, Possible strict violation.
core/modules/color/color.js: line 221, col 8, Unnecessary semicolon.
core/modules/color/preview.js: line 22, col 101, ['colors'] is better written in dot notation.
core/modules/color/preview.js: line 23, col 99, ['colors'] is better written in dot notation.
core/modules/color/preview.js: line 27, col 81, ['vertical'] is better written in dot notation.
core/modules/color/preview.js: line 26, col 11, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/modules/color/preview.js: line 32, col 18, Bad for in variable 'j'.
core/modules/color/preview.js: line 32, col 13, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.
core/modules/color/preview.js: line 36, col 12, Don't make functions within a loop.
core/modules/color/preview.js: line 21, col 7, The body of a for in should be wrapped in an if statement to filter unwanted properties from the prototype.

Comment #2

nod_

French

Lille

CreditAttribution: nod_ commented 14 July 2012 at 17:16

Status:

Active

» Needs review

File	Size
core-js-jshint-color.patch	8.29 KB

Fix all, works.

Comment #3

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 14 July 2012 at 21:17

File	Size
drupal-1684830-3.patch	9.31 KB

interdiff.txt	885 bytes

I was working on this patch earlier but forgot to upload, I had almost exactly the same thing, but I copied the closure from drupal.js and fixed indentation. If that's out of scope, then #2 is RTBC.

The interdiff is with -w to hide indentation changes.

Comment #4

tim.plunkett

he/him

English

Philadelphia

CreditAttribution: tim.plunkett commented 14 July 2012 at 21:21

+++ b/core/modules/color/preview.jsundefined
@@ -3,39 +3,48 @@
+Drupal.color = {

In #1684906: Drupal.progressBar is a constructor, rename it Drupal.ProgressBar there is a change from Drupal.progressBar to Drupal.ProgressBar, should this be Drupal.Color?

Comment #5

nod_

French

Lille

CreditAttribution: nod_ commented 14 July 2012 at 21:39

Drupal.color is never used as a constructor (it's already an object) so we can't capitalize it. I feel like the closure thing goes overboard. $: yes, Drupal: yes, the rest: not so much.

And that'll make things weird when we'll go towards a module achitecture. So I'd rather not put that with this patch.