Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
3.69 KB
YesCT’s picture

Status: Needs review » Needs work
FileSize
625 bytes
3.68 KB

manually tested. And the js error is gone, but it jumps me out of the views overlay and wizard and to the front page.

tried to improve grammar of the comment. See if it still says the correct meaning of what you wanted it to say.

YesCT’s picture

Status: Needs work » Needs review
YesCT’s picture

Status: Needs review » Needs work
dawehner’s picture

manually tested. And the js error is gone, but it jumps me out of the views overlay and wizard and to the front page.

So we probably want to set this link to use an empty fragment, so it doesn't do anything for itself, without the javascript?

YesCT’s picture

@dawehner How do we do that?

Also, this is "needs tests"
what kind of test should we add? webtest?

dawehner’s picture

Well, the removing just works on the javascript so I don't see how we should be able to test this?
The error mentioned by tim happens if you click on the link but the javascript fails.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.89 KB
3.77 KB

Gave this a quick tidy up, now it matches alot of the other js code (I think :)). Also tested, and confirm the fix is good.

Removing needs tests too; Javascript is rubbish, and hard to test ;)

dawehner’s picture

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+  /**
+   * Handle handler deletion by looking for the hidden checkbox and hiding the
+   * row.
+   */
...
+  /**
+   * Handle display deletion by looking for the hidden checkbox and hiding the
+   * row.
+   */

These aren't method documentations so should we use "//" instead?

olli’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+Drupal.behaviors.handlerRemoveLink = {};

.handlerRemoveLink -> .viewsUiHandlerRemoveLink

+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+    $('#views-row-' + id).hide();
+    $('#views-removed-' + id).attr('checked', true);

These could use the context param too.

+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+  $('a.display-remove-link', context).addClass('display-processed').click(function(event) {

Why not .once() instead of .addClass()?

+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+      var id = $(this).attr('id').replace('display-remove-link-', '');
+      $('#display-row-' + id, context).hide();
+      $('#display-removed-' + id, context).attr('checked', true);
+      event.preventDefault();
+  });

Indentation.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.23 KB
2.68 KB

Rerolled and added in feedback from #9 and #10, thanks. Its been a while!

YesCT’s picture

FileSize
960 bytes
2.67 KB
+++ b/core/modules/views_ui/js/views-admin.jsundefined
@@ -1098,6 +1098,32 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+Drupal.behaviors.viewsUIhandlerRemoveLink = {};
+Drupal.behaviors.viewsUIhandlerRemoveLink.attach = function(context) {

In this js file, UIwhatever is more using the pattern: UiWhatever.

+++ b/core/modules/views_ui/js/views-admin.jsundefined
@@ -1098,6 +1098,32 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+  /**
+   * Handle handler deletion by looking for the hidden checkbox and hiding the
+   * row.
+   */
...
+  // Handle display deletion by looking for the hidden checkbox and hiding the
+  // row.

missed one. :)

YesCT’s picture

verified that the bug still exists without the patch.

manually tested with the patch:
Works great, stays in views ui, and I can save. Everything fine.

Next steps:
a code review (I should not rtbc as I did the most recent patch).

======

Actually, with and without this patch, rearranging the sort criteria or the no results behavior... results in all of those being deleted instead of rearranged. Rearranging the filter criteria works fine though. I dont see a issue for rearranging sort criteria bug (yet).

Should not stop this issue as it is a pre-existing bug.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I talked to @damiankloip in irc.
rtbc!

tim.plunkett’s picture

RTBC +1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f140bce and pushed to 8.x. Thanks!

I guess we need a new issue for the bug mentioned by @YesCT in #13

damiankloip’s picture

I think #1929070: Refactor views_ui_rearrange_filter_form to remove theme function for table rendering is related to that, and will possibly/probably/maybe fix what is mentioned by YesCT.

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