Follow-up from #2055853: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout.

When adding a block from the modal, the newly added block is often hard to find visually, especially if you forget to fill out a region for it.
This colors the row of a just-placed block, and also scrolls it into view.

A follow-up: #2088027: just placed block keeps changed color even after saving the block layout page

Files: 
CommentFileSizeAuthor
#17 core-block-ui-highlight-2078951-17.patch1.54 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 58,741 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#10 block-highlight-row-2078951-10.patch5.91 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 58,787 pass(es).
[ View ]
#10 interdiff.txt975 bytesjessebeach
#9 interdiff.txt2.15 KBtim.plunkett
#9 block-2078951-9.patch5.72 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,324 pass(es).
[ View ]
#7 block-2078951-7.patch4.42 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,186 pass(es), 85 fail(s), and 87 exception(s).
[ View ]
#7 interdiff.txt2.09 KBtim.plunkett
#5 block-2078951-5.patch3.28 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,349 pass(es).
[ View ]
#5 interdiff.txt3.61 KBtim.plunkett
#1 block-2078951-1.patch2.6 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,573 pass(es).
[ View ]

Comments

Status:Active» Needs review
Issue tags:-Usability, -sprint, -Spark
StatusFileSize
new2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,573 pass(es).
[ View ]

Didn't mean to copy all those tags.

Issue tags:+Needs manual testing

AFAIK this can't be tested with simpletest.

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.

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?

Status:Needs work» Needs review
StatusFileSize
new3.61 KB
new3.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,349 pass(es).
[ View ]

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.

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.

StatusFileSize
new2.09 KB
new4.42 KB
FAILED: [[SimpleTest]]: [MySQL] 58,186 pass(es), 85 fail(s), and 87 exception(s).
[ View ]

Rerolled, removed the Drupal:: call

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new5.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,324 pass(es).
[ View ]
new2.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.

StatusFileSize
new975 bytes
new5.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,787 pass(es).
[ View ]

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.

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.

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.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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.

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() ".

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).

Status:Fixed» Needs review
Issue tags:-JavaScript
StatusFileSize
new1.54 KB
FAILED: [[SimpleTest]]: [MySQL] 58,741 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Reopening.

That kind of stuff is handled by 20+ years old technology (we broke) #block-placed. This patch + #1870006: Fragment navigation with toolbar + tableheader should be enough.

Status:Needs review» Needs work

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

Assigned:tim.plunkett» Unassigned

I'm manually testing this in conjunction with #1870006: Fragment navigation with toolbar + tableheader, 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.

Status:Needs work» Fixed

I'll open follow-up then.

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

Issue summary:View changes

Updated issue summary.