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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rballou’s picture

As far as splitting up the files, it looks like we could potentially split:

  • Any of the Drupal.viewsUi
  • Any of the 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.

dawehner’s picture

@rballou
The best way to make progress would be small, reviewable patches.

rballou’s picture

nod_’s picture

oooh thanks a lot for getting that going. I'll review tonight if someone doesn't beat me to it :)

nod_’s picture

Status: Active » Needs work

there are some $('li.add > a', $menu) left that would need to look like $menu.find('li.add > a')

this type of things

var windowWidth = $(window).width(),
    windowHeight = $(window).height(),
    maxWidth = parseInt(windowWidth * 0.85, 10), // 70% of window
    minWidth = parseInt(windowWidth * 0.6, 10); // 70% of window

should be

var windowWidth = $(window).width();
var windowHeight = $(window).height();
// 70% of window
var maxWidth = parseInt(windowWidth * 0.85, 10);
// 70% of window
var minWidth = parseInt(windowWidth * 0.6, 10); 

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.

rballou’s picture

Status: Needs work » Needs review
FileSize
16.65 KB

A few more changes to look at:

e7104b5 Finished refactoring .bind() calls
4c38776 Added new line at end of the file.
e3fba0f Changed some variable names to use the $var convention
612b77d Changed a few unbind() calls to off()
4a85ddf Created a variable for the jQuery context variable so that it is cached
54f7569 Changed up a couple more .bind() calls to .on()
785e1d4 Changed jQuery(selector, context) calls to context.find()

Status: Needs review » Needs work

The last submitted patch, 1839130-refactor-views-admin-js-extra2.patch, failed testing.

rballou’s picture

Status: Needs work » Needs review
FileSize
42.83 KB

Re-rolled a combined patch

Status: Needs review » Needs work

The last submitted patch, 1839130-refactor-views-admin-js-combined.patch, failed testing.

rballou’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing

Tagging?

nod_’s picture

FileSize
63.37 KB

Reroll, not sure I have everything you did though.

nod_’s picture

Title: Refactor modules/views/views_ui/js/views-admin.js » Refactor modules/views_ui/js/views-admin.js
nod_’s picture

FileSize
64.93 KB

replaced some more .bind(), added debouncing to the scoll and resize event handler.

nod_’s picture

FileSize
63.89 KB

reroll

dawehner’s picture

I would like to review that patch, but the amount of changes are sadly way over my head.

droplet’s picture

FileSize
1.98 KB
62.97 KB

Suggest to review 2 patches side-by-side in the editors :)

+++ b/core/modules/views_ui/js/views-admin.jsundefined
@@ -107,7 +101,7 @@ Drupal.viewsUi.FormFieldFiller = function ($target, exclude, replace, suffix) {
+  // NOTE: $.proxy will not work for this because it assumes we want only

@@ -340,171 +297,152 @@ Drupal.behaviors.viewsUiSearchOptions.attach = function (context) {
+   *   A $ object representing the rows of filterable options to be

I think jQuery is easier to understand when this is in the comments.

+++ b/core/modules/views_ui/js/views-admin.jsundefined
@@ -253,55 +221,51 @@ Drupal.viewsUi.addItemForm.prototype.refreshCheckedItems = function() {
+    var $menu = $(context).find('#views-display-menu-tabs').once('views-ui-render-add-view-button-processed');

@@ -340,171 +297,152 @@ Drupal.behaviors.viewsUiSearchOptions.attach = function (context) {
+$.extend(Drupal.viewsUi.OptionsSearch.prototype, {

@@ -647,261 +581,231 @@ Drupal.viewsUi.rearrangeFilterHandler.prototype.duplicateGroupsOperator = functi
+    $(context).find('#views-ui-config-item-form div.form-item-options-value-all').once('filterConfigSelectAll')

@@ -1098,34 +987,29 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
-  $('a.views-remove-link', context).once('views-processed').click(function(event) {
...
+    $context.find('a.views-remove-link').once('views').on('click', function (event) {

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.

Status: Needs review » Needs work

The last submitted patch, interdiff.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Did some manual testing:

  • Checked the js behavior on the views wizard
  • Check the views field search
  • checked ordering of fields, and removing
  • checked some preview
  • Checked some of the dropbuttons
  • Checked exposed filters.
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, 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!

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