JSHint modules/views/views_ui/js/views-admin.js [#1835102]

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 functionality.
Create patch and upload for the testbot.

Files: modules/views/views_ui/js/views-admin.js

Comment	File	Size	Author
#18	core-js-jshint-views_admin-1835102-17.patch	8.52 KB	nod_
#18
#14	drupal-1835102-14.patch	12.64 KB	dawehner
#14
#14	interdiff.txt	1.12 KB	dawehner
#13	1835102-13.patch	12.35 KB	damiankloip
#13
#13	interdiff.txt	5.35 KB	damiankloip
#10	core-js-jshint-views-views-ui-views-admin2.patch	7.77 KB	rballou
#10
#7	interdiff.txt	509 bytes	dawehner
#7	drupal-1835102-7.patch	7.84 KB	dawehner
#7
#6	interdiff.txt	502 bytes	dawehner
#6	drupal-1835102-6.patch	8.11 KB	dawehner
#6
#2	core-js-jshint-views-views-ui-views-admin.patch	8.1 KB	rballou
#2

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

rballou CreditAttribution: rballou commented 8 November 2012 at 03:55

Initial pass for JSHint:

views-admin.js: line 1016, col 47, A leading decimal point can be confused with a dot: '.85'.
views-admin.js: line 1016, col 50, Missing radix parameter.
views-admin.js: line 1017, col 47, A leading decimal point can be confused with a dot: '.6'.
views-admin.js: line 1017, col 49, Missing radix parameter.
views-admin.js: line 1025, col 49, A leading decimal point can be confused with a dot: '.8'.
views-admin.js: line 1025, col 51, Missing radix parameter.
views-admin.js: line 1040, col 54, Missing radix parameter.
views-admin.js: line 1041, col 57, Missing radix parameter.
views-admin.js: line 1055, col 52, Missing radix parameter.
views-admin.js: line 1056, col 55, Missing radix parameter.
views-admin.js: line 1101, col 3, Missing semicolon.
views-admin.js: line 569, col 8, 'i' is not defined.
views-admin.js: line 569, col 15, 'i' is not defined.
views-admin.js: line 569, col 27, 'i' is not defined.
views-admin.js: line 570, col 56, 'i' is not defined.
views-admin.js: line 769, col 8, 'i' is not defined.
views-admin.js: line 769, col 15, 'i' is not defined.
views-admin.js: line 769, col 46, 'i' is not defined.
views-admin.js: line 772, col 46, 'i' is not defined.
views-admin.js: line 12, col 70, 'settings' is defined but never used.
views-admin.js: line 12, col 60, 'context' is defined but never used.
views-admin.js: line 29, col 69, 'settings' is defined but never used.
views-admin.js: line 221, col 15, 'length' is defined but never used.
views-admin.js: line 258, col 81, 'settings' is defined but never used.
views-admin.js: line 298, col 54, 'event' is defined but never used.
views-admin.js: line 397, col 69, 'event' is defined but never used.
views-admin.js: line 441, col 69, 'settings' is defined but never used.
views-admin.js: line 471, col 77, 'settings' is defined but never used.
views-admin.js: line 880, col 75, 'settings' is defined but never used.
views-admin.js: line 893, col 73, 'settings' is defined but never used.
views-admin.js: line 893, col 63, 'context' is defined but never used.
views-admin.js: line 911, col 81, 'settings' is defined but never used.
views-admin.js: line 911, col 71, 'context' is defined but never used.
views-admin.js: line 961, col 66, 'e' is defined but never used.
views-admin.js: line 973, col 76, 'settings' is defined but never used.

35 errors

File	Size
core-js-jshint-views-views-ui-views-admin.patch	8.1 KB

Comment #3

nod_

French

Lille

CreditAttribution: nod_ commented 8 November 2012 at 04:07

Status:

Needs review

» Reviewed & tested by the community

looks good

Comment #4

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 12 November 2012 at 09:00

Status:

Reviewed & tested by the community

» Needs review

This is a metric ton of changes. Has anyone tested this manually to ensure things still work?

Comment #5

nod_

French

Lille

CreditAttribution: nod_ commented 12 November 2012 at 15:17

Status:

Needs review

» Reviewed & tested by the community

I know it was short, but I did test it before RTBC. Lots of unused variables removed by this patch and variables got declared, it's straightforward.

Retested just to make sure, all good.

Comment #6

dawehner

German

CreditAttribution: dawehner commented 12 November 2012 at 15:23

Status:

Reviewed & tested by the community

» Needs review

File	Size
drupal-1835102-6.patch	8.11 KB

interdiff.txt	502 bytes

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -221,7 +221,7 @@ Drupal.viewsUi.addItemForm.prototype.handleCheck = function (event) {
-    for (var i = 0; i < this.checkedItems.length; i++) {
+    for (var i = 0; i < length; i++) {
       if (i === position) {
         this.checkedItems.splice(i, 1);

This code alters the array, so i'm really not sure whether this is safe.

File	Size
drupal-1835102-7.patch	7.84 KB

interdiff.txt	509 bytes

Comment #8

nod_

French

Lille

CreditAttribution: nod_ commented 12 November 2012 at 15:39

That was a good catch, This code can be triggered by adding a new filter condition and selecting/unselecting a checkbox. It controls the display of the "Selected :" box above the forms controls.

I'll let someone else RTBC :p

Comment #9

nod_

French

Lille

CreditAttribution: nod_ commented 13 November 2012 at 07:58

Status:

Needs review

» Needs work

Can it be rereolled without the whole length thing and keep the this.checkedItems as before? so that it's straightforward to RTBC?

We'll take care of adding manual tests and using a proper construct to not abusing a for loop in the refactor issue later on #1839130: Refactor modules/views_ui/js/views-admin.js. Having the counter as well as the max value being altered inside the for loop sounds really bad :p

Comment #10

rballou CreditAttribution: rballou commented 13 November 2012 at 14:29

Status:

Needs work

» Needs review

File	Size
core-js-jshint-views-views-ui-views-admin2.patch	7.77 KB

Here's a patch that keeps this.checkedItems and removes the length variable from that section of code. Thanks for catching that it was changing the array in the loop... I missed that!

Also, I am working on getting the Test Swarm module working to make testing some of this easier (you can view it for currently committed D8 work, but this way I can check before/after some changes).

Comment #11

dawehner

German

CreditAttribution: dawehner commented 13 November 2012 at 15:29

This looks good now, but we need still some manual testing.

Comment #12

droplet CreditAttribution: droplet commented 22 December 2012 at 15:00

Status:

Needs review

» Needs work

space problems (http://drupal.org/node/172169/#declarations) & redundant "var height = 0;"

Comment #13

damiankloip CreditAttribution: damiankloip commented 20 March 2013 at 11:29

Status:

Needs work

» Needs review

File	Size
interdiff.txt	5.35 KB
1835102-13.patch	12.35 KB

Comment #14

dawehner

German

CreditAttribution: dawehner commented 20 March 2013 at 20:34

File	Size
interdiff.txt	1.12 KB
drupal-1835102-14.patch	12.64 KB

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -1033,15 +1033,13 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
-  var height = 0;

wait .... this variable is used below.

Comment #15

droplet CreditAttribution: droplet commented 2 April 2013 at 01:54

Status:

Needs review

» Reviewed & tested by the community

Looks good. It's more like reformat the code :)

Comment #16

catch

he/him

English

CreditAttribution: catch commented 8 April 2013 at 14:26

Status:

Reviewed & tested by the community

» Needs review

Not completely sure about this bit:

-Drupal.behaviors.viewsUiEditView.attach = function (context, settings) {
+Drupal.behaviors.viewsUiEditView.attach = function () {
 
   "use strict";
 
@@ -26,7 +26,7 @@ Drupal.behaviors.viewsUiAddView = {};
  * In the add view wizard, use the view name to prepopulate form fields such as
  * page title and menu link.
  */
-Drupal.behaviors.viewsUiAddView.attach = function (context, settings) {
+Drupal.behaviors.viewsUiAddView.attach = function (context) {

While those arguments aren't used, those are the arguments that actually get passed to the function, so do we really want to remove those?

Comment #17

droplet CreditAttribution: droplet commented 8 April 2013 at 16:03

Passing unused parameters ?

Comment #18

nod_

French

Lille

CreditAttribution: nod_ commented 8 April 2013 at 22:36

File	Size
core-js-jshint-views_admin-1835102-17.patch	8.52 KB

Let the unused parameters, I took out the option from our JSHint configuration, too much trouble for what it's worth. Can always use as documentation in a pinch too :)

Here is the updated patch with some spaces changes in the way of the new coding standards which are being written over there #1778828-7: [policy, no patch] Update JS coding standards help welcome.