[UX] Prefix admin labels of all menu-provided blocks with "Menu:" and views-provided blocks with "View:"

[jenlampton]

1. It bothers me that Account menu is the first block in the Add blocks list.
2. I would prefer if all menu blocks were listed together.
3. I think it would be clearer for users if the blocks that were menu blocks clearly identified themselves as such (even/especially when the word 'menu' does not appear in the menu name.)

I propose that we prefix all menu blocks with "Menu:"

This is a follow up to https://github.com/backdrop/backdrop-issues/issues/2241, where I saw the Add block list getting cleaned up, and wanted to add another clean-up task.

[klonos]

I agree and you actually beat me to filling an issue for this @jenlampton. I intended to file one more generic though where we also add the word "view" to any views-provided blocks. You see, in both D7 as well as D8, they have indicators as to if a block is provided by a view...

D7 "Blocks" page:

D8 "Place block" dialog and "Block layout" page:

[klonos]

PS: in D8, the instant search field in the "Place block" dialog only searches block names (I had to type "re" to filter to the recent comments/content blocks because "view" would not work), but our search filter does fussy search on both block title/type as well as description text, So users could start typing "view" or "menu" and the list of blocks would be updated accordingly. UX++

[klonos]

...so @jenlampton do you agree to make this a generic issue where both "View: " and "Menu: " are added to respective types of blocks?

[klonos]

Oldie, but goodie 😅 ...I thought that this would be simpler than what it ended up being, because we are not 100% consistent across the ways that we provide the various block titles in the various hook_block_info() implementations, and certain functions are already adding prefixes to things where they are used in other places etc. ...I tried to document as many things that could potentially trip others up in the future - I hope that the inline comments aren't too excessive.

Here's the PR: https://github.com/backdrop/backdrop/pull/4746 ...it is adding (or making consistent) the prefixes for:

• views-provided blocks
• menu blocks
• dashboard blocks (either from the Dashboard module itself, or if they have dashboard as a required context)

Turns out that the Field: prefix (for layouts with the node/% path/context) is already added in field_get_block_list() - I didn't change that.

Should we be adding the module name as a prefix? ...at least for non-core modules?

[stpaultim]

@klonos - I'm confused, because it seems to me that views already does this. Yet, you indicate that we should improve it and even indicate that your PR does it.

Yet, this is what I see when I look at a Backdrop site before this PR.

~~What is not happening, in regards to views blocks, that you wish would happen?~~ ~~What did your PR do to improve the situation with views blocks? I did some testing to try and find an improvement, but so far I've not had luck.~~

EDIT: Nevermind. I see that for core previded views it was not labeling with the correct prefix. Both of the views in my screenshot were new views, which did seem to get that prefix automatically.

Otherwise, this PR is working great for Menu blocks.

[stpaultim]

If I am correct. The following describes the current situation:

1. Dashboard blocks all had a prefix, but it was a hardcoded prefix with a dash "-" instead of a colon ":".
2. Views did not provide a prefix for default views provided by core, but did add a views prefix "View:" to new views created by people using the site.
3. Menu blocks were not getting any kind of prefix.

AFTER PR:

All three types of blocks automatically get appropriate prefixes followed by a colon and then the block title.

I think this is a nice improvement to the UI for placing blocks. Works for me!

Since this issue has a PR that seems like it could be close to ready, I'm giving it the Milestone Candidate tag. If anyone else thinks this is close and supports adding it to the Milestone, they can do so. https://docs.backdropcms.org/documentation/adding-milestones-to-issues

I've removed the Needs Testing tag, but would love to see at least one more person give it a test and make sure I didn't miss anything.

[klonos]

@stpaultim yes, you figured it out correctly 👍🏼 ...there were inconsistencies with block titles in the "Add block" dialog:

• Some had prefixes, some didn't. That's OK, but then there is no grouping whatsoever and related blocks were "scattered" around the list. We should have some sort of grouping for some types of blocks (which is one of the reasons why @jenlampton raised this issue in the first place).
• Those block titles that did have prefixes were visually inconsistent: some were using : as the separator between the type and the title of the block, others were using dashes.
• There were inconsistencies with certain types of blocks, where the prefix was added for the majority of the blocks provided in the respective hook_block_info() implementation, but some were missed. dashboard_block_info() for instance was adding the prefix to all blocks, with the exception of the "Backdrop News" block. I'm assuming that this has happened because that block was added at a later point instead of along with the rest of the other Dashboard blocks. We must have missed the inconsistency when we reviewed the PR that added it. In any case, making sure that the appropriate prefixes are being added somewhere "centrally" makes this error less prone to happen + developers have one less thing to worry about: just add the block title without any prefix, and one is added if needed.
• Views was already adding the View: prefix in certain cases (when no admin label is explicitly provided for the block). So some views-provided blocks had a prefix, some didn't. I wasn't sure where else those existing prefixes were used, so instead of removing that functionality, I am checking if the prefix already exists, and adding it only if it doesn't (otherwise we'd end up with titles that had duplicate prefixes, like View: View: my view name).

While working on the PR, I had do some head-scratching before I realized that the menu module provides menu blocks via its menu_block_info() hook implementation only for custom, user-created menus. The blocks for the 3 default menus that come OOTB with Backdrop are actually provided by system_block_info() instead (defined in menu_list_system_menus() in menu.inc). I tried to add useful inline comments and update various function docblocks in order to make that clear for the next person that gets tripped by that.

The PR currently adds prefixes consistently to the titles of menu blocks, dashboard blocks, and views-provided blocks. I also thought that it'd be useful to prefix block titles provided by non-core modules with the names of these modules ...I've asked feedback on that before I actually do it (could be a follow-up).

I hope that the above is clear and sums things up better.

[jenlampton]

I left an extensive review on the PR. In short -- 0) I love it!

1. The prefixes should be added by the module that adds the blocks (not layout module)
2. Let's not make any API changes
3. Let's not make any unrelated text changes

[klonos]

Thanks for the thorough review @jenlampton 🙏🏼 ...I'm working on things.

1. The prefixes should be added by the module that adds the blocks (not layout module)

I can certainly make that happen within each individual hook_block_info() in each module, and I actually have already considered that. However, that means that the prefix will be added to the block titles everywhere these are being displayed. Are we sure we are OK with that? ...I am not opposing it, and I don't have any specific example where that would look bad - I was just trying to minimize impact and limit the change to only the "Add block" dialog (the layout_block_add_page() function basically).

Before I make the change, can I please get a confirmation on the above consideration @jenlampton?

[klonos]

...another consideration @jenlampton: Take the Dashboard module for example. If we add the prefixes in dashboard_block_info(), then the prefixes are only added for the set of dashboard blocks provided by core. Currently, if any contrib or custom module wants to provide a dashboard block, all they need to do is to implement hook_block_info(), and make sure that they are adding 'required contexts' => array('dashboard') to the array of each block definition/delta that they want to be a dashboard block. By checking the required context somewhere "centrally", like what I've done in the PR, these contrib-provided or custom-provided dashboard blocks are getting the Dashboard: prefix for free (DX++), whereas if we make the change you have requested, we now expect developers to manually be adding the prefix in the 'info' key of the block delta. If they forget to do that, then we'll end up with prefix-less dashboard blocks "scattered" in the "Add block" dialog, depending on alphabetical sorting and what letter the 'info' string begins with. ...which is basically the problem that we are trying to solve here. Right?

Noting that developers could implement hook_block_info_alter() instead, and be adding their dashboard-specific blocks that way, but that's manual work and more code logic that needs to be added, which defeats the purpose of hook_block_info() I believe.

What do you think?

[klonos]

...some other final things to consider as well:

• As it is currently, the Dashboard module is adding the Dashboard - prefix manually to each individual block delta in dashboard_block_info(). I want to eliminate that manual process, as I find it messy. What we could do for the modules that we want prefixes for (Views, Menu, Dashboard) is that in each respective hook_block_info() implementation, before we return the blocks array, we could loop through all blocks in a foreach and add the prefix that way; then return the array. That saves us from the trouble of having to manually add the prefix to any new block added to the hook in the future.

• The above would be fine and all either way we do it (although the manual way is very tedious/annoying), if only it weren't for the following problem: if we add the prefixes in the modules, then the prefixes are being added everywhere, as I pointed out in my previous comment. Currently that would be beneficial for at least two use cases in core:

  • the "Add block" dialog in the layouts
  • the "Blocks" report in /admin/reports/blocks

  The above two places would automatically get the prefixes, and it would be an improvement 👍🏼 (better sorting/grouping of blocks). Here's what would be problematic though: say that you want to provide a select or radio or checkboxes form element, where the values are views-provided blocks specifically. Because the prefixes have already been added to the block titles by the module, every single option would have the prefix Views:, which is redundant/superfluous/annoying if the select was titled "Select views blocks". Now you need to add logic that removes the prefix (DX--). That's why it would be best to be adding the prefixes selectively, only when they are needed or where they are actually adding value.

[klonos]

Apologies for the lengthy comments above. Perhaps it would be good to have a discussion about this in the next UX/design or dev meeting?

In the meantime, I've filed #6541 to consider as an alternative (don't want to derail the discussion here).