Add a link to the Block Layout page on the Appearance page

"Did you mean to...?" is very unusual wording for help text. Normally that kind of wording would be used in an error message or similar.

Also I'm pretty sure "!blocks" should be "@blocks" these days. And what happens to this link if the Block module is disabled?

And is help text even the right place to put this link? I think it might be better to have a "Block layout" link underneath each theme (i.e., next to the existing links such as "Settings" and "Set as default").

Also linking to a previous issue in which this usability issue was raised (nice to know that testing confirms it!) and which proposed a different, although more involved, solution.

Adding the backport tag since this page hasn't really changed much since Drupal 7, so I assume the testing results are valid there too.

@David Typically we do link to connecting concepts via the description on top. Not in-line with each theme, as that would create a lot of duplication.

What we could do is change the text to be more in line with other places (e.g. "Place blocks for each theme on the block layout page").

@David_Rothstein If the block module is disabled, this happens: #2498675: Linking to a non-existent route should not throw an exception.

@Bojhan so there wouldn't be duplication on the page (since each link would go to the block configuration page for that particular theme). There would be duplication with the other path for getting to these pages (via Admin >> Structure). Depending on how strong the usability testing results were (i.e. what percentage of people went to "Appearance" first?) I honestly wonder if it would make sense to just remove block configuration from the Structure menu and have Appearance be the one and only place you go to do it? (It is not a wrong instinct to go there; in many ways it makes more sense than Structure.) That would be a larger change, though.

For help text, "Place blocks for each theme on the block layout page" sounds pretty reasonable (and there would need to be a moduleExists check to not print this at all if the Block module is disabled). The question is how much it actually solves this, since apparently very few people actually read help text :)

@David_Rothstein I just don't want to overreact to pogo-sticking. We can consider an IA change, but just not this late in the release cycle I think.

@David_Rothstein I was also thinking the same thing about moving blocks to Appearance while working on #2512456. It would also dovetail nicely if something like this were to happen.

@MattA I totally agree with you; I made a similar suggestion during the study analysis. Implement your mockup as a replacement blocks layout UI, then merge the Block layout UI and the Appearance UI.

Ironically the "Demonstrate block regions" screen is the only place to see a "theme preview". So it would be a major enhancement to the Appearance/theme management workflow.

This hybrid of Appearance + Block Layout could live at its own top-level menu item.

Ironically the "Demonstrate block regions" screen is the only place to see a "theme preview".

You know, I never really thought about it until you said it. It is so true that it's actually sad.

Speaking of top-level menus, it amazes me how "Appearance" is even up there right now. You set up a site, you set the theme and theme settings exactly once, and then you never click on it again! At least with blocks, it would you know... be a menu, instead of a mislabeled "theme" button.

Oh look, another usability issue that came up during testing for D7 at UMN 2011. Seems this might be a duplicate of #1188790: Add "Manage blocks" link on Themes admin page which was bumped to D8. It even contains a similar screenshot:

Really wish that usability issues would stop just getting kicked down the road...

Great find @MattA. I've added it to the related issues.

From #4. Did we come to any agreement about no putting this in the help text?

+++ b/core/modules/system/system.module
@@ -104,6 +104,9 @@ function system_help($route_name, RouteMatchInterface $route_match) {
+      if (\Drupal::moduleHandler()->moduleExists('block')) {
+        $output .= '<p>' . t('Do you want to <a href="@blocks">insert a block</a> into the layout?', array('@blocks' => \Drupal::url('block.admin_display'))) . '</p>';
+      }

Why can't block_help do this?

The only difference I have noticed so far, is the change in the order of those 2 lines. If the order is not important, I could go ahead and put it in the block module.

As someone rather new to drupal i can see the use in this as i have had to issue before.

The patch appears to be working as it should be to me. :)

I would attach images to show that it works but dcmul already out an image showing it, i also have very little php knowledge so i dont really understand how your code gets this to work so i will leave it as to be reviewed still.

Good work :)

putting the code to the block module.

Note: I haven't looked at the other patches, but if you want this to live in the System module you need to use a module exists check to make sure that the Block module is enabled when you make the link.

This looks good to me, I guess there is no need to check if block module exists when the code is inside block module. Also simplifies the switch + if combo into one switch.

1. The text still needs to be changed to something more like #7.
2. The issue dcmul noted in #18 is still present - the paragraphs are displayed in the wrong order. Maybe hook_module_implements_alter() could be used to change the order... but honestly, I think it's better to just keep this in the System module with a module exists check. It's a lot simpler that way, and more in line with how things are normally done (all the relevant help text for the page in the same place in the code). I'm not even sure this new sentence should be a separate paragraph anyway(?) - and if not it would definitely need to be in the System module.

Nice find, MattA, on the related issue (#1188790: Add "Manage blocks" link on Themes admin page)! Given that issue already exists for a more comprehensive solution, I definitely agree with just doing the more limited solution here of changing the help text... especially since it's backportable.

1. Updated help text.
2. Moved help text related description to system module's hook_help().

Thanks @mohit_aghera.

+++ b/core/modules/system/system.module
@@ -104,6 +104,7 @@ function system_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<p>' . t('You can place blocks for each theme on the <a href="@blocks">block layout</a> page.', array('@blocks' => \Drupal::url('block.admin_display'))) . '</p>';

You need to wrap this in a if block to see if block module is enabled. See #17.

Thanks @hussainweb.
Updated patch as per your suggestion.

Still looking good, after the requested changes.

+++ b/core/modules/block/block.module
@@ -39,13 +39,13 @@ function block_help($route_name, RouteMatchInterface $route_match) {
-  }
-  if ($route_name == 'block.admin_display' || $route_name == 'block.admin_display_theme') {
-    $demo_theme = $route_match->getParameter('theme') ?: \Drupal::config('system.theme')->get('default');
-    $themes = \Drupal::service('theme_handler')->listInfo();
-    $output = '<p>' . t('This page provides a drag-and-drop interface for adding a block to a region, and for controlling the order of blocks within regions. To add a block to a region, or to configure its specific title and visibility settings, click the block title under <em>Place blocks</em>. Since not all themes implement the same regions, or display regions in the same way, blocks are positioned on a per-theme basis. Remember that your changes will not be saved until you click the <em>Save blocks</em> button at the bottom of the page.') . '</p>';
-    $output .= '<p>' . \Drupal::l(t('Demonstrate block regions (@theme)', array('@theme' => $themes[$demo_theme]->info['name'])), new Url('block.admin_demo', array('theme' => $demo_theme))) . '</p>';
-    return $output;
+    case 'block.admin_display':
+    case 'block.admin_display_theme':
+      $demo_theme = $route_match->getParameter('theme') ?: \Drupal::config('system.theme')->get('default');
+      $themes = \Drupal::service('theme_handler')->listInfo();
+      $output = '<p>' . t('This page provides a drag-and-drop interface for adding a block to a region, and for controlling the order of blocks within regions. To add a block to a region, or to configure its specific title and visibility settings, click the block title under <em>Place blocks</em>. Since not all themes implement the same regions, or display regions in the same way, blocks are positioned on a per-theme basis. Remember that your changes will not be saved until you click the <em>Save blocks</em> button at the bottom of the page.') . '</p>';
+      $output .= '<p>' . \Drupal::l(t('Demonstrate block regions (@theme)', array('@theme' => $themes[$demo_theme]->info['name'])), new Url('block.admin_demo', array('theme' => $demo_theme))) . '</p>';
+      return $output;
   }
 }

I'm not necessarily against making this small code refactor, but it is technically out of scope for this issue.

The other part looks fine.

I agree it is out of scope. Removing it entirely.

Thanks for taking that out!

+++ b/core/modules/system/system.module
@@ -104,6 +104,9 @@ function system_help($route_name, RouteMatchInterface $route_match) {
+        $output .= '<p>' . t('You can place blocks for each theme on the <a href="@blocks">block layout</a> page.', array('@blocks' => \Drupal::url('block.admin_display'))) . '</p>';

Minor nitpick: The page is technically called "Block layout" (note the caps). Sorry, didn't notice that before.

Fixing component

Patch in #34 looks good. #35 confirmed that tests are indeed failing.

I don't think I fully understand what #35 is all about. Otherwise #34 seems fine

#35 was to confirm that the tests, without the fix, were actually covering the change. #34 is fine to be committed.

#34 needs a reroll.

It seems this got commited as part of https://www.drupal.org/commitlog/commit/2/c8702065a19f2431ff172691c15621... . That way test only patch should pass :P

Oh the test was also commited which causes nice fatal error.

Thanks @lauriii. Fixed all of this and attributed all the people.

Committed 46d59d6 and pushed to 8.0.x. Thanks!