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: shortcut/shortcut.admin.js

CommentFileSizeAuthor
#10 shortcut-1684866-10.patch794 bytesoriol_e9g
#4 core-js-jshint-shortcut.patch839 bytesnod_
#2 shortcut-js.patch653 bytesdroplet

Comments

nod_’s picture

nod_
Primary language French
Location Lille
commented 
core/modules/shortcut/shortcut.admin.js: line 55, col 75, 'self' is not defined.
core/modules/shortcut/shortcut.admin.js: line 55, col 95, 'self' is not defined.
droplet’s picture

droplet commented
Status: Active » Needs review
StatusFileSize
new shortcut-js.patch653 bytes
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

I'd prefer we don't add a new variable since replacing self with this would work just as well.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-js-jshint-shortcut.patch839 bytes
nod_’s picture

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

works for me

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

David_Rothstein’s picture

David_Rothstein commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

I guess in general y'all aren't backporting these JSHint patches to Drupal 7 (which makes sense)... but this one seems like a straight-up bugfix, no?

droplet’s picture

droplet commented

I'd rather to hardcode it than using global (default) setting.

var changedRowObject = new tableDrag.row(changedRow, 'mouse', false, 0, true);
nod_’s picture

nod_
Primary language French
Location Lille
commented

That's not a bug actually just sloppy code.

self will resolve to window.self and both will be undefined. Meaning they will coerce to false in all the conditions tabledrag has on them. Basically it "just works"™ and we should hardcode it like droplet proposed, since this will not have any hierarchy ever.

(In D8 all this code is gone now thanks to the remove shortcut limit patch)

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new shortcut-1684866-10.patch794 bytes
droplet’s picture

droplet commented
Status: Needs review » Reviewed & tested by the community
droplet’s picture

droplet commented
Category: task » bug

it's a bug fix and not the backports. D8 removed that part of code from other patches.

webchick’s picture

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

Since it seems David already approves of this for 7.x, committed and pushed to 7.x. Thanks!

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