Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Needs to be split up. I don't want another 1k LOC file of JS in core.
- File needs a top-level closure with the proper dependencies,
- a couple of for loops are abused.
- Selectors needs optimisation
- use .on/.off everywhere
- needs tests scenarios in http://drupal.org/node/1777342
Comment | File | Size | Author |
---|---|---|---|
#16 | core-js-1839130-16.patch | 62.97 KB | droplet |
#16 | interdiff.patch | 1.98 KB | droplet |
#14 | core-js-1839130-14.patch | 63.89 KB | nod_ |
#13 | core-js-1839130-13.patch | 64.93 KB | nod_ |
#11 | core-js-1839130-11.patch | 63.37 KB | nod_ |
Comments
Comment #1
rballou CreditAttribution: rballou commentedAs far as splitting up the files, it looks like we could potentially split:
Drupal.viewsUi
Drupal.behaviors
There are also a few classes like
Drupal.viewsUi.FormFieldFiller
,Drupal.viewsUi.addItemForm
,Drupal.viewsUi.OptionsSearch
, and others that could be split up/organized in a different way. I'm still going through the file, but I have a branch on Github that starts on some of the cleanup. I can create patches from that if helpful.https://github.com/robballou/drupal/tree/8.x-views-admin-refactor
Commits so far there include the top-level closure, using a closure-specific $ variable, and a few other minor cleanup changes.
Comment #2
dawehner@rballou
The best way to make progress would be small, reviewable patches.
Comment #3
rballou CreditAttribution: rballou commented@dawehner
Thanks for reminding me ... things had gotten split among different machines and forgot to get it all reconciled until now. :)
First patch:
$
bind()
calls to useon()
Second patch (this is an additional patch, apply both patches):
Drupal.behaviors
object and then define the attach functionbind()
calls toon()
callsThere's still more to refactor here but hopefully this is a start.
Comment #4
nod_oooh thanks a lot for getting that going. I'll review tonight if someone doesn't beat me to it :)
Comment #5
nod_there are some
$('li.add > a', $menu)
left that would need to look like$menu.find('li.add > a')
this type of things
should be
and like you said, there are some
.bind
left over. Great progress :) We can talk alter about splitting up the files once it's all nice and clean. It's already much more pleasant to read.Comment #6
rballou CreditAttribution: rballou commentedA few more changes to look at:
Comment #8
rballou CreditAttribution: rballou commentedRe-rolled a combined patch
Comment #10
rballou CreditAttribution: rballou commentedTagging?
Comment #11
nod_Reroll, not sure I have everything you did though.
Comment #12
nod_Comment #13
nod_replaced some more .bind(), added debouncing to the scoll and resize event handler.
Comment #14
nod_reroll
Comment #15
dawehnerI would like to review that patch, but the amount of changes are sadly way over my head.
Comment #16
droplet CreditAttribution: droplet commentedSuggest to review 2 patches side-by-side in the editors :)
I think jQuery is easier to understand when this is in the comments.
These lines we have to pay more attention. (other lines are basic changes only.)
views-ui-render-add-view-button-processed
I think we need a change on it, right ?
I will do another manual testing soon.
Comment #18
nod_Comment #19
dawehnerDid some manual testing:
Comment #20
webchickAwesome, thanks for the manual testing. Since nod_ was the one who wrote a lot of this, it's unlikely anyone else is going to be able to review it. :P~
Ok, let's do this. Alex Pott tells me this is the last file needed for us to be able to run jslint on all core JS before it's committed!
Committed and pushed to 8.x. Thanks!