Problem/Motivation

yoroy should be credited in any commit for this issue.

Spinning off a more actionable part of #1957276: Let users set the block instance title for Views blocks in the Block UI since that issue is going to be a lot harder to solve.

  • Views blocks currently do not provide the user with any indication of what their title is.
  • Views blocks do not provide a default machine name.
  • Views blocks do not indicate that the primary way to set the title is via Views.
  • The logic that generates Views titles is dynamic.
  • There are usability concerns with potentially allowing the user to override the View title through the Block UI when the user normally sets it in Views.
  • The Block system having a say in what the view title is complicates things architecturally.
  • #1957214: Title setting in views UI does not indicate when the title might be overridden is also going to be tough to solve in a robust and maintainable way.

configure_views_block.png

Proposed resolution

Let's start with something we can all at least live with.

  • Provide a message to the user in the Block UI that the title is set with Views, and provide a link to edit the view.
  • Indicate that the title is dynamic.
  • Provide a default label and machine name.
  • Continue to allow the "Display title" checkbox on the block instance to control title visibility. (Views already respects it in HEAD.)
  • Either:
    1. Disable the label field and set it to the view title, or to the view name if there is no view title. (Followup: Should views use the same "title required but can be hidden" logic blocks and panels do?) Or:
    2. Provide a label field that defaults to the above. Do not disable it, but also add a message indicating that this field does not actually control the title the view displays, but only provides an administrative label.

    There are problems with both 1 and 2, but 2 seemed more confusing overall, so I went with 1 in the attached patch.

views_block_titles.png

Remaining tasks

  • Decide if this approach is an acceptable compromise. (It's certainly better than what we have.)
  • Discuss the default value for the block title field, and whether it should be disabled or not.
  • This definitely deserves good test coverage for all the various combinations of view and block configurations.
CommentFileSizeAuthor
#14 what_the_no_results_block_looks_like.png11.78 KBxjm
#14 what_the_arg_block_looks_like.png11.01 KBxjm
#14 executing_the_display.png19.7 KBxjm
#14 block_with_default_arg_executed.png12.55 KBxjm
#14 not_executing_the_display.png12.97 KBxjm
#13 views-block-1967460-13.patch2.01 KBxjm
#13 interdiff-13.txt901 bytesxjm
#9 views-block-1967460-8.patch1.27 KBxjm
#9 1967460-8-interdiff.txt669 bytesxjm
#8 views-block-1967460-7.patch1.32 KBxjm
#8 1967460-7-interdiff.txt1.43 KBxjm
views-block-title-take-2.patch1.4 KBxjm
views_block_titles.png15.38 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -64,9 +64,25 @@ public function blockAccess() {
+    if (!empty($title)) {
+      $form['label']['#disabled'] = TRUE;
+    }

The empty() check is a leftover from earlier logic. I'll remove it in the next reroll.

+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -64,9 +64,25 @@ public function blockAccess() {
+            '@url' => url($this->view->storage->uri()['path'] . '/edit/' . $this->displayID),

I dunno if there is a better way to get the edit URL for the display than concatenating like this?

+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -64,9 +64,25 @@ public function blockAccess() {
+            '@viewname' => ($view_name),

Silly parens are silly.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -64,9 +64,25 @@ public function blockAccess() {
+        t('<a href="@url">Edit @viewname view</a>.',
+          array(
+            '@url' => url($this->view->storage->uri()['path'] . '/edit/' . $this->displayID),
+            '@viewname' => ($view_name),
+          )

This could actually probably just be an l(), since the whole string is the link value, not just part of it.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Just wondering, what does this display when I have a argument in my title? Should this not use tokens?

Status: Needs review » Needs work

The last submitted patch, views-block-title-take-2.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Just wondering, what does this display when I have a argument in my title? Should this not use tokens?

Turns out, this is actually really complicated to calculate, because any number of things can override it. See in #1957214: Title setting in views UI does not indicate when the title might be overridden. In the meanwhile, as a first step, this will just use the title the View returns.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -64,9 +64,25 @@ public function blockAccess() {
+            '@url' => url($this->view->storage->uri()['path'] . '/edit/' . $this->displayID),

Apparently this doesn't work in 5.3. :) Fixing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -64,9 +64,25 @@ public function blockAccess() {
+    $this->view->executeDisplay($this->displayID);
+    $title = $this->view->getTitle();

I guess for everything which needs dynamic stuff (like arguments etc.) executing the view certainly does not help. I guess we should just call getTitle() (withote executing it) and let it fetch the Title configured in the display. This also improves performance.

+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -64,9 +64,25 @@ public function blockAccess() {
+            '@url' => url($this->view->storage->uri()['path'] . '/edit/' . $this->displayID),

Directly using the function result is just possible in php 5.4: http://www.php.net/manual/en/migration54.new-features.php

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Needs review
FileSize
1967460-7-interdiff.txt1.43 KB
views-block-1967460-7.patch1.32 KB
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
1967460-8-interdiff.txt669 bytes
views-block-1967460-8.patch1.27 KB

Oops, crossposted. @dawehner, is this all you mean?

Status: Needs review » Needs work
Issue tags: -Usability, -VDC, -Block plugins

The last submitted patch, views-block-1967460-8.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review

#9: views-block-1967460-8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +VDC, +Block plugins

The last submitted patch, views-block-1967460-8.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Needs review
FileSize
interdiff-13.txt901 bytes
views-block-1967460-13.patch2.01 KB

Right, that test. Writing actual tests later. We need a lot more than the "is disabled" equivalent of this one assertion now. :)

Would love some reviews on the functionality (both code and user interaction) before I go crazy codifying it in SimpleTest, also. I'm not looking for perfect; I'm looking for "make a major UX regression into at least just a minor one."

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
not_executing_the_display.png12.97 KB
block_with_default_arg_executed.png12.55 KB
executing_the_display.png19.7 KB
what_the_arg_block_looks_like.png11.01 KB
what_the_no_results_block_looks_like.png11.78 KB

So here's the implications of executing the display or not. I'd actually prefer to execute it, to get a more meaningful title more of the time. We can always remove it if we get some fancy title pattern logic later.

We have these two blocks with dynamic titles: The first one has the title set as a no results behavior; the second sets it based on a default argument.
what_the_no_results_block_looks_like.png

what_the_arg_block_looks_like.png

Here's what placing a new instance of these blocks looks like if there's no site content/other argument if the display is executed:
executing_the_display.png

block_with_default_arg_executed.png

Here's what they look like if the display is not executed:

not_executing_the_display.png

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

@timplunkett and I just came up with yet another way of solving this. Another darn issue incoming shortly. :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Alternate solution: #1968296: Block title != view title

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Needs review » Needs work

I will check out the other issue, but I would say this in it's current form is a no go.

This would set the machine name of a block based on the current context - which seems bad. So if the view was currently empty your machine name could be 'Sorry, no results' from a title override or 'User %1' from an argument but obviously that is a very dynamic thing.

Are we thinking the other issue is a better route forward now?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Closed (won't fix)

I spent an hour on the phone with @webchick and @effulgentsia talking about this problem yesterday; just haven't gotten around to posting annoying iteration #5 and #6 yet. This one is wontfix.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Can't we instead of open new issues and then close and then open another one, just think about a proper solution first, in the open.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Sorry, never saw your reply @dawehner -- we are back to #1957276: Let users set the block instance title for Views blocks in the Block UI.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.