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
Files: 
CommentFileSizeAuthor
#16 core-js-1839130-16.patch62.97 KBdroplet
PASSED: [[SimpleTest]]: [MySQL] 57,616 pass(es).
[ View ]
#16 interdiff.patch1.98 KBdroplet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 core-js-1839130-14.patch63.89 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 57,566 pass(es).
[ View ]
#13 core-js-1839130-13.patch64.93 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 57,399 pass(es).
[ View ]
#11 core-js-1839130-11.patch63.37 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 56,123 pass(es).
[ View ]
#8 1839130-refactor-views-admin-js-combined.patch42.83 KBrballou
FAILED: [[SimpleTest]]: [MySQL] 48,868 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#6 1839130-refactor-views-admin-js-extra2.patch16.65 KBrballou
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1839130-refactor-views-admin-js-extra2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 1839130-refactor-views-admin-js.patch21.26 KBrballou
FAILED: [[SimpleTest]]: [MySQL] 48,763 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#3 1839130-refactor-views-admin-js-extra1.patch22.21 KBrballou
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1839130-refactor-views-admin-js-extra1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

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.

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

StatusFileSize
new22.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1839130-refactor-views-admin-js-extra1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new21.26 KB
FAILED: [[SimpleTest]]: [MySQL] 48,763 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

@dawehner
Thanks for reminding me ... things had gotten split among different machines and forgot to get it all reconciled until now. :)

First patch:

Second patch (this is an additional patch, apply both patches):

There's still more to refactor here but hopefully this is a start.

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

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.

Status:Needs work» Needs review
StatusFileSize
new16.65 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1839130-refactor-views-admin-js-extra2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new42.83 KB
FAILED: [[SimpleTest]]: [MySQL] 48,868 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-rolled a combined patch

Status:Needs review» Needs work

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

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

Tagging?

StatusFileSize
new63.37 KB
PASSED: [[SimpleTest]]: [MySQL] 56,123 pass(es).
[ View ]

Reroll, not sure I have everything you did though.

Title:Refactor modules/views/views_ui/js/views-admin.jsRefactor modules/views_ui/js/views-admin.js

StatusFileSize
new64.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,399 pass(es).
[ View ]

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

StatusFileSize
new63.89 KB
PASSED: [[SimpleTest]]: [MySQL] 57,566 pass(es).
[ View ]

reroll

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

StatusFileSize
new1.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new62.97 KB
PASSED: [[SimpleTest]]: [MySQL] 57,616 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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.

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.