Issue #2254: Add prefixes to various block titles in the 'Add block' dialog #4746
Conversation
|
Related to: backdrop/backdrop-issues#2254 |
| } | ||
| // Custom menus are coming from menu_block_info(), while system-defined menus | ||
| // from system_block_info(). | ||
| if ($block_module == 'menu' || ($block_module == 'system' && isset($block_info['type']) && $block_info['type'] == 'menu')) { |
There was a problem hiding this comment.
I'm not sure I like this approach, it's a bit messy. Can we leave this as simply $block_module == 'menu' and then add the same prefix in system module for the menu blocks added?
| // Custom menus are coming from menu_block_info(), while system-defined menus | ||
| // from system_block_info(). | ||
| if ($block_module == 'menu' || ($block_module == 'system' && isset($block_info['type']) && $block_info['type'] == 'menu')) { | ||
| $block_info['info'] = t('Menu: @block_title', array('@block_title' => $block_info['info'])); |
There was a problem hiding this comment.
I like using : more than -. Let's update the Dashboard blocks to use a :: instead ✅
There was a problem hiding this comment.
The PR already does that @jenlampton ...did I miss a spot?
There was a problem hiding this comment.
Oh, I see what you mean: make the change in the dashboard_block_info() hook implementation. Gotcha 👍🏼
| // Make sure to account for blocks intended for use in dashboards that are not | ||
| // necessarily provided by the Dashboard module itself. | ||
| if ($block_module == 'dashboard' || (isset($block_info['required contexts']) && in_array('dashboard', $block_info['required contexts']))) { | ||
| $block_info['info'] = t('Dashboard: @block_title', array('@block_title' => $block_info['info'])); |
There was a problem hiding this comment.
Same goes for dashboard, the prefix needs to be added in Dashboard module.
| @@ -491,6 +491,12 @@ function menu_reset_item($link) { | |||
|
|
|||
| /** | |||
| * Implements hook_block_info(). | |||
| * | |||
| * This returns the blocks for any custom menus created on the site (as opposed | |||
There was a problem hiding this comment.
This is why the Menu module was renamed Menu UI in Drupal 8. It doesn't provide the ability to have menus, only the UI to create them :)
There was a problem hiding this comment.
Yes, I have now realized this 👍🏼
| @@ -2595,6 +2595,12 @@ function system_user_timezone(&$form, &$form_state) { | |||
|
|
|||
| /** | |||
| * Implements hook_block_info(). | |||
| * | |||
| * Note that this also returns the system-defined menu blocks (as opposed to any | |||
There was a problem hiding this comment.
I don't think we need this, as any module can define menu blocks. There's nothing special about System module. (I do like the new docblock for Menu module though, because people will probably look there first for menu blocks.
| // types of blocks provided by the System module. | ||
| // @todo: should this be provided by a system_layout_context_info() | ||
| // implementation instead or be a 'required contexts' key? | ||
| 'type' => 'menu', |
There was a problem hiding this comment.
This would be an API change. Let's not do this. Instead the 'Menu:' prefix in the name.
| @@ -2908,6 +2908,7 @@ class MenuBlockTestCase extends BackdropWebTestCase { | |||
| * Test Menu Block functionality. | |||
| */ | |||
| function doMenuBlockTests($menu_name, $menu_title) { | |||
| $add_menu_block_link = 'Menu: ' . $menu_title; | |||
There was a problem hiding this comment.
we don't need this if Menu: is in the $menu_title; ;)
| @@ -2931,7 +2932,7 @@ class MenuBlockTestCase extends BackdropWebTestCase { | |||
| $this->clickLink(t('Remove'), 1); | |||
| // Add a block to the sidebar (the fourth region). | |||
| $this->clickLink(t('Add block'), 3); | |||
| $this->clickLink($menu_title); | |||
| $this->clickLink($add_menu_block_link); | |||
There was a problem hiding this comment.
we should be able to revert this :)
| @@ -85,7 +85,10 @@ class views_plugin_display_block extends views_plugin_display { | |||
| $info = $this->get_option('block_description'); | |||
| } | |||
| else { | |||
| $info = t('View: @view (@display)', array('@view' => t($this->view->get_human_name()), '@display' => t($this->display->display_title))); | |||
| $info = t('View: @view (display: @display)', array( | |||
There was a problem hiding this comment.
love it! The prefix comes from views module ✅
There was a problem hiding this comment.
I didn't actually add the View: prefix here - I only added the display: prefix. The View: prefix was actually added already, which is actually causing problems with duplicated View: View: prefixes.
Because this is only added to certain views blocks (if there is no block_description added in the block display), you can end up with some views-provided blocks that have the prefix, and some that don't 😞 ...I had to add logic in _layout_block_add_block_title_prefix() to ensure that there aren't duplicated prefixes, while at the same time making sure that all views-provide blocks had the Views: prefix in the "Add block" dialog (so that they aren't "scattered" by the alphabetical sorting).
There was a problem hiding this comment.
...the reason why I added the display: prefix by the way is that we anded up with things like this (that is before any changes in this PR, and that is only one of the examples that I randomly found):
View: Layout Context Test (User account) (Block)
In the above title, you don't know what the strings in the brackets are. Which one is the name of the display? My change adds the display: prefix, so you get this instead:
View: Layout Context Test (User account) (display: Block)
|
Tugboat has finished building a preview for this pull request! Website: https://pr4746-l8prdkpjjauybkjmhzmibcw6k6b91dv8.tugboatqa.com/ This preview will automatically expire on the 13th of July, 2024. |
Fixes backdrop/backdrop-issues#2254