Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: -Usability, -sprint, -Spark
FileSize
2.6 KB

Didn't mean to copy all those tags.

tim.plunkett’s picture

Issue tags: +Needs manual testing

AFAIK this can't be tested with simpletest.

Bojhan’s picture

Whoo :) This is soooo critical and an issue we identified + solution we actually identified back in 2009 at the Baltimore usability test. Very happy to see this patch :)

I do think that choosing a region should become required. Although the UX is a little wonky, the whole disabled region concept is even wonkier.

tim.plunkett’s picture

Status: Needs review » Needs work

Ahhhh crap. This won't work with the overlay at all.

Anyone have a good suggestion about how to get the information from one page to the next? Local storage?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.61 KB
3.28 KB

Here's a different approach.
There is already a bug with closing a modal while in the overlay, I'll open a separate issue for that.

tim.plunkett’s picture

The nice thing about this approach is that the color highlighting will still work without JS on, you just won't get the scroll animation.

tim.plunkett’s picture

FileSize
2.09 KB
4.42 KB

Rerolled, removed the Drupal:: call

Status: Needs review » Needs work

The last submitted patch, block-2078951-7.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
5.72 KB
2.15 KB

For me, #2076411: Remove the request scope from the current user service breaks the toolbar's JS. If this patch doesn't work during manual testing, you probably have Uncaught SyntaxError: Unexpected token < in your JS console. Disable the toolbar, comment on that issue about how broken it is, and continue testing :)

I forgot there is a route controller sitting in front of the list controller, I have to pass the request all the way through.

jessebeach’s picture

I manually tested the patch on multiple browsers. It works.

I have one small change to the JS that introduces once(). Otherwise that screen will scroll to the just-placed block any time an AJAX request is sent off or something else invokes Drupal.attachBehaviors.

/**
 * Highlights the block that was just placed into the block listing.
 */
Drupal.behaviors.blockHighlightPlacement = {
  attach: function (context, settings) {
    if (settings.blockPlacement) {
      $('#blocks').once('block-highlight', function () {
        var $container = $(this);
        // Just scrolling the document.body will not work in Firefox. The html
        // element is needed as well.
        $('html, body').animate({
          scrollTop: $('#block-placed').offset().top - $container.offset().top + $container.scrollTop()
        }, 500);
      });
    }
  }
};

Tim, just have a look at the JS I changed. If you're fine with it, this is ready to be RTBC pending the bot.

tim.plunkett’s picture

Issue tags: -Needs manual testing

Thanks, I always forget about .once() until someone reports a weird bug, then I remember it!

+1, leaving for someone to RTBC.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I tried to break this during manual testing, but could not :)
I tried to find nitpicks, but could not :)

Much nicer UX indeed.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

nod_’s picture

Issue tags: +JavaScript

tag.

Also, animate() #sadpanda. and using a function as an argument to .once() and doing a var $thing = $(this) when you're already creating the jquery object the line before #sadunicorn.

tim.plunkett’s picture

So like this?

diff --git a/core/modules/block/js/block.admin.js b/core/modules/block/js/block.admin.js
index 5c657d4..60cbdc8 100644
--- a/core/modules/block/js/block.admin.js
+++ b/core/modules/block/js/block.admin.js
@@ -74,8 +74,8 @@ function showBlockEntry (index, block) {
 Drupal.behaviors.blockHighlightPlacement = {
   attach: function (context, settings) {
     if (settings.blockPlacement) {
-      $('#blocks').once('block-highlight', function () {
-        var $container = $(this);
+      var $container = $('#blocks');
+      $container.once('block-highlight', function () {
         // Just scrolling the document.body will not work in Firefox. The html
         // element is needed as well.
         $('html, body').animate({

I'm not sure what the replacement for .animate() would be, and I honestly don't know what you mean when you say you're sad about "using a function as an argument to .once() ".

nod_’s picture

Almost:

diff --git a/core/modules/block/js/block.admin.js b/core/modules/block/js/block.admin.js
index 5c657d4..35bb5e8 100644
--- a/core/modules/block/js/block.admin.js
+++ b/core/modules/block/js/block.admin.js
@@ -74,14 +74,14 @@ Drupal.behaviors.blockFilterByText = {
 Drupal.behaviors.blockHighlightPlacement = {
   attach: function (context, settings) {
     if (settings.blockPlacement) {
-      $('#blocks').once('block-highlight', function () {
-        var $container = $(this);
+      var $blocks = $('#blocks').once('block-highlight');
+      if ($blocks.length) {
         // Just scrolling the document.body will not work in Firefox. The html
         // element is needed as well.
         $('html, body').animate({
-          scrollTop: $('#block-placed').offset().top - $container.offset().top + $container.scrollT
+          scrollTop: $('#block-placed').offset().top - $blocks.offset().top + $blocks.scrollTop()
         }, 500);
-      });
+      }
     }
   }
 };

When we'll replace jquery with regular JS it'll be easier to change things when this form is used instead of the function thing. Which also creates a new scope that really isn't needed usually.

For the animate thing I don't know haven't saw the D8 UI in weeks so maybe it's needed, but I took out the animation for collapsible fieldsets a while back, I hoping the same can happen here (or at least it was supposed to be removed in #2060573: Standardized filter/search behavior; surface filter matches regardless of their containing box's open state; but anyway).

nod_’s picture

Status: Fixed » Needs review
Issue tags: -JavaScript
FileSize
1.54 KB

Reopening.

That kind of stuff is handled by 20+ years old technology (we broke) #block-placed. This patch + #1870006: HTML5 validation with table sticky header is misaligned over the toolbar should be enough.

Status: Needs review » Needs work

The last submitted patch, core-block-ui-highlight-2078951-17.patch, failed testing.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

I'm manually testing this in conjunction with #1870006: HTML5 validation with table sticky header is misaligned over the toolbar, and I see different behavior with and without a modal.

Starting on http://d8/#overlay=admin/structure/block, using the modal link to place a block, I end up on http://d8/admin/structure/block/list/bartik?block-placement=bartikadministration-2#block-placed, because submitting a modal form apparently kills the overlay.

Starting on http://d8/#overlay=admin/structure/block/add/system_menu_block%3Afooter/bartik (bypassing the modal, but still in the overlay, I end up on http://d8/#overlay=admin/structure/block/list/bartik%3Fblock-placement%3Dbartikfooter-2%23block-placed, which has the second # encoded, and obviously does not work for either the scroll or for the addition of the class and ID anchor itself.

nod_’s picture

Status: Needs work » Fixed

I'll open follow-up then.

YesCT’s picture

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.