Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

selector in admin js are real bad.

wierd thins in menu.js

oxyc’s picture

FileSize
1.94 KB

Just as i patched this I noticed #1660952: Replace all jQuery.each() with filtered for loop, but as that issue is regarding $.each I decided to roll one with everything except for the loop change.

oxyc’s picture

Status: Active » Needs review
nod_’s picture

Status: Needs review » Needs work

nice :)

so first .on you could bind it to the #edit-menu and use event delegation selecting input.
use camelcase variables, no underscore
you don't have to scope the id selector

good work, thanks!

oxyc’s picture

FileSize
3.19 KB

Thanks for the review, here's a modified version.

oxyc’s picture

Status: Needs work » Needs review

I keep forgetting to tag as NR

Status: Needs review » Needs work

The last submitted patch, core-1751398-5.patch, failed testing.

oxyc’s picture

Status: Needs work » Needs review
FileSize
3.19 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, core-1751398-8.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

should probably pull the new changes, patch doesn't apply either.

nod_’s picture

Status: Needs review » Needs work
oxyc’s picture

Status: Needs work » Needs review
FileSize
3.43 KB

Whops,

Hope it goes better this time.

nod_’s picture

There is another patch to replace the jQuery each with a filtered for loop, can you make the change in here already ?

(edit) apart from that it's good to go :)

oxyc’s picture

FileSize
3.52 KB

Sure, are you fine with renaming index machineName? I had to log it myself to understand what it was therefore I think machineName is more readable, if not I'll reroll.

seutje’s picture

Status: Needs review » Reviewed & tested by the community

Naming changes like that, which only help readability, are more than welcome.

Looks good to go, this clears up a lot of weird stuff that was going on there.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks like great clean-up. Committed/pushed to 8.x.

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