Problem/Motivation

Since the hook_help text for the Blocks module was written, its Administration page has been changed. The "Place Blocks list" has been removed, and replaced by a button per region, but the help text was not updated.

Proposed resolution

Update the hook_help text.
Check the UI text on the Blocks administration page and update it if necessary as well.

Remaining tasks

Update the hook_help text, and possibly the UI text as well

User interface changes

This is a UI text change.

API changes

None.

Data model changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because the User interface was changed, but the help text was not changed accordingly
Issue priority Minor
Unfrozen changes Unfrozen because it only changes strings in the hook_help for the block module
Disruption None
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ifrik created an issue. See original summary.

marjadoedens’s picture

I'll be working on this today.

marjadoedens’s picture

Status: Active » Needs review
FileSize
1.56 KB

I updated the help-text of the first use because the interface was changed.

pguillard’s picture

Status: Needs review » Reviewed & tested by the community

According to me, this is RTBC

ifrik’s picture

Issue summary: View changes

Thanks a lot, and congratulations for making your first patch!

I've added the beta-evaluation, so it just needs waiting for the test to finish.

yoroy’s picture

Status: Reviewed & tested by the community » Needs review

This change is good. If changing the UI text on top of /admin/structure/block is also in scope of this issue then we still have work, because there it still refers to the right sidebar that is now removed.

ifrik’s picture

Issue summary: View changes
Status: Needs review » Needs work

... that should teach me to read my own issue descriptions properly in future....

I'm adding that to the patch now, because as far a as I know the mdoedens is not around during the extended sprint.

ifrik’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
1.55 KB

In fact the UI text on that page had quite a few errors in it by now.

I've restricted the text to issues: that you need to place the blocks per theme, and that you need to click save at the bottom, because these are not obvious when you look at the table.
There isn't much explanation that could be added for the "place blocks" or "configure" links that users wouldn't see when they click on either link, so I would rather not clutter up the explanation text with any of that.

MattA’s picture

1a.

+++ b/core/modules/block/block.module
@@ -43,7 +43,7 @@ function block_help($route_name, RouteMatchInterface $route_match) {
+    $output = '<p>' . t('You need to place the blocks specifically for each theme because not all themes implement or display the same regions in the same way. Changes will not be saved until you click <em>Save blocks</em> at the bottom of the page.') . '</p>';

Hmm, technically the tabs at the top of the page already imply that these are per-theme settings. If that is clear to users, the part explaining how themes implement regions kind of becomes irrelevant too. So I wonder if the first sentence could be removed (or just emphasize the per-theme-ness)?

1b.

Of course if that sentence gets removed, the phasing of the next one becomes a little weird. A quick fix would be to just use what it was supposed to be from the patch/botched review that created this issue:
After moving blocks, remember that your changes will not be saved until you click the <em>Save blocks</em> button at the bottom of the page.

2.

+++ b/core/modules/block/block.module
@@ -43,7 +43,7 @@ function block_help($route_name, RouteMatchInterface $route_match) {
     $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>';

Not relevant to this issue, but thought I'd take the time to remind people that this is a regression (which should be fixed by #2514150: Advertise the block region demonstration in a more prominent way).

ifrik’s picture

There is nothing on the page that alerts users that the blocks need to be set per region - the tabs only give the names of the enabled themes, and the place blocks pop-up does not have any information about in which theme you are placing the block at all.

So the chance of users missing the fact that blocks are placed per region are really high. Therefore we really need to keep that information.

The reminder to save changes is there because even though there is a message displayed at the top of the page, a user can miss the save button as well as the message when they move a block somewhere in the middle of the page.

MattA’s picture

FileSize
77.15 KB

Implied settings.
I really wouldn't say that this page has nothing on it informing users about what is going on. Obviously, having the UI heavily imply something isn't as great as making it explicit to the user (like this), but eventually we have to give them some credit. At the end of the day though, we'd really need a usability test to determine the probability of them misunderstanding the concepts on the page and if help information could even fix it.

Also, I never said anything about removing the save button sentence. Only that it's phasing would be off if 1a happened. That thing is so far off the screen, it might as well be in Antarctica...

jhodgdon’s picture

In the latest patch:

You need to place the blocks specifically for each theme because not all themes implement or display the same regions in the same way.

This seems unnecessarily verbose... if (given the above discussion) we do need to say something about this, how about if we just change it to say something like:

Block placement is specific to each theme on your site.

ifrik’s picture

Thanks, having that sentence shorter is a good option.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I think that the patch is very sensible. It removes the part of the page-specific help that was telling how to use the UI, which is something we are not supposed to do, and leaves in a concise 2 sentences for 2 things you might not notice. Tenatively RTBC...

MattA’s picture

Satisfies both 1a and 1b of #9, so +1 RTBC pending testbot results.

yoroy’s picture

Status: Reviewed & tested by the community » Needs review

This is looking good. I have one nitpick:

…by using the drop-down Region list

sounds a bit weird to me. Maybe this is a bit simpler:

…by using the Region drop-down,

MattA’s picture

Hmm, I guess that does sound a little weird. How about "the Region drop-down list" instead though. That is what is used in other parts of core like the help text for the Action, Editor, Field UI, and Language modules. Also, "drop-down" is technically a programmer-specific term for a UI element which normal people may not understand, so at least those people can clue in with "list".

MattA’s picture

Oops. Too early in the morning for me. Just did a previous patch number + 1 instead of looking at comment number there. Oh well...

yoroy’s picture

Yep, doesn't hurt to keep the 'list' keyword in there. Thanks!

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

It seems that Yoroy likes this. I like it too. Let's do it.

xjm’s picture

Status: Reviewed & tested by the community » Fixed
  1. +++ b/core/modules/block/block.module
    @@ -26,7 +26,7 @@ function block_help($route_name, RouteMatchInterface $route_match) {
    -      $output .= '<dd>' . t('You can place a new block by clicking on its title in the <em>Place blocks</em> list on the <a href=":blocks">Block layout page</a> and choosing a region to place it in. Once a block is placed, it can be moved to a different region by dragging or using the drop-down <em>Region</em> list, and then clicking <em>Save blocks</em>.', array(':blocks' => \Drupal::url('block.admin_display'))) . '</dd>';
    +      $output .= '<dd>' . t('You can place a new block in a region by selecting <em>Place block</em> on the <a href=":blocks">Block layout page</a>. Once a block is placed, it can be moved to a different region by drag-and-drop or by using the <em>Region</em> drop-down list, and then clicking <em>Save blocks</em>.', array(':blocks' => \Drupal::url('block.admin_display'))) . '</dd>';
    

    I tested manually and confirmed that this describes the new behavior.

  2. +++ b/core/modules/block/block.module
    @@ -43,7 +43,7 @@ function block_help($route_name, RouteMatchInterface $route_match) {
    -    $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>' . t('Block placement is specific to each theme on your site. Changes will not be saved until you click <em>Save blocks</em> at the bottom of the page.') . '</p>';
    

    This is much more readable now; nice work!

Committed and pushed to 8.0.x. Thanks!

  • xjm committed 6f4afea on 8.0.x
    Issue #2572513 by ifrik, MattA, mdoedens, yoroy, jhodgdon: Update the...

Status: Fixed » Closed (fixed)

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