Problem/Motivation

There are the things in the Add blocks page (Block library listing) which are block plugins. Those hold the description and the body. (for custom block, basic block type). Then there are other things on the Blocks admin listing. Those are block instances of those block plugins. The dropdown buttons on the blocks in the UI on the site, in place in the regions, have items in the dropbutton list, and there are also items in the dropbutton list on the block admin list. Some of those items deal with the block plugin: edit, delete. Some deal with the block (block instance): configure. This is confusing.

Also, on the block admin list, the description is there, but not the block title. So multiple instances dont look any different because the title is not used there.
This is also confusing.

Proposed resolution

Use the title on the block admin listing.
Put the action items for just the instance next to the title.
Put the action items for just the plugin next to the description.

Remaining tasks

  • Create initial patch to change the table.
  • more tasks to come later...

User interface changes

Yes. new way of using the block admin table.

API changes

No API changes.

Files: 
CommentFileSizeAuthor
#61 block-instance-1956448-61.patch8.87 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 60,095 pass(es).
[ View ]
#56 interdiff.txt1.57 KBbenjy
#54 block-1956448-54-reroll.patch8.35 KBbenjy
FAILED: [[SimpleTest]]: [MySQL] 59,034 pass(es), 6 fail(s), and 4 exception(s).
[ View ]
#54 block-instance-1956448-54.patch9.4 KBbenjy
FAILED: [[SimpleTest]]: [MySQL] 59,716 pass(es), 6 fail(s), and 4 exception(s).
[ View ]
#54 interdiff.txt1.05 KBbenjy
#46 block-1956448-46.patch8.44 KBed.hollinghurst
PASSED: [[SimpleTest]]: [MySQL] 58,984 pass(es).
[ View ]
#46 interdiff.txt1.84 KBed.hollinghurst
#45 Screen Shot 2013-09-27 at 11.06.52 AM.png25.92 KBtkoleary
#44 Screen Shot 2013-09-27 at 10.42.27 AM.png36.84 KBtkoleary
#42 block-1956448-42.patch7.35 KBed.hollinghurst
FAILED: [[SimpleTest]]: [MySQL] 58,600 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#42 interdiff.txt712 bytesed.hollinghurst
#37 block-1956448-37.patch6.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,569 pass(es).
[ View ]
#25 Block_layout___Drupal_D8_Dev.png117.99 KBjessebeach
#19 Screen Shot 2013-09-05 at 7.24.43 PM.png154.08 KBtim.plunkett
#17 block-1956448-17.patch5.86 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1956448-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 interdiff.txt4.21 KBtim.plunkett
#15 block-1956448-15.patch1.65 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,381 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#11 add-block-category-block-admin-listing-1956448-11.patch1.04 KBbenjy
PASSED: [[SimpleTest]]: [MySQL] 58,457 pass(es).
[ View ]
#2 title_description.png47.89 KBYesCT
#2 just_title.png43.45 KBYesCT
#1 no_title.png239.99 KBYesCT

Comments

YesCT’s picture

Title:use the block instance title on the block admin listing and make obvious which drop button items effect the block vs the plugin» use the block instance title on the block admin listing
StatusFileSize
new239.99 KB

I change my mind.. doing both in one issue might block the simpler change of just adding the title.

on admin/structure/block add a block
on admin/structure/block/list/block_plugin_ui%253Abartik/add add a custom block

add another instance of that. (go back to the add block page)

no_title.png

YesCT’s picture

StatusFileSize
new43.45 KB
new47.89 KB

How to incorporate the title?

use both in the same column of the table?
a)
title_description.png

or, make two different columns of the table, one for the title, and one for the description?

The title, is by default the same as the description unless it is changed (by configuring it).
So,
b) just use the title
just_title.png
or
c)
use the title, and then expand out with a drop down like the pattern used for the modules page.
In the more details expansion it can list: the description (and what module it is from).

xjm’s picture

I'm pretty sure this issue is a duplicate of... something somewhere. :)

YesCT’s picture

xjm’s picture

Maybe it's a duplicate of that and my IRL conversations with Kevin, but I still swear there's an issue I'm not finding related to this.

I think there should be two columns: The configured instance title, and then the block name (which is used currently).

xjm’s picture

Edit: Oops, wrong issue. :)

Bojhan’s picture

Title:use the block instance title on the block admin listing» Use the block instance title on the block admin listing
Issue tags:-Needs usability review

Lets get an initial patch up, I think most sense makes [Instance name] - [Block type] - [Operations]. Where block type, is the kind of module it comes from.

YesCT’s picture

in untangling the name, title, description, summary comment on #663946-83: Merge "List links" page into "Edit menu" page is related to similar pattern and how it relates to menus.

xjm’s picture

@Bojhan already reviewed this idea in #7--the next step is an actual patch. Nothing more to review here until then. :)

sdboyer’s picture

#2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues would change the API used to access the data for this. wouldn't really make it easier, but getting the necessary data isn't actually that hard.

in any case, giant +1 to this.

benjy’s picture

Status:Active» Needs review
StatusFileSize
new1.04 KB
PASSED: [[SimpleTest]]: [MySQL] 58,457 pass(es).
[ View ]

We now only have the block instance title so thats no longer an issue.

Patch adds a second column with the block category. Do we have any methods for standardising the output? Eg, capitalising the first letter and replacing underscores for spaces?

tim.plunkett’s picture

What does the category have to do with this issue? I don't think we have any plans to add that to the table.

To answer your question though, we have #2060859: Make Block plugin's "category" a translatable string

benjy’s picture

@Bojan is asking for it to be added in #7? I spoke with him last night on IRC before I did it I got the impression it still needed adding.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
Status:Needs review» Postponed
Issue tags:+Block UI overhaul
tim.plunkett’s picture

Status:Postponed» Needs review
StatusFileSize
new1.65 KB
FAILED: [[SimpleTest]]: [MySQL] 58,381 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Here's a proposal:

We always show the block category.

And instead of the admin label, we show the block title itself. 90% of the time, the admin label and the title are different, we show title (admin_label)

See the screenshot (taken with #2060859: Make Block plugin's "category" a translatable string applied for the pretty category names).

Status:Needs review» Needs work

The last submitted patch, block-1956448-15.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new4.21 KB
new5.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1956448-17.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixed the test assertions.

klonos’s picture

@tim.plunkett, #15:

See the screenshot (taken with #2060859: Make Block plugin's "category" a translatable string ...

What screenshot? Forgot to attach it perhaps?

tim.plunkett’s picture

StatusFileSize
new154.08 KB

Heh, thanks
Screen Shot 2013-09-05 at 7.24.43 PM.png

klonos’s picture

No problem mate ;)

This looks awesome btw! I really like how we are actually moving not-used blocks from the "disabled" area to a dedicated "block library" that sits nicely to the right. It makes more sense and way much better UX!

Thanx for your work on this.

tim.plunkett’s picture

@klonos There is still a disabled section. That's unchanged. And the sidebar is already in core :) But thanks for your compliments!

klonos’s picture

Oh, I was under the impression that we were getting rid of the disabled section and use the sidebar as a replacement. The disabled section is not visible in the screenshot back in #19 and that caused me to think that we've actually ditched it already. My bad ;)

benjy’s picture

To me it feels somewhat confusing that sometimes the block name is showed on it's own and then at other times it's in brackets. A few more suggestions.

  1. We make the first column always the title and then either add another column for the block name or have one column with category|block name
  2. Maybe we don't need the block name when we have a title. We could always show the block name in the left column and if a title is set we just show that instead. However this is probably worse than @tim.plunkett's current solution.
tim.plunkett’s picture

Is this still a tag?

To sum up:
We currently have three columns in the block admin listing: Block, Region, Operations.
In the Block column, we currently display the admin_label of the block, which is immutable and defined at the module level, and is the same for all placed instances of the same block.
We have two other pieces of information that might be useful to display: The block category (which is as static as the admin label) and the block title (which is customizable for each block instance).

We need to decide which of these 3 things to show, how many columns to show them in, the order of those columns, and the labels for the column header.

jessebeach’s picture

re:23, Adding a column for Title and removing the Category column would look like this:

Screenshot of the blocks UI page. A column Title has been added to the right of the Block column.

I removed the Category column because things are really starting to get cramped. The category seems important as a way to chunk blocks that the user is seeking to place, but it becomes less important as an data point for placed block, since this information cannot be changed and it does not help the user make any further decisions.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Issue tags:+Needs usability review

So in that screenshot, 3 of the 4 blocks have identical titles and admin_label. Which is why I proposed what I did in #19.

As I can't really drive this forward without consensus, unassigning.

Bojhan’s picture

Why can't we just have one title? The instance title? I don't really see the point of needing to expose the plugin title? (it is handy, but when you assign a custom title, I hardly see the need). We can have it on edit, but in general showing two titles will be confusing anyways. To me #19 looks cleaner, because we at least aren't exposing two similar table columns.

benjy’s picture

+1 for #19, that seems clear to me.

klonos’s picture

The way it is now in D7 with most contrib out there is to append the module name in brackets after the block title or in the case of views to prepend it without brackets and use a colon instead. So in one of my sites I currently have:

Site Navigation (Nice menu)
Site Management (Nice menu)
Breadcrumb (Crumbs)
View: Front page slider

I guess this is a common pattern and to tell the truth it does help me pinpoint these blocks. On the other hand this seems to be the case only wit blocks that do not expose their title, so I guess we can adopt something like that (but be consistent - either standardize appending with brackets or prepending with colon). This should happen only for the block admin UI though because block titles can be exposed and overridden/hidden. Whatever we do for the block UI should never get in the way of those features.

I propose to use a single "Block" column that includes block name (not necessarily block title) + module/plugin that offers the block. Does that make sense?

klonos’s picture

...for example:

If the block title was not overridden use: Administration (Menu)
If the title was overridden with something custom: myCustom Title (Menu/Administration)
If the title was set to <none>: (Menu/Administration)

Thoughts?

tim.plunkett’s picture

@klonos "module/plugin that offers the block" has been superseded by "category", see the screenshot in #19.
And as far as the "View: " prefix goes, see #2086447: Remove 'View:' prefix from views block derivative admin labels

klonos’s picture

@tim.plunkett: yeah, I've been following this issue since the beginning so I'm aware of the design in #19, but unless that column is actually used for sorting/filtering it makes no sense to waste horizontal space on a separate column. That's why I proposed to merge it in the "Block" column.

Thanx for the link to the issue, but my proposal can be implemented regardless if #2086447: Remove 'View:' prefix from views block derivative admin labels is committed or not. That is if we do want to save horizontal space I mean.

jessebeach’s picture

The hesitance I have with the parenthetical block name approach:

DEEZ AR MAH TOOLZ (Tools)

Is that (1) the presence of the parenthesis is inconsistent and (2) it's not immediately apparent what the label in the parenthesis refers to because it doesn't have a column label to indicate what type of information it encodes. The block name information is useful, but maybe not in this table. What if we put this information in a read-only field on the configure screen (like the machine name)? If we do that, then we only list the block title in the table. We give the user a pre-filled title based on the block name. The title field is required, so we'll never end up with the case of a titleless block.

tkoleary’s picture

@tim.plunkett @klonos

Jesse has won me over on this one but I would suggest we test it (with all other patches to this UI applied as well) when we get to Prague because I am certain that there are usability issues we are overlooking.

ben.kyriakou’s picture

#17: block-1956448-17.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Usability, +Block plugins, +Block UI overhaul

The last submitted patch, block-1956448-17.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new6.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,569 pass(es).
[ View ]

Ditching the " (admin_label)" suffix.

McGo’s picture

Patch from #37 works for me.

My thought on that: Think of 2 custom blocks and 4 instances of those blocks. You are not able to see directly which block instance is created from which custom block. This is applicable to nice menus, too.... But: The question is: What is more important, to see which is the base block of the instance or to see the category of the base block. The instance label currently shown is important and defaults to the label of the base block. By creating the block, the user made the decision to override it and for me this implies the decision that the overriden label is more important than the label of the base block.

dmitrii’s picture

Status:Needs review» Reviewed & tested by the community

I looked at the code manually. Patch works.

ed.hollinghurst’s picture

Also reviewed for a UX point of view and I can confirm that this works as designed, with the Block Instance title appearing in line with the screenshot in #19.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs work

Oh, I completely forgot the other half of the discussion I had with @jessebeach and @Bojhan about this:

We want to include the admin_label on the block config form as a indicator of what they're editing, since when the label is customized you have no way of knowing otherwise.

ed.hollinghurst’s picture

Status:Needs work» Needs review
StatusFileSize
new712 bytes
new7.35 KB
FAILED: [[SimpleTest]]: [MySQL] 58,600 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Please see the attached updated patch that adds the admin_label to the Block Configuration form.

Status:Needs review» Needs work

The last submitted patch, block-1956448-42.patch, failed testing.

tkoleary’s picture

Status:Needs work» Needs review
StatusFileSize
new36.84 KB

@ed.hollinghurst

Works great but I don't think there is a need to have it take up two lines. We are tight on vertical space in forms in general and specifically in forms in modals so we need to start implementing ways to mitigate this in every small way we can. See attached image.

This should be achieved in a good abstracted way by adding a class for any instance of a field item in core that is printed on the page but not editable (it's not really disabled since the field is in use and you can't "enable it" it's just not editable).

I *think* you would do it like this:

.form-item-settings-admin-label-printed  {
  display: inline;
  margin: 0 .5em 0 0;
}
.form-item-settings-admin-label-printed:after  {
  content: ':'
}

Resultiing in:

image

Ignore the after pseudo-element if it's easier to selectively add the colon to the string using PHP.

tkoleary’s picture

StatusFileSize
new25.92 KB

Actually my last image shows an improper context. Clicking the configure link presents this form:

image

But it should obviously be done in both places (place and configure)

ed.hollinghurst’s picture

StatusFileSize
new1.84 KB
new8.44 KB
PASSED: [[SimpleTest]]: [MySQL] 58,984 pass(es).
[ View ]

My patch in #42 failed as the test checks to ensure only certain form elements are present.

I've therefore updated the test and attach a new patch and difftext.

I also don't think it's necessary to check_plain() against the admin_label so I've removed this. Although please advise if you think this should be there.

tkoleary’s picture

@ed.hollinghurst

I think we cross posted and you missed #44 and #45

ed.hollinghurst’s picture

Displaying the title inline is a good idea. Like you say, it seems like we need a generic solution to implement this.

However I am not aware of a way in Drupal's form system that enables this to be implemented. Can anybody else advise on a way to achieve this?

bsnodgrass’s picture

Patch on #46 works as described.

This is my first look at this and while I like the list in the sidebar I miss not having an easy way to enable multiple blocks and move them into regions without having to open each block configuration.

tkoleary’s picture

@bsnodgrass

This is my first look at this and while I like the list in the sidebar I miss not having an easy way to enable multiple blocks and move them into regions without having to open each block configuration.

That and other features of this UI are compromises that optimal has made with implementable.

benjy’s picture

Status:Needs review» Needs work

#44 still needs to be addressed.

tkoleary’s picture

@benjy

Right. If it can't be done with PHP it most certainly can be done with javascript.

jessebeach’s picture

Let's move ahead with the presentation illustrated in the image included in #44.

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new1.05 KB
new9.4 KB
FAILED: [[SimpleTest]]: [MySQL] 59,716 pass(es), 6 fail(s), and 4 exception(s).
[ View ]
new8.35 KB
FAILED: [[SimpleTest]]: [MySQL] 59,034 pass(es), 6 fail(s), and 4 exception(s).
[ View ]

I re-rolled the patch and then added the CSS pretty much exactly as suggested. Not sure if it's the right place but I added it into block.admin.css.

Status:Needs review» Needs work

The last submitted patch, block-instance-1956448-54.patch, failed testing.

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new1.57 KB
new8.82 KB
FAILED: [[SimpleTest]]: [MySQL] 59,035 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fixed the issues with the re-roll. Still got a few failing tests but not sure why at this point.

Status:Needs review» Needs work

The last submitted patch, block-instance-1956448-56.patch, failed testing.

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new552 bytes
new8.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,386 pass(es).
[ View ]

Errors were caused by the setUp method been called multiple times and the blocks stored in $this->blocks were no longer inline during testBlockAdminUiPage().

I'm not sure this is the right fix, just want to use the test bot, i'm going to think about this a little further.

jessebeach’s picture

The UI/CSS approach looks fine. As far as I can tell, the PHP looks good as well, but I'd appreciate a second opinion there.

jibran’s picture

Status:Needs review» Needs work
  1. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
    @@ -264,6 +264,9 @@ public function form(array $form, array &$form_state) {
    +    $form['#attached']['css'] = array(
    +      drupal_get_path('module', 'block') . '/css/block.admin.css',
    +    );

    Why not library?

  2. +++ b/core/modules/block/lib/Drupal/block/BlockListController.php
    @@ -255,16 +257,19 @@ public function buildForm(array $form, array &$form_state) {
    +            '#markup' => check_plain($info['label']),

    String::checkPlain() now.

benjy’s picture

Status:Needs work» Needs review
StatusFileSize
new669 bytes
new8.87 KB
PASSED: [[SimpleTest]]: [MySQL] 60,095 pass(es).
[ View ]

Changed to String::checkPlain().

Not sure about the library. We already have block.admin.css used in a library but that comes with a JS file that we don't need here. So, we have three options.

  1. Leave as is
  2. Create another library including just this CSS file.
  3. Re-use the existing library and have an un-needed file on the page

The above are in order of preference for me, but i'm happy for someone to say otherwise.

jibran’s picture

We are loading extra css anyways. Why not separate the css file and create a new library. I think it is an overkill let's not get into that at all and leave it as is. It is two lines of css anyway.

jessebeach’s picture

Leaving the #attached/css is fine here. No need to create a library abstraction for a 2 lines of CSS!

jessebeach’s picture

Issue summary:View changes

Updated issue summary, I dont know how that tag got added back in... anyway. ready for initial patch.

sdboyer’s picture

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

I think this is ready to go. Thanks everyone for following up on this!

jessebeach’s picture

Issue tags:+Spark, +sprint
webchick’s picture

Status:Reviewed & tested by the community» Fixed

I think this makes sense.

Before, I added two instances of Search form to the blocks page, "Search form" and "Search formitty" but both looked identical in the listing, so it was impossible to tell them apart.

Now, the different block instance names are properly identified, along with their categories in case I for some reason have a "Recent comments" view and a "Recent comments" custom block.

Ergo, committed and pushed to 8.x. Woohoo!

jessebeach’s picture

Issue tags:-sprint

Status:Fixed» Closed (fixed)

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