Implement the new block layout design to emphasize the primary interaction of placing a block

To be clear, we are talking about this thing, right?

If nothing else the wording is quite bad - a block instance is being created, not placed? The blocks in the center already exist and can be placed?

There is also a tab for the custom block library at the top. More WTF.

The first half of #2078601: Move the block placement browser to the left of table and collapse it would make it more obvious, but collapsing it probably won't help.

Adding a bit of context. The core of the issue is that all of our users in the UMN 2015 test had significant difficulties in placing just a block. Since this is a key site building concept, that users had no difficulty with before - we are making this critical. UX issues such as this should be marked critical, as it severely hinders new and existing adoption of Drupal 8.

We identified several possible solutions. I will pick up and do some design exploration, I think there are multiple avenues to this. We need to ensure that, the time we put in is towards the most valuable one. I think either 2) or 3) would be most fruitful. While 1) is an option, I think the fact that it is considered supplementary will make people just go to the right side.

My quick thought, why don't we just add it also an explicit step to add blocks. This allows to explain users what is going on, which is IMHO of a higher value than making things easy.

1) sounds just like a workaround to be honest. 2) Seems to be the solution which is more common with the entirety of the Drupal UI. If you add something you have an add button

-

If it were me, I'd just add a third tab, placing it before the custom block tab, and call it something like "dynamic block library" or maybe "persistent block library". Verbage that denotes it as being a bit more static as opposed to being custom. Use the two library pages to enable/ disable which blocks are placed.

On a side note, "layout" should be renamed IMO. Layout is usually what happens after the item is placed. Rename it to "block placement"? Ideally you are placing the block on the page. It would fit the current verbage of placing the block. What happens when you throw panels or display suite into the mix which completely overrides all predefined themes? You now have block layout through core and layout through panels. To me, layout makes more sense when using a system like panels or DS. From a core standpoint, core doesn't (currently) handle the layout of anything. It simply regurgitates what it's told.

Installed on simplytestme and didn't even see the placement options until I scrolled to the very bottom of the region list. Lots of scrolling on mobile.

Just want to say that I'm really glad this is being address. When I first tried Drupal 8, this was the biggest WTF for me. I took quite a while to figure out how the block administration worked. In the list of proposed solutions, #2 sounds the best. It falls in line with how fields are added. You click an "add" button and the choose new or existing.

Yeah special case UIs are local optimizations which doesn't help you to find necessarily the global optimum.

Why don't we call that thing the "Block Library"?

I agree with third tab, I myself just recently searched how to place a block myself ...

Block Placement | Block Library | Custom Blocks

Preview of moving the sidebar to the left:

My suggestions:

1. Move the sidebar to the left. It drastically improves visibility.
2. Rename the sidebar header to "Add block". This makes it clear, where to go to add more blocks. This also necessitates a rename of the blue button to "Create custom block" which further emphasizes that it will not add an existing block to the site layout.
3. Yes, rename that custom block library tab. No where else do we use "library" for a simple overview page. This should not be an exception.
4. This may be a little controversial, but also recolor the sidebar. As a user of Drupal 6 and 7, my initial reaction to the new block page was to first scroll to the bottom and drag a block from the disabled section up, and second to try and click the blue button. It didn't initially dawn on me to check the right sidebar because it looked grey, which meant "useless disabled form elements" in my mind. The image makes that area white, which clearly means active (being bright also commands your eye's attention to look here).

Thanks Matt. Moving the sidebar over was also our first thought - as you throw it right in there face (hopefully). One of the things that does though - is creates a unique pattern to the blocks page (as we don't do this elsewhere). One of the things we noticed is that users are very attracted to the table of drag-and-drop.We fear that simply moving it, doesn't really solve the core of the problem.

We discussed this over the spriting at DrupalCamp Minneapolis (Webchick, Lewis, Ivan et al.). The solution to this problem is quite tricky, and there are very different ways to solving this.

From an interaction design perspective we wish to reduce the confusion by:

• Avoiding two competing primary interactions on the block layout page (placing blocks and creating custom blocks)
• Reducing the emphasis on custom blocks
• Providing a highly useable place block interaction

Users were unable to achieve this task because they did not see the sidebar. Users ventured into custom blocks without realising the fact that there are existing blocks that can be placed.

To achieve this we think the best way to tackle this is to take the following actions:

1. Remove the sidebar from the blocks layout page
2. Make "place blocks" use our standardised UI pattern of having the primary action at the top of the page
3. Allow all blocks (custom, module provided, etc.) to be placed via this "place blocks" page that follows the action
4. Separate the creation of custom blocks and custom block types to a separate page.

While this seems rather radical, this critical problem truly prevented people from learning a core concept of Drupal. While we can try various visual tricks - we ought to really streamline this experience with the rest of core.

The challenges we see are mostly in using the correct labeling and descriptions to describe the concepts. Key here is that users are not tempted into custom blocks, simply as a "more" pattern - but rather knowning its purpose. We have learned that venturing into the custom blocks interface, can be misleading them into thinking there are no other blocks.

Mockup 1: Only provide one "place" interaction on the block layout

Mockup 2: Allow users to select all kind of blocks (module provided, custom)

Mockup 3: Provide users with the block configuration page

Mockup 4: Provide a separate page, from where you can add the custom blocks (same as now, different labels)

Mockup 5: Provide a separate page from, where you can add block types (same as now, different labels)

I would love to know what you think, this is quite a tricky problem - in a space that we had much bigger plans for. However we now need to move forward to release, and this critical building block should be highly usable.

Since the overall consensus (2hours ago) seems to be in favour of option 2, I've started work on an implementation of said option.
Attached you'll find an initial path which removes the block placement bar and moves it to a separate page. I'll continue working on this to match @Bojhan's mockups.

Hmm... Well let's start with the mockups:

Mockup 1:
Has a major issue of no obvious way to let users create a new block. First thought was definitely not to look in the "block types" tab. As you mentioned, labeling is so important! It also has that "classic" layout. Previous Drupal users are going to expect disabled blocks at the bottom - are we keeping that section?

Mockup 2:
Well it works. If that button creates a modal dialog, it's fine, but if not it should really be a select list.

Mockup 3:
Not different from what we already have, but... "UGH. I have to visit that form for every block I want to move? No #@$%ing way." ...was my initial thought if it became required to moving blocks around.

Mockup 4/5:
Don't mind. Same as #1 for that tab label though.

The more that I think about it, the more I think that we might also be missing the obvious solution. What if we placed disabled blocks into "default" sections of the table right next to active blocks that would be greyed out and have an "enable" link? If that link was ajax-ified and worked like the edit field gear icon, and then allowed in-place configuration... could be promising. It might require changes to the block plugin system though.

Also, personally, I group blocks and themes together in my head for whatever reason. Like the theme page, I kind of expect a slightly unique UI/appearance on the block page. What if block layout was a child of the appearance menu? Would people be more willing to deal with a custom UI and let us get away with different concepts?

I'm using the mockups as a general guideline since I already anticipated disagreements on certain elements (such as Mockup1's tab label). Attached you'll find a new patch + interdiff + screenshots of the current add block workflow.

New block layout page.
I've chosen to rename the Block library tab to be Custom blocks because that's what the tab actually is, a way into custom block management. However, I feel this tab might actually be misplaced here.

New block placement page.
This is currently just the original sidebar, but moved to a seperate page. Will refactor this to more closely match the other admin screens.

Placement of a block. AJAX goodness
Nothing has changed here, Clicking one of the blocks will open a modal.

Submitting the modal adds the block and returns the user to the block layout page.

Here's a preview of my thoughts in #20:

This way blocks are nice and visible right there in the main table, instead of buried at the bottom, if that's what we're going for. They can also be immediately enabled and set in the user's desired region, with ajax changing the link to a "configure" button. There's just that pesky issue of implementation...

Also, yes to getting rid of the regions column. It is redundant information and at odds with the concept of a draggable table.

@MattA,

how does your proposition handle adding multiple instances of the same block? I'm afraid the list of uninstantiated blocks will clutter the block layout overview.

Personally I think separating the block placement from the block layout tremendously cleans up the admin UI at the cost of 1 extra click. Therefor i think option 2 is the best way to solve this issue.

Removed accidentally embedded images.

It doesn't, so scratch that idea. Ironically enough, I had removed the sidebar while working on the concept and forgot about that feature. /sigh

On a side note: how about "add block to layout" instead of "place block" to keep with standards?

@legolasbo
It is great to see progress here.

In general I think being able to search might still makes sense on that additional page. Do you think it should be part of a modal as well? The views UI has a similar pattern when you add a new field for example.

There is already a separate issue: #2513534: Remove the 'disabled' region from Block UI

I think we should be able to keep these two patches separate, which would reduce the complexity of this issue.

@dawehner Yhea, I'd love if we can retain that. I didn't want to assume that we can easily paste it over though - if we can that'd be great.

Another update. The block placement page now actually respects the chosen theme in stead of just placing blocks on the default theme.

@dawehner I was also thinking that the search feature should be retained. As for placing the actual block placement form in a modal... I didn't quite think of that option, but that would be a great way to keep users informed of the context of the form. However, for now I'll focus on actually implementing the proposed resolution. We can always add the modal in a follow-up issue.

@dawehner I was also thinking that the search feature should be retained. As for placing the actual block placement form in a modal... I didn't quite think of that option, but that would be a great way to keep users informed of the context of the form. However, for now I'll focus on actually implementing the proposed resolution. We can always add the modal in a follow-up issue.

Totally agree. I was just thinking out load.

@legolasbo++

One thing that has caught me out a few times when I have looked at this screen is that the items in the "Place blocks" list are prefixed with "+". In Drupal, that usually means you can drag and drop them - e.g. rearrange the order of items in a list. So, several times I have come to this screen and tried to drag a block from the "Place blocks" list and drop it in one of the screen regions on the left. Of course it doesn't work (wherever you drag it, it always has a "no entry sign" cursor) and hence evokes a momentary WTF? reaction.

When I think about it, I realise that the "+" in this context denotes an expanding/collapsing hierarchical list. But drag and drop does seem the natural way to go about it here.

Why not a drag and drop interface?

+1 regarding what StuartJNCC sad. The first time I used Drupal 8 that is exactly what I tried to do with the "+".

@StuartJNCC Yup, I've done that... as recently as today...

As mentioned in #17 from a usability perspective, keeping things consistent leads to people knowing what to do, generally speaking. So creating a sidebar that people may or may not see, and then adding a drag-and-drop behavior that isn't implied, mentioned, or used elsewhere in core isn't ideal.

@legolasbo Nice work so far. Just some nitpicking:
Should probably make the path: admin/structure/block/placement.
The "Add custom block" action on that page probably isn't necessary/optimal.
It might be better to also use the theme local task on that page too.

@MattA,

I agree with the nitpicks and will incorporate them.

For now I've gotten a little stuck, which is probably caused by working on this for too long. I'll continue work on this on thursday. Unassigning the issue in case someone else wants to pick up work on this in the mean time.

There is a lot of space beside each region title (Header, Primary Menu, Secondary Menu, Help, Highlighted). What about a "add block" link directly beside these titles? From there open up a modal to select the specific block or add another. My gut (needs to be validated) is that users think layout first when wanting to add a new block to a page.

#34: Yes, count me into the users that tried to drag and drop the blocks ...

Took a fresh stab at this just to get more familiar with Drupal...

@todo
- Tests. Duh.
- Add/remove/edit help text.
- Thoroughly review the javascript, I haven't written code in that language in years...
- I might need to reinstall, but the Custom blocks page title disappears after the first time viewing it?!?!
- There's quite a bit of unused CSS laying around now.
- The grey modal background doesn't play nice with sticky headers.

@MattA,

I wanted to take a look at your patch to see which direction you are taking, but unfortunately it failed to apply.

Hmm... ok, tried it myself, and after some quick Google-ing it seems PowerShell messes up the encoding. Seems it output in UTF-16?! That would definitely explain the large file sizes of my last couple patches...

Let's try this again, not in Windows this time. :D

Nope, missed the new files. /sigh

I'm not sure why the automatic testing is falling. However, I notice that patch from #44 have a lot trailing whitespace. So here is an updated version of #44 without those trailing whitespace.

Posted this design over on #2513528: Add a link to add a block in empty regions on the Block layout page. the block list expanding is a related but additional to that issue.

After looking at this for a while I think maybe the block list needs to be labeled with a verb to underscore the action required. But instead of "place blocks" I'd say "Choose a block"

@tkoleary That's pretty cool! I feel safer if we went with a modal though, if we aren't aiming for a drag-and-drop interface in this release. A modal is impossible to miss :)

@LewisNyman Yes, modal is definitely the way to go if we do this. My current dev site has it, and it turns the whole process into a nice ajax wizard like effect. :)

There are quite a few options when it comes to adding header links though.

Personally, I don't feel like any of them look great, but for now it is probably all we can do. I am more a fan of Plan B (preferably with drag & drop support and weight handles).

Here's a patch with the ajax goodness (went with option #3 just for simplicity).

Even though I like the idea of being able to add blocks to a region directly, I think this falls outside of the scope of this issue.

Let's focus on moving the sidebar to a separate page and address the direct block placement feature in #2513528: Add a link to add a block in empty regions on the Block layout page. after this issue lands.

#56: "Users were unable to figure out how to place a block" - I think the patches and screenshots give users easy ways to place a block?

I'll work on the patch in #49 for a few hours to figure out why the tests are failing.

Yeah the more I use it, the more I feel that it just doesn't look right (the workflow is so much better though!). Oh... and the table sorting doesn't work in a modal dialog. I don't even know where to begin debugging that issue though, so just going with #44 and saving that for a follow-up sounds like a good plan for now.

Please do try and keep the patches as simple as possible but keeping solutions to their separate issues. It get's harder to review a patch if it introduces loads of changes.

We already have these two issues: #2513528: Add a link to add a block in empty regions on the Block layout page. #2513520: Add a contextual link to add a block to a region

I agree with @LewisNyman,

The changes in #44 already broke 67 assertions which need mending. Adding additional functionality would add further complexity to this already growing patch.

I'll upload a new patch which fixes several broken tests in a few minutes. There are still a few to go, but I'm running out of time for today.

Attached patch fixes several tests which were broken in #44. I'll continue work on this tomorrow.

Details for each test are listed below:
Drupal\block\Tests\BlockLanguageCacheTest was failing because we moved the block listing. After changing the paths it passes again.

Drupal\block\Tests\BlockUiTest was failing because we moved the block listing and changed the listing to a table. After changing the paths and xpath queries it passes again.

Drupal\block\Tests\BlockXssTest was failing because we moved the block listing and changed the listing. Adjusted the test accordingly.

Drupal\block_content\Tests\BlockContentListTest was failing because of the changed content listing title. I’ve changed the test accordingly.

Drupal\block_content\Tests\BlockContentTypeTest was failing because it was testing if the user would be redirected to the block configuration form after a custom block was added from the block placement form. Removing the Add custom block button is actually a major regression because users would now have to follow the following path to add a new custom block to the layout:

1. Click the custom blocks tab
2. Add a custom block
3. Return to block layout
4. Click the Add block to layout action button
5. Find their block
6. Click the Add block operation button
7. Configure the block

I’ve undone the removal of the Add custom block button and relabelled it to Add new custom block to layout. After which I've changed the paths in the tests to reflect the new block placement url.

Hmm, I still have strong reservations about bringing back the custom block action button. Specifically the thoughts mentioned in #17:

From an interaction design perspective we wish to reduce the confusion by:

• Avoiding two competing primary interactions on the block layout page (placing blocks and creating custom blocks)
• Reducing the emphasis on custom blocks
• Providing a highly useable place block interaction

Here is the train of thought that I think new users will go through for each button:

• For "Add block to layout" they won't know what existing blocks are available, so the decision to click won't be weighed very highly.
• For "Add new custom block to layout" they won't know what a "custom block" is. To them it could include existing blocks. Combined with the low weight given to the primary button, it ends up being about 50/50.

One solution is to explain what a custom block is, and provide a link to create one in the help text instead. That way we can still emphasize our desired primary interaction. This also goes well with efforts in #2514150: Advertise the block region demonstration in a more prominent way. So we effectively switch the demo and custom block positions and end up with:

@MattA,

How about something like this?

That would still allow the user to directly add a custom block, but still emphasises the desired primary action.

It's a nice visual aid, but no, I don't think it is enough.

Some thoughts:
a) Visual aids don't help the color blind, or in tree testing.
b) From a language perspective, the two combined are still bad. The word "new" does nothing to clarify the action, as that is what both buttons do. The word "custom" is just too ambiguous as well. There's just no way to do what we want in one word. Maybe two if we used something like "user-defined" instead, but even that just makes it technical. Mind you, given what actually happens, the button's verb is also wrong. It should really be more like "create and add
block to layout" (also, the '+' symbol which implies "add" is set by the theme, and can't be removed).
c) No where else in core is there a grey action button. Not sure if we should start a trend here.
d) Even if we wanted to, I've looked into the code, and it doesn't look like we can easily change the button color since it is apparently hard-coded into template_preprocess_menu_local_action() as $link ['localized_options']['set_active_class'] = TRUE.

e) Wait, why are we even discussing this! We're getting side-tracked. This issue should just be about making the block placement action obvious and moving the "custom blocks" link to a help text paragraph certainly does that (while not causing a total regression of the old custom block workflow). If we need to make it more prominent later then we can create a new issue and let some other guy worry about it.

@MattA,

Good point in e). let's move into that direction. Fixing/closing this critical should be our first priority. Removing the ability to create & place a custom block would introduce a new regression. Moving the link to do so however does not introduce said regression while still focussing on improving the block placement/layout workflow.

The current patch is looking great! Good to see so much progress so quickly.

I'm a little concerned that we're moving a little too fast for some with less time to keep up. @Bojhan has yet to respond the feedback to his designs. It's important that we get buy-in from the UX maintainers to make sure we are designing a consistent interaction with the rest of core and keeping other problems and concepts in mind.

At the sprint on Sunday we discussed some of the points that @MattA has raised in #20 and Bojhan's designs have already considered some of them. For example, the reason why the 'create custom block' button is hidden further down in the IA is because that button was extremely misleading

Could we please focus our time and energy in Bojhan's original designs for now? I think there are a few tweaks and improvements we can add to it, like adding the AJAX block settings and filtering, we should be very careful of overbaking the designs in one patch.

I've updated the issue summary with Bojhan's designs. Hopefully we can move forward with consensus.

Let's see how this goes...

Notes:

• The old BlockXssTest used to check if block titles were sanitized in the old sidebar. So technically, the test could be removed. Right now it just tests the text on the table, which is probably unnecessary?
• This patch does NOT contain an "add custom block" related section, because we need to decide how to output it. If placed in a help paragraph it would not show if the help module is disabled. It also shouldn't be provided by the block module, since it only applies if the block_content module is enabled. So basically, should the block_content module output as help text or should it alter block_admin_display_form?
• Right now the block content test just checks if the theme query argument works, but if someone wants to make it actually follow a link again, be my guest.

Depending on how this turns out, our remaining tasks would be to:

• Remove the now unused CSS from the sidebar.
• Add the "custom block" paragraph and link.

I gave #71 a try. It looks pretty great and works as expected for me. Looks like the minimal amount of changes here to get the critical issue fixed.
Great work so far!

1. +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -31,4 +61,97 @@ public function listing($theme = NULL, Request $request = NULL) {
   +   */
   +  public function layout($theme) {
   +    $headers = [
   +      ['data' => t('Block'), 'field' => 'name', 'sort' => 'asc'],
   +      ['data' => t('Category'), 'field' => 'category'],
   +      ['data' => t('Operations')],
   +    ];
   +    $rows = [];
   +
   +    // Only add blocks which work without any available context.
   +    $definitions = $this->blockManager->getDefinitionsForContexts();
   +
   

   When manually testing the patch I thought it might be better to sort by category by default, so the behaviour is more similar to before ... Given that I am not sure whether its worth to be able to choose which click sort you want.

2. +++ b/core/modules/block/src/Plugin/Menu/LocalAction/BlockLayoutAddLocalAction.php
   @@ -0,0 +1,37 @@
   +    // The default task is always the default theme since it is given a lower
   +    // weight. Therefore, it should always be the default action target.
   +    $theme = \Drupal::config('system.theme')->get('default');
   +    // The action target requires a {theme} parameter, but that parameter is
   +    // not set in the default task route, so it has to be manually provided.
   +    $route->setDefault('theme', $theme);
   

   Looks perfect for me

3. +++ b/core/modules/block/src/Tests/Views/DisplayBlockTest.php
   @@ -60,19 +60,20 @@ public function testBlockCategory() {
    
   +    $pattern = '//tr[.//td/div[text()=:text] and .//td[text()=:category] and .//td/div/div/ul/li/a[contains(@href, :href)]]';
   +
   

   Wow, that xpath looks really detail, maybe we can figure out something better which also doesn't break too easy, in case the HTML would change. For example maybe it is enough to check for the a href with :href and nothing else.

4. +++ b/core/modules/block_content/block_content.links.action.yml
   @@ -8,7 +8,5 @@ block_content_add_action:
      route_name: block_content.add_page
      title: 'Add custom block'
      appears_on:
   -    - block.admin_display
   -    - block.admin_display_theme
        - entity.block_content.collection
      class: \Drupal\block_content\Plugin\Menu\LocalAction\BlockContentAddLocalAction
   

   Yeah its a good question whether just doing that would be enough, it seems that this could lead to another level of confusion, as its not trivial to find where to add a custom block.

The old BlockXssTest used to check if block titles were sanitized in the old sidebar. So technically, the test could be removed. Right now it just tests the text on the table, which is probably unnecessary?

Well I think there is some value in checking that the labels on the extra page are sanitized properly as well.

This patch does NOT contain an "add custom block" related section, because we need to decide how to output it. If placed in a help paragraph it would not show if the help module is disabled. It also shouldn't be provided by the block module, since it only applies if the block_content module is enabled. So basically, should the block_content module output as help text or should it alter block_admin_display_form?

Good question. In general yes, I think this should live in block_content</block>. Regarding <code>hook_help() vs. hook_form_alter()
I would lean towards the first approach, so you can disable help module, in case you don't need that.

I tested the patch manually and it actually works better than the test fails let me expect ;) I guess most of the test fails are because web tests are failing to set blocks into regions.

Just a few nits from the patch but overall looks good.

1. +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -31,4 +61,97 @@ public function listing($theme = NULL, Request $request = NULL) {
   +      ['data' => t('Block'), 'field' => 'name', 'sort' => 'asc'],
   +      ['data' => t('Category'), 'field' => 'category'],
   +      ['data' => t('Operations')],
   

   Maybe use $this->t instead

2. +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -31,4 +61,97 @@ public function listing($theme = NULL, Request $request = NULL) {
   +   * @return array
   

   @return mixed[] is little bit more explicit

3. +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -31,4 +61,97 @@ public function listing($theme = NULL, Request $request = NULL) {
   +      ['data' => t('Block'), 'field' => 'name', 'sort' => 'asc'],
   +      ['data' => t('Category'), 'field' => 'category'],
   +      ['data' => t('Operations')],
   ...
   +    if ($order['name'] == t('Category')) {
   ...
   +        'title' => t('Enter a part of the block name to filter by.'),
   ...
   +      '#empty' => t('No blocks available.'),
   

   Maybe use $this->t instead

4. +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -31,4 +61,97 @@ public function listing($theme = NULL, Request $request = NULL) {
   +    if ($order['name'] == t('Category')) {
   

   This is a little bit odd. I don't know why it is like this but maybe it could be done some other way.

I Queued the patch for retesting to see which tests actually fail. I'll continue work on this 2 or 3 hours from now.

@dawehner
#3: The href is being used to make sure it's the right block since like 3 of them have the same name. The test also specifically changes the category so that part needs to be checked too. So as far as I can remember, only the text is optional.
#4: Before deciding, note that it is kind of similar to #2513580 in that a functional link should not be part of help text.

@legolasbo Looks like it was just the testbot. Greeeen! :)

+      $links['add'] = [
+        'title' => 'Add block',

This should be wrapped in t() though. Don't know how that even happened.

So I'm pretty sure this would work:
$pattern = '//tr[.//td[text()=:category] and .//td//a[contains(@href, :href)]]';
Don't know why I didn't just do that to begin with... should also let you remove the ':text' field from $arguments as well.

Starting work on the notes in #73, #74 and #77

Note that xano has written a "Plugin Selector" which aims to be a reusable solution (think views add dialog) for drupal 8:
https://www.drupal.org/project/plugin

Its probably out of scope to implement this here but as the current views add dialog implementation already lacks re-usability, I would like to propose that the mid-term solution should be something reusable. We'll need the same for adding rules configurations for #d8rules for example.

I've gone over the issues mentioned to start with a clean slate before further development and will continue with

Depending on how this turns out, our remaining tasks would be to:

Remove the now unused CSS from the sidebar.
Add the "custom block" paragraph and link.

Notes
#73-1 Category is now the default sort column.
#73-3 see #78 below.
#73-4

Well I think there is some value in checking that the labels on the extra page are sanitized properly as well.

This was my motivation to adjust the test instead of removing it.

#73-4 #77-4 We should discuss this further, but I don’t think the discussion should hinder the development of this patch since block placement is the main focus of this issue. In my opinion, the only thing we should discuss in this issue is if the removal of the option to add a new custom block from the block placement form is an acceptable regression or not.

#74-1 replaced all t() calls in BlockListController by $this->t()
#74-2

/**
 * Builds a listing of entities for the given entity type.
 *
 * @return array
 *   A render array as expected by drupal_render().
 */
public function render();

Is from EntityListBuilderInterface and also encountered at other methods returning a render array. It seems to be a convention to document returned render arrays as an array.

#74-3 see #74-1
#74-4 @lauriii Could you clarify what seems odd to you?

#77 Wrapped in $this->t()

#78 manually tested he xpath-query and it selects a unique and correct row. Adjusted the query accordingly.

Oops

1. Looks like a few instances of find & replace were missing the space between => and $this.
   

   -      '#placeholder' => t('Filter by block name'),
   +      '#placeholder' =>$this->t('Filter by block name'),

2. Should use the xpath in #78.

Fixed #83
Fixed my xpath mistake from #81 and #82 and this time waited for a green test locally ;)
Removed unused CSS from block.admin.css by first locating all pages where said file was used and then searching for selectors to see if the css rules still apply. Also added some comments to the remaining sections to clarify why they exist.

.

Great work, but...

+++ b/core/modules/block/css/block.admin.css
@@ -21,88 +22,10 @@ a.block-demo-backlink:visited {
\ No newline at end of file

\ No newline at end of file

While thinking about a good way to formulate the custom blocks help paragraph i encountered 2 things.

1. There were still several mentions of the block library which is no longer a thing in this patch. I've changed said mentions to block listing in stead.
2. The current state of this patch makes it really difficult to explain how to add a new custom block to a region. I'll explain why below.

Let's say i want to add a new block to the layout. My first instinct (even though i know I can't actually succeed that way) is to click "add block to layout". Which leads me to the new block placement page. That page only lists existing blocks and has no way to add new blocks nor does it offer an easy way to return to the regions listing as seen below.

This means I have to go back to square one and follow the successful path where I first click the "custom blocks" tab. On the new page I see a listing of custom blocks and a "Add custom block" button which I click. I can then fill out a form to add a new block.
So far so good, but upon clicking "Save" I return to the custom block listing.

This means I have to click the "Block layout" tab (if I use blocks often and know that gets me to the right page). And then use the steps described above to add the block to the layout.

To me this seems like a lot of (confusing) work. Especially for inexperienced users. And I think it is impossible to write a short paragraph to describe these steps. Even if we were to manage to explain these steps in a short paragraph it would be something like this

"Do, this, that, that, and something else. Or click this link which is some magical shortcut because we removed the 'Add new custom block to layout' button"

Basically i see two options here:

1. Reintroduce the 'Add new custom block to layout' button - low impact on the scope of this patch. At the expense of the primary action.
2. Relocate the Custom blocks tab to the "Add block to layout" page and introduce a "Add custom block" local action there. - medium/high impact on the scope of this patch

My preference goes towards option 1. I think we should limit the scope of this issue to the removal of the sidebar in favour of a new block placement page. We can still address the custom blocks in a follow up issue.

Reintroduce the 'Add new custom block to layout' button - low impact on the scope of this patch. At the expense of the primary action.

This sounds fine for me ... we could even use destination here to redirect the user to the placement block page after adding the custom block.

@dawehner,

Thats what the original implementation did.

Reintroduce the 'Add new custom block to layout' button - low impact on the scope of this patch. At the expense of the primary action.

1+ like this idea, we should keep it simple for now, just like @LewisNyman mention in couple comments already.

Instead of "Add custom block" for creating it, could we not rename that to "Create custom block"

So "Add" is used for placement, and "Create" is used for creation.

It seems a little less confusing.

I see this as Adding Blocks to the layout, including both supplied by core blocks and custom user created blocks.

Then you have the additional functionality of creating your own custom blocks for inclusion into the list of available blocks that can be added to the layout.

Also maybe the block listing/placement page/tab could be subdivided. Showing all blocks, but on collapsible fields for "Core Blocks" "Module Blocks" "User Blocks"

Good point @mbrett5062!
It explains really better about the underlying architecture.

Also maybe the block listing/placement page/tab could be subdivided. Showing all blocks, but on collapsible fields for "Core Blocks" "Module Blocks" "User Blocks"

Such conversations should happen in a follow up.

So, to summarise, remaining tasks:

1. Restore the removed add custom block button, but name it "Create custom block" as suggested in #94
2. Restore + adjust the removed tests relevant to the custom block button.

Possible follow up issues:

• Separate block placement page into two or more collapsible fields for core/module provided and user blocks.
• Relocate the "Custom blocks" tab to the block placement page.
• Load the block placement page in a modal

This issue needs also new tests for the way block page works now

Good work on the issue everyone and thanks for the IS update! We have very nice progress here. Some comments of the patch we have right now:

1. +++ b/core/modules/block/js/block.admin.js
   index aa669fa..643fcad 100644
   --- a/core/modules/block/src/BlockListBuilder.php
   
   +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -306,69 +304,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -          'data-dialog-options' => Json::encode(array(
   

   After removing this use Drupal\Component\Serialization\Json; can be removed

2. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -306,69 +304,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -        'url' => Url::fromRoute('block.admin_add', [
   

   After removing this use Drupal\Core\Url; can be removed too :)

3. +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -31,4 +61,96 @@ public function listing($theme = NULL, Request $request = NULL) {
   +    $rows = [];
   

   Ubernitpick but can we move this variable declaration before the foreach so its explicit that its being populated there?

4. +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -31,4 +61,96 @@ public function listing($theme = NULL, Request $request = NULL) {
   +    $build['filter'] = array(
   

   This could use the new array syntax

5. +++ b/core/modules/block/src/Tests/BlockLanguageCacheTest.php
   @@ -61,7 +61,7 @@ public function testBlockLinks() {
   +      $this->drupalGet('admin/structure/block/list/classy/add', array('language' => $langcode));
   
   @@ -72,7 +72,7 @@ public function testBlockLinks() {
   +      $this->drupalGet("admin/structure/block/list/classy/add", array('language' => $langcode));
   

   (optional) change to have new array syntax :)

This issue needs also new tests for the way block page works now

It doesn't need new tests, because we've adjusted the tests according to the changes we've made. Unless you can pinpoint specific changes which are currently untested of course.

Thanks for bringing the scope back down to the original issue. That's really appreciated.

While the dialog interaction is nice - it is actually completely a hack. Views uses this pattern, but it has not consistently made its way throughout core. We added it to blocks, in the anticipation of rolling it out further - but never made it. I think for now we are safe to assume, that while its a nice pattern - we shouldn't pursue something that is half implemented across core.

Having a dialog in the block placement flow, makes little sense - you are not moving back to the page behind it. This should just be a normal page.

I understand the concerns with regards to adding a custom block. However I am adamant about not having two primary actions on the top of the page (just making the other gray, doesn't help). We have learned in testing Drupal 8 that having two action buttons is incredibly confusing, and this test again made it clear that people do not know what "adding a custom block" necessarily means. At best I would go for option 2) of adding it to the block placement page, however it is a bit of a break in the mental model. So I am not yet convinced.

Fixed a few help text issues:

• and provides a <a href="!block-listing">Custom block listing</a> listing all of them.
• Multiple occurrences to "Place block" from the old sidebar.
• Other general cleanups.

Simplified some labels from "custom block listing" to just "custom blocks" where appropriate.
Updated with most of the changes from #99 (not #5).
Removed the ajax stuff as per #101.

@Bojhan I totally agree with not having another primary action, but I also don't think adding it to the new page we just created is the solution either. We might be stuck with just adding a sentence to the help text, but that's all for a separate issue...

Fixed that one test...

@Bojhan, @MattA,

In your opinion, where does that leave us in regards to adding a custom block from this patch? I guess we'll just accept the regression for now?

Perhaps we should treat custom blocks as content in a same way the D7 bean module does. That would remove the tight coupling of the block and block_custom UI and possibly simplify maintenance. But that's a discussion for a follow up issue.

Fixed some code style issues and ESLint validation of block.admin.js.

Changed array notation for code touched by this patch to use the shorthand notation for consistency and removed an unused use statement.

All the code we have in the patch looks sane and clean. We have still remaining task regarding the add custom block button which should be restored. Setting for NW to get that done.

@lauriii,

This issue is about focussing the block layout page on it's primary task, which is the layout and placement of blocks. While creating and placing custom blocks has some relevance regarding this issue I think we should consider it out of scope for now.

We should open a follow-up issue (perhaps even a critical) to discuss and change the custom_block UI in general. Because besides this button, the "Custom blocks" tab also seems out of place here and when we start moving these things around, more issues will arise which would extend the scope even more. For now let's keep focussing on the changes at hand.

I'm sorry I missed #101. Updating the remaining tasks then.

This looks good in all terms for me so I'm setting this to RTBC.

Lets allow for some review.

Just a clarification: all the custom block related discussions should happen in the follow-up. Right now there is no way to add custom blocks on the Block layout page but instead on the custom block library page.

I'm generally happy with the code as well.

There are some small nitpicks that we could address, like

1. +++ b/core/modules/block/src/Tests/Views/DisplayBlockTest.php
   @@ -51,7 +49,7 @@ public function testBlockCategory() {
   -    $edit = array();
   +    $edit = [];
   

   Conversion to new array syntax in related code.

2. +++ b/core/modules/menu_ui/src/Tests/MenuTest.php
   @@ -181,7 +181,7 @@ function addCustomMenuCRUD() {
   -  function addCustomMenu() {
   +  protected function addCustomMenu() {
   

   Correctly use protected (or public) for class methods.

These changes are actually scope creep, not sure if this is a showstopper. The code is nice and clean like it is, but might be breaking a few policies.

Follow-up issue for the custom block module: #2523154: Improve workflow for creating custom blocks

+++ b/core/modules/block/js/block.admin.js
@@ -28,8 +28,8 @@
+         * @param {number} index The index of the block.
+         * @param {HTMLElement} label The label of the block.

Small nit, update the label description to: "The element containing the block name." Even with the type-hint, the description makes it sound like a string and I'm getting very tired with the poor (to put it nicely) documentation in core (mostly class properties that just restate the variable name instead of actually explaining what it is or what it is used for).

Also, might want to rename that function too since it doesn't just "show" rows.. it toggles them.

@googletorp Oh, you have no idea how hard I had to hold myself back from setting visibility modifiers on those... sadly, I determined that it probably was issue creep. :(

Fixed both nitpicks in #117 and a array notation i found because of #115-1.

In regard to #115-2, I don't think changing the other visibility modifiers should be part of this patch.

+++ b/core/modules/block/src/Controller/BlockListController.php
@@ -31,4 +60,91 @@ public function listing($theme = NULL, Request $request = NULL) {
+  public function layout($theme) {

Thank you for adressing the ordering issue mentioned before!

I think the points in #115 are valid, but should not block the issue from being done.

Nice to see this RTBC!

I think we will need @webchick's signoff on this one for the product implications, so assigning to her.

Assuming the testbot agrees, +1 RTBC.

+1 on RTBC

I'd like a chance to review this as well, as the block.module maintainer.

Should show the actual block type, not just "custom".

I know that we are pushing to kill the criticals off, but I'd like to leave this on needs review for a bit to make sure we are not missing any gaps.

@Bojhan The list generation is pretty much a copy/paste of the old code. Currently, core just says "Custom" too, so that should be a separate feature request.

Also #125 shows "Block layout" twice in breadcrumbs so needs to be fixed here

I know that we are pushing to kill the criticals off, but I'd like to leave this on needs review for a bit to make sure we are not missing any gaps.

I'm sorry but noone is stopped from looking at RTBC issues ...

@andypost So trivial... and actually from the a menu we haven't touched, but will update with fix in a few.

#127 fixed locally, waiting for tests to pass.

@legolasbo Oh hmm... well here's what I had. First part is the fix for #127, but I also added some more context to the page by adding the theme name to the page title since we probably shouldn't rely solely on the breadcrumb for that.

@MattA,

I was just about to upload my patch, but I've merged in your changes aswel. Unfortunately I don't have enough time remaining to wait for all tests to finish (the tests pass so far though).

We do need tests for the Dynamic title.

Tagging based on @tim.plunkett's comment in #124 and my comment above. (Reference: #2457875: [policy] Evolving and documenting Drupal core's structure, responsibilities, and decision-making) Once we get these signoffs, let's list them in the summary. We can wait to assign to @webchick until it's RTBC again (after @tim.plunkett's review, fixing the bug @andypost found, etc.).

@Bojhan, did you want the opportunity to review it further as well yourself? Tagging for that too. When you think it's acceptable, please untag and mention it in the summary. :) I am sure @webchick will be glad to hold off on committing it for a few days if it's just for more general feedback.

And finally, NW for the added test coverage.

Just going to see how tests do with a clean version of the patch from #131 (also has a title test).

@MattA,

It seems we've taken a different approach to solving the same problem. I like your solution, but my approach matches the pattern already used by the block(_content) module. Which is why I think my pattern is a better fit. But since I don't have any time to work on this during the weekend I'll leave it up to you.

more nits

1. +++ b/core/modules/block/block.routing.yml
   @@ -37,7 +37,16 @@ block.admin_display_theme:
   -    _title: 'Block layout'
   +    _title_callback: 'theme_handler:getName'
   

   Looks better to use controller's defined title callback to make like "Blocks for @theme"

2. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -143,12 +141,14 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Override the page title of theme sub-tabs.
   +    $form['#title'] = $this->t('Block layout');
   +    ¶
        // Add a last region for disabled blocks.
   
   +++ b/core/modules/block/src/Controller/BlockListController.php
   @@ -16,6 +20,41 @@
   +  protected $themeHandler;
   +  ¶
   +  /**
   
   +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -132,23 +132,28 @@ function testBlockAdminUiPage() {
   +    // Also test the dynamic title.
   +    $this->assertTitle(t('Add block to layout (Classy)'));
   +    ¶
        // Trigger the custom category addition in block_test_block_alter().
   

   trailing white-space

@legolasbo Umm, might be too early for me, but I don't follow. What pattern is that?

@andypost Same with #1 here. I don't know what "controller's defined title callback" is referring to, but I think the breadcrumb title should match the tab title, so going to keep it as is for now. Fixed the whitespace though.

@MattA,

In core/modules/block/block.routing.yml two routes are defined for the block layout page.

block.admin_display:
  path: '/admin/structure/block'
  ....
block.admin_display_theme:
  path: 'admin/structure/block/list/{theme}'
 ....
 

The first is used for the default theme, the second for all other themes. Your patch does not provide this 'pattern' while mine does. Besides that i think the path used by the current patch (admin/structure/block/list/{theme}/add) is not really descriptive of what should happen there. while admin/structure/block/placement(/{theme}) describes the main action at the resulting page, which is placing a block.

For the same reason I also think the path admin/structure/block/<strong>list</strong>/{theme}/add should be changed to admin/structure/block/<strong>layout</strong>/{theme}/add. But that's for another issue.

I'm gonna try to merge two patches we have here so that we have one patch everyone can work on! I'll come back with a patch soon.

I won't write a patch here because of the differences were smaller than I thought. Just as an future reference it is very confusing if there is patches going in their own trees, especially if its not clearly documented what are the differences between them.

I guess what this is mostly about is

Patch #132 has path structure of:

/admin/structure/block
/admin/structure/block/list/{theme}
/admin/structure/block/placement (for the default theme)
/admin/structure/block/placement/{theme}

Patch #140 has path structure of:

/admin/structure/block
/admin/structure/block/list/{theme}
/admin/structure/block/list/{theme}/add

In #140 user is always redirected to a URL containing the theme. Also the BlockListControllers layout method has the layout as a required parameter in #140.

I personally think that the structure is better on #132, but it might be worth changing /admin/structure/block/list/{theme} into /admin/structure/block/{theme}

@legolasbo Ah I see. Well both of those paths were already there, and the new page needed a path with '{theme}' defined, so it was logical to create the new page there (not to mention the only option).

That said, we are getting into issue creep. Again. The goal here is to clean up the UI layout. We can save making route paths prettier for later.

I'm okay with fixing that later.

Minor note: Will need a re-roll, depending on which goes in first, due to #2514998: Reduce fragility in the monolithic BlockListBuilder.

@Lauriii, #132 already has MattA's page title 'feature'. All it needs is a few tests fixed because they still refer to the paths introduced by MattA. Since those tests were altered before to make the new page work, they shouldn't be hard to find.

@MattA, I disagree on improving the paths later. We've introduced this new page in this patch and should make sure we get it right the first time. It's a minor detail and we're still waiting on the UI changes to be approved anyway. So let's just fix the tests in my patch #132 and we're good to go with proper paths.

I find the proposed changes ( and after testing patch ) makes the block placement very much harder and less usable than before.
I can see the benefits of rearrangement of custom block button , but has anyone tested with just moving the sidebar to the left and leaving the block sidebar as is ( i personally love this feature but do agree that the + sign beside the block is confusing as it gives the impression that you can drag the block into a region ).

@bill richardson Yes. All the way back in #16. See #17 for why not.

Also, just for clarity, by "block placement" do you mean moving an existing block around (although that hasn't really changed) or adding a new block? Assuming it's the latter, we have a couple follow-up issues to help, such as #2513528: Add a link to add a block in empty regions on the Block layout page. and #2513520: Add a contextual link to add a block to a region. So once the follow-ups go in (and hopefully the intermediate pages get ajax-ified like they were in #55) the old sidebar functionality should be back.

If we have to proceed this way , at least implement mockup1 , where region was displayed in a column so that a block could easily be moved to another region via the dropdown .

It's still there, click 'show row weights'. Currently the sortable table is focusing on its job, which most people apparently prefer using. So if you want to use the select list (and get rid of that pesky drag icon) you'll have to click that exactly once, since your browser will remember the setting.

@bill richardson @MattA @legolasbo
Whatever happens here keep in mind that a) we really want to fix the critical part of the issue and b) we can iterate easily and refine/improve it later

@dawehner,

Both patches (#132 and #140) are identical except for the fact that #140 uses sub-optimal paths and #132 stil has a few failing tests (because of the changed paths). I'm fine with #140 going in as is and fixing the paths in a follow-up issue. However if that doesn't happen before Monday morning I'd prefer to fix the tests on #132. In any case I'm not planning to do any further UI changes. I think the current patch(es) solve the issue at hand and any other UI changes should be and will be addressed in the follow-up issues which are already created.

Monday i've got all day 10:00 - 17:00 (GMT+1) to take this home. The tests (for #132) should be fixed within an hour, which should be the last bit to finalise the patch. Any new feedback from a UX review or review by subsystem maintainer or product manager can be fixed then aswel. I'll be on IRC in the mentioned timeframe if any discussion is required.

I still think it should be a separate issue, but I was also thinking, what if instead of adding more routes, we just rename the original. So instead of:

/admin/structure/block
/admin/structure/block/list/{theme}
/admin/structure/block/placement (for the default theme)
/admin/structure/block/placement/{theme}

Note: Really, route 3 shouldn't even exist either, because you should just have route 4 with {theme} as an optional parameter and dynamically change it in a RouteSubscriber.

We just have something like:

/admin/structure/block
/admin/structure/block/layout/{theme}

The path for the add action would then become: /admin/structure/block/layout/{theme}/add.

This allows us to not add more routes, not add additional code required to get the default theme in the controller and not have to subsequently create a new test. With the added benefit of the current theme tabs being under block/layout/{theme} which makes more sense than leaving block/list/{theme} in.

Just reminding here that which paths block layout is providing shouldn't be part of a discussion in the critical issue. How does it make the UX of the block layout better? It is important to have sane path structure but it can fixed in a normal/major follow-up.

The page title missing when on custom blocks tab with sub menu block selected.

@bill richardson That's actually a problem with the Views module from what I can tell. Same thing happens with the front page view on my system. "Welcome to Drupal 8" disappears. If you disable the module you can see that it isn't a bug related to this.

...was added/fixed way back in #140.

MattA - yes the page title error is in beta 12 --- nothing to do with patch --- sorry for the noise.

Yeah, no changes have gone into the patch, the only thing happening has been discussion. Went ahead and created #2526766: Improve naming of block paths for @legolasbo though. Any relevant comments can go there now.

I was going through the block module's files for a different issue and it seems like we forgot to remove the block-list.html.twig file that was used for the two-column layout (and it's hook_theme entry).

@MattA,

You are right. I actually removed those in #31, but you forgot them in your fresh start. I'll remove them and will also check if we've got tests for all relevant changes.

Anyways, also renamed BlockLayoutAddLocalAction to BlockLayoutLocalAction since it's technically reusable with any action on the block layout page (although we've probably learned our lesson about that...) and removed an unused trait.

testCandidateBlockList() used to also test the block placement URL but that test was removed. Reintroduced that check.

While reviewing #167 to find additional things which should be tested, I encountered a discrepancy with regards to the new block placement page.

On /admin/structure/block/list/bartik/add (default theme) there is a breadcrumb trail. If i were to click the last of it's items ('bartik').
.

Expected result:
I would expect to navigate to /admin/structure/block since bartik is the default theme.
.

Actual result:
However this link leads to /admin/structure/block/list/bartik which has broken local tasks because the 'bartik' task is not selected.
.

Solution:
I solved this problem by implementing the path change I suggested in #132 which does 3 things for us.

1. Fix the newly introduced bug described above
2. Removes the Add block to layout (THEMENAME) title -which feels hacky to begin with - in favour of just Add block to layout.
3. Adds local tasks to the the "Add block to layout" page which enables the user to switch themes. Just like on the Block layout page.

Besides this I've also changed any tests which were using hardcoded paths to use paths generated from the route, which should make a future change of the paths (if needed) easier to implement. To keep the scope as small as possible I've only touched things which we were already changing to begin with.

Oops, I missed a use statement.

I’ve walked trough all comments since the last RTBC status in #120 to find any actionable tasks and list their status.

#125 - Bojhan - Category should show the custom block type, not just Custom. Created #2527522: Show block type in stead of "Custom" on block placement page

#127 - andypost - “Block layout” is shown twice in the breadcrumbs FIXED