Use the block instance title on the block admin listing

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

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

Edit: Oops, wrong issue. :)

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.

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

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

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?

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

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

It's not in #2055853-35: [meta] Improve the place block UX; Separate interaction from the create block UX; Improve the existing blocks-by-theme layout, which is the new rounds of revisions. #7 is from March.

We'll see!

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

Fixed the test assertions.

@tim.plunkett, #15:

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

What screenshot? Forgot to attach it perhaps?

Heh, thanks

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.

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

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

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.

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.

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

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.

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.

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.

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

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?

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

@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

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

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.

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

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

Ditching the " (admin_label)" suffix.

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.

I looked at the code manually. Patch works.

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.

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.

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

@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:

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

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

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

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.

@ed.hollinghurst

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

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?

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.

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

#44 still needs to be addressed.

@benjy

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

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

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.

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

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.

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.

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.

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.

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.

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

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

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

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!