Advertise the block region demonstration in a more prominent way

Note: currently the block region demonstration link is displayed only if help module is enabled. see #2513580: [Regression] Demonstrate block regions link in Block layout should be visible even if help module is not enabled

Thanks for sharing this, the obvious solutions to these are clear - but I am afraid we might lose some of the usability of this page if we start adding links all over the place.

In this case, I think we also want to explore the following directions:
- Placing it more prominently in the text (e.g. by placing it in between two paragraphs)
- Placing it just above the table, or with the first region header (avoiding duplication, but still showing it)

I think we want to avoid abusing the action pattern. The idea of the action pattern that it links you to creating things, in this case it doesn't - which will break people's mental model about action buttons. We also generally avoid having two primary action buttons next to each other, we've learned from previous tests that having two action buttons at the top creates a high cognitive burden to find out which one is the prominent one for adding things (and which one is the 99% adding usecase).

I hope we can explore this a little more.

What about removing half of the help text there, so that its one line ending with demo? Most of the text there is really unneeded.

Why separate the demo from the block list? Replace the block listing with the demo, to get closer to the D8 layout initiative :)

The demo preview is much more intuitive than the boring block list.

Yep, that is definitely something I would love to see us to explore in 8.1.x+. When we're 25 issues away from RC though, we need something a bit more "quick and dirty" for 8.0.0.

This is by no means scientific, but I had to scan around that screenshot for a good 10+ seconds before I saw the link. :\ It's very difficult to find it with the BIG ASS BUTTON being so prominent.

Of course, the *real* way to solve this is to make the block demonstrations page the block placement UI. Stick add buttons in the regions everywhere, show a labeled box for each block. Drag and drop. Done. However, that's obviously a bit more radical proposal. ;)

You know... I should probably just go ahead and link this here too:

@webchick Fortune favors the bold. :D

Let me work on the text here a little bit

As per part 2 of the solution in #2512456-210, here is a basic patch with the demonstration link as the action button.

I'm posting it in this issue instead of #2513580: [Regression] Demonstrate block regions link in Block layout should be visible even if help module is not enabled because it's more inline with this one, and I doubt the "radical solution" here would even go into 8.0.x anyways.

I'm posting it in this issue instead of #2513580: [Regression] Demonstrate block regions link in Block layout should be visible even if help module is not enabled because it's more inline with this one

so should we mark as duplicate and close #2513580: [Regression] Demonstrate block regions link in Block layout should be visible even if help module is not enabled?

attached patch should fix the fail.

@willzyx Dunno. I suppose, if people really want to take the other route, then we can update the IS and move the patch over to the other one?

I'm really curious about this failing test though. I mean the text was already being sanitized wasn't it? Maybe it's the assertion that needs to change instead? Although, if that were true, how does the text in the tab link test pass? So confused...

Increasing this one to major, since #2513580: [Regression] Demonstrate block regions link in Block layout should be visible even if help module is not enabled (which will get fixed as a result of this one) is major.

Also looks like we have tests now.

Hmm, since the other issue was a regression, we may want to change testBlockDemoUiPage() to make sure that the link is still there without the help module/block being available.

Added test for the regression.

I'm not a fan of using a local action for this button. These buttons are used to add new things to the system, which is not the behaviour this new button triggers. This button also draws the user's attention away from the primary action of the page (placing blocks).

In my opinion we should use a regular button as shown below.

+++ b/core/modules/block/block.module
@@ -40,13 +40,11 @@ function block_help($route_name, RouteMatchInterface $route_match) {
-    $demo_theme = $route_match->getParameter('theme') ?: \Drupal::config('system.theme')->get('default');
-    $themes = \Drupal::service('theme_handler')->listInfo();
...
-    $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>';

Only these lines have to be removed, the other changes are out of scope. (even though i agree the overall readability of the method is improved)

I disagree, also local actions provide this extremely useful mechanism, otherwise we'd be back to putting moduleExists checks directly into block code, or hook_form_alter-ing.

Changing the if to a case is a noob out-of-scope change, but whatever.

'm not a fan of using a local action for this button. These buttons are used to add new things to the system, which is not the behaviour this new button triggers. This button also draws the user's attention away from the primary action of the page (placing blocks).

This is a styling problem, not a conceptual problem with action links. Follow up created: #2537840: Make the '+' icon for action buttons optional

Double comment

Just like the double comment above, there were two issues opened. #2537840: Make the '+' icon for action buttons optional is the real one.

@lewisnyman This is slightly abusing the pattern. Because its not about "adding" here, its purpose is actually quite unique and not found elsewhere in core.

I wonder if removing the + is really the solution here, don't we want a different "prominent" pattern that doesn't contain the idea of adding.

@Bojhan These components are called "action links" not "adding links" though. I think in contrib these are used for many other things in different contexts. My understanding is "an important action within the current page context".

Bojhan +1,

Even if we remove the + people will still expect actions that add things there, since that's what they are taught trough out core.

LewisNyman +1,

I too have always thought of them as general actions. To me, the '+' is just a huge, incorrect assumption made by the theme, and as such #2537840: Make the '+' icon for action buttons optional seems reasonable.

Even if we went with a different prominent pattern, wouldn't it make sense to keep the action link, and just make the theme function it uses override-able?

Fix for changes made in #2338081: Local Tasks, Actions, and Contextual links mark strings from derivatives (or alter hooks) as safe and translated.

Assigning to @webchick since she's already commented on the issue and was as the usability testing.

Was failing on MigrateBlockedIPsTest, setting back to rtbc per #26

So I was the one who originally asked for this, so conceptually have no problems committing. :)

But it's interesting, because over at #507488: Convert page elements (local tasks, actions) into blocks I was asked to do research to determine what to call that blue button on the top of the page. In researching that, I looked up the User interface standard for that pattern, which is at https://www.drupal.org/node/1089894. (Action links)

That page states (emphasis mine):

A create action related to the content or data on the page. [...]

The links have a distinct position on the page and are preceded by a plus sign indicating an add action.

Usage

• Add an item like the ones listed on the page.
• Add an item related to the listed data.

If you click around core's UI, this is indeed how the pattern is used consistently atm. (I know contrib does some different things; for example @Berdir showed me a screenshot of SimpleNews that would probably make @Bojhan cry. :D)

So this would clearly be deviating from the existing UI pattern, and doing so in an extremely prominent place. (This is brought up by @Bojhan in #4 and @legolasbo in #25.) Based on the current usage of action links, #2537840: Make the '+' icon for action buttons optional would basically be won't-fix, since the plus sign is intended to cause awkward discomfort if this pattern is mis-used for other things.

I agree the long-term solution for this is either #2537840: Make the '+' icon for action buttons optional (meaning discuss expanding the existing pattern to apply to actions other than create), or another pattern in Seven for a prominent action (suggested by @Bojhan in #30).

In the short-term though, and given this is a patch that would have major impact on the usability of this form (in addition to fixing a bug with the dependency on Help module), I wonder if we can do what @legolasbo suggested in #25 and just do a normal button for now:

It initially looks like "ACK! WEIRD!" but in some ways, that's good. This is a weird interaction, and it's what people are desperately searching for when they get to this page, so we should call attention to it.

Thoughts?

Since we're talking about a one-off solution which looks:

"ACK! WEIRD!" but in some ways, that's good.

Why don't we position the block demonstration in the table header? We've already added buttons to the table itself, also adding the demonstration button to the header feels more logical to me than just a random button. This way it is incorporated in the actual form widget it is supposed to clarify and more in line with the direction we've taken so far.

Hm. My initial reaction is "Um, ick! :D" but let's see what others think.

It's a fair point that it is contextually related to the stuff the table is doing. The thing is though, the other buttons in table headers ("Place block") are a "do things right here" type of button. This is a "do a completely different thing, for the whole page" so it makes sense IMO for it to be visually separated. Additionally it is a primary action of this page to preview what the regions are going to look like (at least for newbies / experienced people new to a particular theme), so putting it in the same place as other primary action buttons (aka the screenshots in #25/#40) I think is preferable.

My initial reaction is "Um, ick! :D"

That was also my initial reaction when creating the mockup ;) Let's discuss which is feels 'better', "Um, ick!" or "ACK! WEIRD!".

This is a "do a completely different thing, for the whole page" so it makes sense IMO for it to be visually separated. Additionally it is a primary action of this page to preview what the regions are going to look like (at least for newbies / experienced people new to a particular theme), so putting it in the same place as other primary action buttons (aka the screenshots in #25/#40) I think is preferable.

I think you are right about this.

So far none of the options feels like a great solution for the problem. Perhaps @Bojhan could share his two cents? He's usually full of good ideas :)

Thanks for picking this up, it is indeed unfavorable to abuse the pattern like this. We really want to establish the idea that "blue on top" is adding to "the list below".

I think webchick proposal here should suffice, I do not want us to do more unconventional things like adding it to a table header. While ideally we have a good pattern for these type of demo links, we do not yet. And inventing one in this issue, might be a bit much. Lets try to bootstrap this and go with Angie's proposal.

Yay, thanks! (To be clear, that was @legolasbo's proposal, I just +1ed it. :))

All right I'll play ball. Here's a fresh patch, so no interdiff. I even removed that "noob out-of-scope change".

I was able to replace ThemeManager with ThemeHandler though, we'll see if the testbot agrees...

@todo Needing to change the status to "Needs review" after uploading a patch is a usability problem...

Just to play devil's advocate for a bit though, the UI standards page that @webchick linked also clearly states the original problem as: "User wants easy access to an action related to the data or content of the page." This means that the solution mentioned there only applies to a subset of the issue. It may not be ideal to use our original solution, but it is worth considering how likely contrib is going to use action links for other purposes and whether it is worth the effort to solve the '+' issue. I personally find it highly unlikely that this problem will not come up again in core or contrib. There are already several people in this issue alone that think local actions should apply to the entire problem space.

Yes, I agree we need a follow-up issue to explore adding styling for a "prominent action that is not 'add'" to Seven and/or do #2537840: Make the '+' icon for action buttons optional. But I'd rather not hold this issue up on it, since it's a legit bug fix and a really bad UX problem.

But I'd rather not hold this issue up on it, since it's a legit bug fix and a really bad UX problem.

Well looks like the new patch is green, so as soon as someone reviews it, you'll get to have your pick. :)

+++ b/core/modules/block/src/BlockListBuilder.php
@@ -317,7 +327,7 @@ protected function buildBlocksForm() {
   protected function getThemeName() {
     // If no theme was specified, use the current theme.
...
-      $this->theme = $this->themeManager->getActiveTheme()->getName();
+      $this->theme = $this->themeHandler->getDefault();

This comment no longer reflects reality. Also we use this method in a couple places, is this change really desired in all cases?

Huh? When the route doesn't contain the theme, the render method sets the theme to NULL, so unless I'm missing something else the comment seems fine.

Additionally, since setting the theme only has to be done for the default route, there is only one possible value which it should be. The question is if ThemeHandler::getDefault() returns that value. However, if you look at ThemeLocalTask::getDerivativeDefinitions() the system default theme is specifically set as the default task, which means that it is the only one that should be used. The theme handler returns that value, and the implementation of a current active theme and/or negotiation in a ThemeManager ends up being irrelevant in this case.

Block place module solves this problem.