Problem/Motivation

Note: #1967460: Display the block title and populate the machine name for views blocks tries to provide an actionable first step for this issue.

From #1938062: Convert the recent_comments block to a view. Right now, Views goes out of its way to override the block form and prevent the user from creating a Views block instance title different from the View display title. This results in this UX fail:

configure_views_block.png

It basically prevents the UX improvement of #1875260: Make the block title required and allow it to be hidden from taking effect, regresses from D7 in that you can no longer set a block instance title different from the View title, and confuses users by being different from every other block.

Filing as major because this is considered a blocker for all Views block conversions.

Proposed resolution

Allow users to override the View title on the block configuration form.

Remaining tasks

First, see #1967460: Display the block title and populate the machine name for views blocks.

  1. We need to find a happy medium between letting content authors configure their block instances, and communicating to users that Views controls the block title dynamically. @yoroy proposed this interaction to resolve the issue:
    http://dl.dropbox.com/u/538835/block-title-overrides.m4v

  2. There is also a concern that site builders will no longer be able to control the titles for these blocks once the block is allowed to override it. In #3 (and related issue #1957346: Add some settings on the block display to allow overrides on the block instance configuration), @dawehner suggested that we might want to allow Views admins to restrict when these settings can be overridden on a per-block level.

  3. Finally, we need to find a way to describe what the view title is for the user, since Views sets it dynamically based on conditions like whether the view has no results or what arguments are supplied to it. #1957214: Title setting in views UI does not indicate when the title might be overridden also is exploring this issue within the Views UI itself.

CommentFileSizeAuthor
#87 Screen Shot 2013-12-05 at 7.02.05 PM.png70.19 KBwebchick
#87 Screen Shot 2013-12-05 at 6.55.39 PM.png59.35 KBwebchick
#87 Screen Shot 2013-12-05 at 6.45.13 PM.png57.1 KBwebchick
#87 Screen Shot 2013-12-05 at 6.43.59 PM.png56.57 KBwebchick
#87 Screen Shot 2013-12-05 at 6.42.37 PM.png59.28 KBwebchick
#86 vdc-1957276.patch11.83 KBdawehner
#86 interdiff.txt1.83 KBdawehner
#80 vdc-1957276.patch10 KBdawehner
#80 interdiff.txt1.34 KBdawehner
#79 vdc-1957276.patch9.96 KBdawehner
#79 interdiff.txt1.3 KBdawehner
#77 vdc-1957276.patch9.96 KBdawehner
#77 interdiff.txt1.39 KBdawehner
#72 ui.png5.43 KBdawehner
#71 vdc-1957276.patch9.89 KBdawehner
#71 interdiff.txt3.73 KBdawehner
#66 vdc-1957276.patch7.91 KBdawehner
#66 interdiff.txt978 bytesdawehner
#62 vdc-1957276.patch7.92 KBdawehner
#62 interdiff.txt561 bytesdawehner
#57 vdc-1957276.patch7.87 KBdawehner
#57 interdiff.txt1.3 KBdawehner
#55 interdiff.txt2.39 KBdawehner
#55 vdc-1957276.patch7.74 KBdawehner
#53 vdc-1957276.patch6.16 KBdawehner
#53 interdiff.txt4.12 KBdawehner
#49 interdiff.txt851 bytesdawehner
#49 vdc-1957276.patch4.7 KBdawehner
#44 interdiff.txt4.99 KBdawehner
#44 block-1957276.patch4.72 KBdawehner
#42 title.png8.2 KBdawehner
#42 block0.png14.46 KBdawehner
#42 block-1957276.patch5.08 KBdawehner
#30 2013-05-12_1315.png30.6 KBBojhan
#27 block_title_view_title.png13.75 KBxjm
#26 views-block-title-1957276-26.patch7.86 KBxjm
#23 view-block-title-1957276-23.patch3.47 KBxjm
#23 override_title.png12.71 KBxjm
#7 views-override-title-1957276-7.patch5.83 KBxjm
#7 using_view_title.png5.4 KBxjm
#7 override_title.png11.31 KBxjm
#5 views-1957276-5.patch4.85 KBxjm
views-block-title-be-free.patch831 bytesxjm
configure_views_block.png46.86 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue tags: +Usability, +VDC

Status: Needs review » Needs work

The last submitted patch, views-block-title-be-free.patch, failed testing.

dawehner’s picture

This results in this UX fail:

Couldn't this also fail in the other way round? I guess if we do it here, we should also make it clear in the views UI, that the title is set JUST in the block configuration.

It's kind of sad, that once you have the same view in multiple instances (which certainly happens once you have something like a layout builder) you would have to update each block instance in order to update just the title of the view.

xjm’s picture

It's kind of sad, that once you have the same view in multiple instances (which certainly happens once you have something like a layout builder) you would have to update each block instance in order to update just the title of the view.

Well, this is also true of menus, or custom blocks, or any other block type, and it's actually by design. Those instances should be able to have different titles. I agree that we probably want to make it clear in the Views UI how the title would be used. My one concern at the moment is titles with argument handling, but core also doesn't provide a way to get the context for those arguments into the block yet. =/ But @webchick is adamant that block administration is a content author task and people who shouldn't have access to the Views UI should be able to change it.

@davereid suggested last night allowing tokens for the block instance title that would refer to the Views title. We could alternately expose a checkbox or something to use the display title (so long as we actually show what it is on the form). We also need to automatically generate the machine name if we do that.

xjm’s picture

Status: Needs work » Needs review
FileSize
4.85 KB

So, it sort of confused me that this test and its friends are in the Block module, rather than the Views module, since Views provides the plugins. Anyway. Halfway through adding test coverage for the attached (which just makes Views blocks act like other blocks, except makes the view title a fallback before the display label), I decided we could add a nice toggle, like:

[ Use view title |v|] View title name here in text

With the other option being:

[Override view title] __________________ *

Status: Needs review » Needs work

The last submitted patch, views-1957276-5.patch, failed testing.

xjm’s picture

Status: Needs work » Needs review
FileSize
11.31 KB
5.4 KB
5.83 KB

Something like this.
using_view_title.png

override_title.png

xjm’s picture

Couple things:

  • The form structure is probably not quite right; it should probably use some manner of container element.
  • The title "Title" shouldn't be set on use_view_title; it should be the title of the aforementioned container element and use_view_title should get an invisible title.
  • Not sure what to do about the required marker.
  • The machine name should probably display next to the dropdown when the title is not overridden.
  • I tried using #markup to print the title on the page instead of in the dropdown, but it doesn't seem to support #states, so I just put it inside the select box rather than trying to write more fancy JavaScript. ;) This might be better anyway.
  • I still haven't updated the tests, so they'll still fail.

Status: Needs review » Needs work

The last submitted patch, views-override-title-1957276-7.patch, failed testing.

xjm’s picture

I asked @yoroy and he pointed out that this is a confusing use of the select box, because the value of the title from the view is in it to begin with, and then moves out of it into the text field when you change the value. Going to propose a couple other patterns with some screenshots.

xjm’s picture

@yoroy suggested something like this:
Only local images are allowed.

This takes away the end users' (possibly confusing) ability to choose between overriding the view title or not, and makes it so the view is what determines whether the title is always overridden or not. I think this might address @dawhener's concern about being update all block instance titles, because then it's sitll in the Views' admin's power to force all blocks to inherit a particular title.

xjm’s picture

yoroy’s picture

Assigned: Unassigned » xjm
Status: Needs work » Needs review

Maybe something like this: http://dl.dropbox.com/u/538835/block-title-overrides.m4v

Ah crosspost

yoroy’s picture

Assigned: xjm » Unassigned
Status: Needs review » Needs work

oops

xjm’s picture

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

I updated the remaining tasks with the questions we need to address here.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Title: Let users set the block instance title for Views blocks » Let users set the block instance title for Views blocks in the Block UI
Component: views_ui.module » views.module
Category: bug » feature
Priority: Major » Normal

Alright. Since this turns out to be pretty complicated architecturally, trades one UX regression for another by conflating the Views vs. Block administrative domains, has garnered a lot of resistance from my co-maintainers, and does not have consensus on anything, I've used @yoroy's middle example from #13 as the basis for #1967460: Display the block title and populate the machine name for views blocks. I made that the major bug since it addresses the 2-3 worst UX problems with the current Views block plugin, and I'm recategorizing this as a feature request.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm’s picture

Assigned: Unassigned » xjm
Category: feature » bug
Priority: Normal » Major

Sorry for dropping the ball on this; I had a really overwhelming couple of weeks.

I discussed this problem space at length with webchick for maintainer feedback. We did testing and there is a feature regression from Drupal 7. In Drupal 7, users can set the block title to any old string in the Blocks UI, obliterating any Views title handling whatsoever. @webchick considers both #1968296: Block title != view title and #1967460: Display the block title and populate the machine name for views blocks wontfix.

So, we need to go back to the original approach in this issue: rip out Views' override for the title. Then we're no worse off than in D7; the only difference is that multiple block instances with multiple different overridden titles are now possible in core. Then, we should improve the UI to make it clearer to users that they're overriding a dynamic title. We can incorporate some of @yoroy's ideas, but we should punt for now on trying to tell the user what the View title actually is.

I'll try to follow up more in the next few days.

damiankloip’s picture

Do we mean removing where the title is overridden in ViewsBlock::blockBuild()?

tim.plunkett’s picture

That would force you to respecify the title for each block placement, and would be a regression from being able to use tokens.

But if we decide to ignore the tokens part, before we remove the ability to set a title from the Views UI, why not relabel the setting to say "Default title", with a description saying that it will just pre-fill the block label for you?

xjm’s picture

That would force you to respecify the title for each block placement, and would be a regression from being able to use tokens.

No, that's not the idea at all. Basically, we just change the label of the "Display block title" checkbox to something like "Override view title" or something along those lines. Views still does its full normal title handling, and the title is only replaced if the box is checked. And we uncheck it by default.

I'll roll a path to illustrate.

xjm’s picture

Status: Needs work » Needs review
FileSize
12.71 KB
3.47 KB

This is the general idea.

  • It provides the same features as Drupal 7. (The current state in HEAD is technically a feature regression, even though some might consider it an intentional one.)
  • It allows the ever-flexible View title to be used by default.
  • It still uses the same method for creating the administrative block label and machine name as other blocks.
  • It informs the user that the block title is special, and provides privileged users with a link to edit the View instead if they are so inclined.
  • It disables toggling the title visibility through the Block UI for simplicity. Users can do this part in Views.
  • If site administrators want to disallow overriding the View title here entirely, it's a simple form alter.
  • @webchick okayed this approach, and this issue is in response to her original concern.

We can work on the form text/UX if we want, but functionally, this is a clean, simple compromise.

It will fail tests. I just want to get the proof of concept up before I go celebrate the gradual loss of my youth. ;)

Status: Needs review » Needs work

The last submitted patch, view-block-title-1957276-23.patch, failed testing.

dawehner’s picture

I like the simplicity of the approach.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -39,6 +39,13 @@ class ViewsBlock extends BlockBase {
+  protected $override_title;

@@ -49,6 +56,9 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
+      $this->override_title = $configuration['override_title'];

Should be camel-cazed :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -49,6 +56,9 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
+    if (!empty($configuration['override_title'])) {

I'm not sure but do we need to implement the settings() method to register this configuration?

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -63,20 +73,55 @@ public function blockAccess() {
+      '#default_value' => $this->override_title ? TRUE : FALSE,

There is just no need for this ternary :)

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -63,20 +73,55 @@ public function blockAccess() {
+    if (user_access('administer views')) {
+      $form['override_title']['#description'] = t('<a href="@url">Edit the @viewname view</a> to set the dynamic title.', array('@viewname' => $view_name, '@url' => $display_url));
+    }

I'm wondering whether it might be helpful to tell people without administer views that they maybe need that permission in order to properly change the title of a view?

xjm’s picture

Status: Needs work » Needs review
Issue tags: +Needs usability review
FileSize
7.86 KB

Thanks @dawehner.

Rerolled after #1927608: Remove the tight coupling between Block Plugins and Block Entities (no interdiff because I rerolled by hand). I cleaned up everything around the handling of the override_title setting -- it does want to be in settings() I think, and it doesn't need its own special member property. Also fixed the ternary to check what it was actually supposed to check. ;)

I'm wondering whether it might be helpful to tell people without administer views that they maybe need that permission in order to properly change the title of a view?

My gut feeling is that we shouldn't confuse the user with information about permissions they don't have, but let's get feedback from the usability team on that and on the UI pattern and text.

Edit: that screenshot is now incorrect. Uploading a new one (see below).

It's a bit wordy; I wonder if we can simplify the text without confusing people? What happens is:

  • If the user leaves the box unchecked (the default), Views' dynamic title handling is used and the title in the text field is only used administratively and programmatically (to generate the machine name).
  • If the user checks the box, whatever is in the text field is used in place of the dynamic view title in addition to administatively and for the machine name.
  • The description is only displayed if the user has permission to edit the view and Views UI is enabled.
xjm’s picture

FileSize
13.75 KB

block_title_view_title.png

Bojhan’s picture

Assigned: xjm » Bojhan

I am reviewing this, but will need a bit more time to come up with some ideas. I will get that up a bit later.

xjm’s picture

Issue tags: +Needs tests

Thanks @Bojhan!

Tagging for tests just so I don't forget.

Bojhan’s picture

Issue tags: -Needs usability review
FileSize
30.6 KB

I was intrigued to find #1968296: Block title != view title and #1967460: Display the block title and populate the machine name for views blocks closed. I don't really understand why they were won't fixed.

Upon reviewing this I was quite confused about its intend. I would assume whatever is in my "title" field is going to be the title of my block. Because users will be tremendously focused on that particular field in the task flow of changing ones title. However we currently have no signaling in the actual field that its being replaced by views, nor that its custom. This signalling is lost, and you need read the checkbox below carefully to understand its being overridden and you can override it by changing the title and pressing the checkbox.

I'd rather much prefer us to do:

2013-05-12_1315.png

For a couple reasons:

  1. Its an existing pattern, we use tokens to inform that its created dynamically.
  2. Removing the token, signals you are making it custom.
  3. It avoids having two title related checkboxes beneath each other, adding confusion.

I know in the other issue we out ruled using tokens to do this, but I think that inventing a new pattern of using a checkbox to override the field that is showing above, is quite confusing. Primarily because people are really focused on the "title" field, and probably will miss the checkbox and upon seeing the checkbox have to learn the concept that changing the field, does not actually have it show up - you also need to check this checkbox to make it work.

I hope we can revisit the decision , and use tokens for this - because its a more consistent pattern, that has been validated and does not introduce this magic of changing a field, and having to click a checkbox for that change to take into effect.

Bojhan’s picture

Assigned: Bojhan » Unassigned
xjm’s picture

The problem with #30 is that it now behaves differently from all other blocks, and it no longer provides the administrative label for the block, nor the machine name. We're back to essentially the same problem as in the screenshot in the summary. The user has to set a machine name by hand, and additionally there's no indication what the title of the block is in the admin interfaces. When I fill out this form, I have no idea what this block is going to be called in the blocks UI.

xjm’s picture

@dawehner suggested providing a default of viewname_displayname_count for the machine name when the faux-token is used for the title, but that still leaves us with two problems.

  • What's the administrative label of the block? We could provide a non-editable default value based on the view settings, but then the user has no way of knowing which block instance it is. For all other blocks, the user's title becomes the admin label.
  • Looking at it, I also think I can use the token in my own pattern, like My [view:title] flowers, or even other tokens. I think that leapfrogs us up into another whole realm of complexity.
xjm’s picture

Mmmh, the more I think about it, the more I think the fake token will confuse people. :(

xjm’s picture

Assigned: Unassigned » xjm

I'll try rolling a patch so we can see what this will actually be like.

I hate this issue... We're on approach #7. :P

Bojhan’s picture

@xjm But don't we have other fields where we don't allow all replacement patterns. I definitely don't want us to go down the path of "fake tokens", but it should be clear there is only one token allowed here (which is why I didn't add the whole replacements UI). I am not sure how that will confuse people? Because they can't add their own constructs? I do agree that we can't have a token as label for a block in the block listing.

The administrative label of the block, should be added upon creation, given that this is created by views, it should inherit the title created there. Lets chat on IRC about this.

saltednut’s picture

xjm patiently gave me a deep overview of this problem and we resolved that there is an initial step that can be taken before solving this UI problem for views blocks: #1998582: Auto-generate machine_name for Views Blocks using [viewname]_[displayname] rather than forcing manual entry

merlinofchaos’s picture

In Drupal 7 Panels world, aka content_types, the way this works is that on most panes, you get a checkbox:

[ ] Override Title

If that is checked, than javascript makes the actual title appear. If that is not checked, then you automatically get whatever title that block wanted and the user doesn't have to think about it.

With Views, this is complicated one more step because the presence of that checkbox is actually a setting in the view itself; after speaking with EclipseGc, that setting is probably defaulted wrong, because it defaults to not allowing you to change the title when you place the pane. In general, that's the one setting that should be more permissive.

In D8, this appears to be complicated by deriving a machine name from the title. However, in the case of Views, it looks like #37 addresses this. I think that's a pretty good pattern: Pre-created blocks of this nature should be able to generate their own machine name. Changing the title should, IMO, be a checkbox that you have to activate.

Once you activate the checkbox, then Bojhan's mockup looks pretty good -- you enter the title, and you have a token to get the original title. Though I disagree with Bojhan; I think we should just allow all tokens relevant because people do crazy stuff with titles and if you're going to override a title that was given by the View, there's a good chance you want to do something crazy. Allowing tokens doesn't hurt. That said, at a minimum we still have to have a token for the original title, because it's common for people to just want to put a prefix/suffix around the old title.

I disagree that you should have to use a token as a matter of course. If a user is placing a block, and is required to put in a special token to get the correct title to the block that the block expects to use, then users will be confused and frustrated when they are working quickly and fail to see that they had to do this; and that they have to type a lot of words to get what should be default behavior.

Finally, in the real world the administrative title and the visible title of a piece of content diverge a lot. It should be easy and convenient for users to handle this diversion. The administrative title should mostly be provided by the view, and potentially have a checkbox or something to override it as well. I'm not sure about this last part, as blocks probably don't have the level of customization that Panels do today, so it may be much more rare that you'll need to use the administrative title to differentiate several very similar blocks. But in the real world, that is often necessary.

sdboyer’s picture

very relevant to this: #2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues. if we can get that in, Views can be a lot less backflippy here.

Berdir’s picture

Issue tags: -Usability, -Needs tests, -VDC

#26: views-block-title-1957276-26.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, views-block-title-1957276-26.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

Issue summary: View changes
Priority: Major » Critical
Status: Needs work » Needs review
FileSize
5.08 KB
14.46 KB
8.2 KB

This adds a really simple checkbox based overriding, see screenshots. The label on the block listing is not overridden by that title.


tim.plunkett’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
    @@ -121,7 +121,11 @@ public function getPlugin() {
    +    $definition += array(
    +      'admin_label' => '',
    +    );
    

    This should be in the Block annotation class instead.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
    @@ -94,10 +104,59 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    +        ),
    +        )),
    

    That's some weird indenting

  3. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
    @@ -94,10 +104,59 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    +   * {@inheritdoc}{
    

    Extra {

  4. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
    @@ -94,10 +104,59 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    +      $this->setConfigurationValue('views_label', $form_state['values']['label']);
    ...
    +      $this->setConfigurationValue('views_label', '');
    

    These should just be $this->configuration['views_label']

  5. +++ b/core/modules/views/views.module
    @@ -1355,3 +1355,23 @@ function views_element_validate_tags($element, &$form_state) {
    +function views_form_block_form_alter(&$form, &$form_state) {
    +  // Ensure the block-form being altered is a Views block configuration form.
    

    Why isn't this just in the block plugin?

dawehner’s picture

FileSize
4.72 KB
4.99 KB

These should just be $this->configuration['views_label']

I don't see why? We are doing that in other places, too and it just does exactly why you say as replacement.

tim.plunkett’s picture

grep for "this->setConfigurationValue". It appears nowhere.

The last submitted patch, 42: block-1957276.patch, failed testing.

dawehner’s picture

λ d8 → ħ git 8.x* → ag "setConfigurationValue"
block-1957276.patch
124:+ $this->setConfigurationValue('views_label', $form_state['values']['label']);
127:+ $this->setConfigurationValue('views_label', '');

core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorRenderingTest.php
55: $block->getPlugin()->setConfigurationValue('feed', $feed->id());
56: $block->getPlugin()->setConfigurationValue('block_count', 2);
75: $block->getPlugin()->setConfigurationValue('block_count', 0);

core/modules/aggregator/lib/Drupal/aggregator/Tests/CategorizeFeedItemTest.php
80: $block->getPlugin()->setConfigurationValue('cid', $category->cid);

core/modules/aggregator/lib/Drupal/aggregator/Tests/RemoveFeedTest.php
39: $block->getPlugin()->setConfigurationValue('feed', $feed1->id());
42: $block2->getPlugin()->setConfigurationValue('feed', $feed2->id());

core/modules/block/lib/Drupal/block/Tests/BlockTest.php
258: $block->getPlugin()->setConfigurationValue('cache', DRUPAL_NO_CACHE);

core/modules/block/lib/Drupal/block/Tests/BlockCacheTest.php
201: $this->block->getPlugin()->setConfigurationValue('cache', $cache_mode);

core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.php
58: $display_block->setConfigurationValue('display_message', 'My custom display message.');

core/modules/block/lib/Drupal/block/Plugin/views/display/Block.php
340: $block->setConfigurationValue('items_per_page', $form_state['values']['override']['items_per_page']);

core/modules/block/lib/Drupal/block/BlockPluginInterface.php
65: public function setConfigurationValue($key, $value);

core/modules/block/lib/Drupal/block/BlockBase.php
63: public function setConfigurationValue($key, $value) {

core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
166: $this->setConfigurationValue('views_label', $form_state['values']['label']);
169: $this->setConfigurationValue('views_label', '');

core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.php
109: $block->getPlugin()->setConfigurationValue('block_count', 10);

core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.php
81: $block->getPlugin()->setConfigurationValue('block_count', 10);

core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php
74: $block->getPlugin()->setConfigurationValue('block_count', 2);
136: $block->getPlugin()->setConfigurationValue('block_count', 2);

tim.plunkett’s picture

Yes, those are all external. But every single blockSubmit() method uses $this->configuration.

dawehner’s picture

FileSize
4.7 KB
851 bytes

I should better not comment that at all.

dawehner’s picture

Assigned: xjm » Unassigned
Issue tags: +VDC

Adding tags.

slashrsm’s picture

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
    @@ -94,10 +104,73 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    +    // Load the Views plugin object using form_state array and create a
    +    // Prevent users from changing the auto-generated block machine_name.
    +    $form['id']['#access'] = FALSE;
    +
    

    This comment needs to be fixed a little bit.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
    @@ -94,10 +104,73 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    +  public function blockSubmit($form, &$form_state) {
    +    if (!empty($form_state['values']['views_label_checkbox'])) {
    +      $this->configuration['views_label'] = $form_state['values']['label'];
    +    }
    +    else {
    +      $this->configuration['views_label'] = '';
    +    }
    +  }
    +
    

    Needs to be changed to:

    $this->configuration['views_label'] = $form_state['values']['views_label'];
    

    I created a block view and positioned it to the sidebar. I tried to override the title and I can confirm that it does not save.

    We probably need test coverage for that also.

slashrsm’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
dawehner’s picture

Status: Needs work » Needs review
FileSize
4.12 KB
6.16 KB

Thank you for the review!

slashrsm’s picture

Status: Needs review » Needs work

Now it saves correctly, but it does not have any effect (title stays default even if I override it).

This part would probably also need test coverage.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
2.39 KB

Okay okay ... more test coverage needed

tim.plunkett’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
    @@ -121,7 +121,8 @@ public function getPlugin() {
    +    $definition = $this->getPlugin()->getPluginDefinition();
    +    return $settings['label'] ?: $definition['admin_label'];
    

    Is there any reason not to use an if/else here, and keep the $definition calls in the else? Or is that a micro-optimization?

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
    @@ -46,7 +52,7 @@ public function build() {
    +    $settings = parent::defaultConfiguration();
    

    The rest of this method is

    
        if ($this->displaySet) {
          return $this->view->display_handler->blockSettings($settings);
        }
    
        return $settings;
    

    Should the parent call be +='d into the blockSettings? Otherwise just return parent::defaultConfiguration(); seems cleaner.

dawehner’s picture

FileSize
1.3 KB
7.87 KB

Good points!

Status: Needs review » Needs work

The last submitted patch, 57: vdc-1957276.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

57: vdc-1957276.patch queued for re-testing.

tim.plunkett’s picture

+++ b/core/modules/block/lib/Drupal/block/Entity/Block.php
@@ -121,8 +121,12 @@ public function getPlugin() {
+      return $this->getPlugin()->getPluginDefinition();

Missing an 'admin_label' in here. If this doesn't fail we need tests.

Status: Needs review » Needs work

The last submitted patch, 57: vdc-1957276.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
561 bytes
7.92 KB

Ha.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

This would be better with #2025649: Add title-related methods to BlockPluginInterface to help clarify and resolve many issues, but that can happen once that's in.
RTBC if green.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
@@ -94,10 +104,75 @@ public function buildConfigurationForm(array $form, array &$form_state) {
+      '#markup' => $this->t('<div class="warning">In most cases you better change the title directly on the view. Overriding the title here will remove dynamic title support from arguments for example.</div>'),

I think we could improve on 'you better'. 'it is recommended to', 'it is more appropriate to'?

Why are we offering a UI option then flashing a warning on it straight away? Or to put it a different way, what's the actual use case for setting the title here - that should be explained in the description if anything.

webchick’s picture

Issue tags: +alpha target

AFAIK this is the only thing that blocks views block conversions patches, so tagging it as a target for alpha 7.

dawehner’s picture

Status: Needs work » Needs review
FileSize
978 bytes
7.91 KB

Why are we offering a UI option then flashing a warning on it straight away? Or to put it a different way, what's the actual use case for setting the title here - that should be explained in the description if anything.

People argue that there are people which has access to configure blocks/place blocks but not allow to change the actual views. So we have to copy UI from views to the block configuration.

Here is a suggestion: add that functionality so we can start to do the block conversions, but maybe improve the text/UI in a follow up.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

I agree with this suggestion. We should probably involve some UX people to that follow-up.

Bojhan’s picture

I am not too sure about this solution, why don't we just give a description and be done with it? I feel like we are special casing this, and it is not a pattern we can or should use elsewhere.

Drupal should inform people what their choice means through descriptions, however we shouldn't belittle them by warning them about their choice.

The text is really not up to standard of core. It uses a belittling tone "you better" and uses very technical lingo "dynamic title support". I suggest saying something like "Changing the title here, overrides the title created dynamically by [view]".

I strongly suggest to stop the "follow-up" mentality for UX issues, the whole idea of this issue is to fix the UX. You shouldn't then put the majority of the UX in a followup.

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review
catch’s picture

Changing the title here, overrides the title created dynamically by [view]".

I think this is plenty (without the warning styling). We can make [view] link to the views UI if the module is enabled/user has permission.

dawehner’s picture

FileSize
3.73 KB
9.89 KB

Here is the suggestion implemented.

dawehner’s picture

FileSize
5.43 KB

Here is also a screenshot of the screen.

yoroy’s picture

Status: Needs review » Needs work

Thanks for screenshot. I suspect bojhan meant to suggest that [view] would be replaced with the actual views name. If that's too much, then we have to add "the":

Changing the title here overrides the title created dynamically by [the view].

I was thinking

Changing the title here will remove some flexibility. (Try setting the title [directly in the view]).

With the part between () based on the acces check and the part between [] as the link.

dawehner’s picture

As said on irc:

yoroy: so wel really don't tell people why changing the title here is a bad idea?

The point of that description is to tell people why doing it here is not really right.

dawehner’s picture

The reason why I want to explain things is that people with just block permissions, should better not break the site functionality by just setting a title.

There are clear fundamental disagreements with the assumption whether users want to understand things or not. @xjm Feel free to bring that home, but I cannot really support and issue assuming that people are stupid.

yoroy’s picture

People aren't stupid, I'm just questioning how helpful it is to talk about overrides, arguments dynamic title support from arguments.

Dawehner mentioned he'd be happy with bojhan's version. How about this then:

Changing the title here means it cannot be dynamically altered anymore. (Try changing it directly in [the view].)

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
9.96 KB

New version.

tim.plunkett’s picture

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlockBase.php
@@ -154,10 +154,11 @@ public function buildConfigurationForm(array $form, array &$form_state) {
+      $form['views_label']['#description'] = $this->t('Changing the title here means it cannot be dynamically altered anymore. (Try changing it directly in <a href="@url">@name</a>.)', array('@url' => \Drupal::url('views_ui.edit', array('view' => $this->view->storage->id(), '@name' => $this->view->storage->label()))));

The parentheses are wrong here, the @name is passed to url(), it needs to be passed to t().

dawehner’s picture

FileSize
1.3 KB
9.96 KB

Good catch.

dawehner’s picture

FileSize
1.34 KB
10 KB

Some inspirational idea: let's link to the proper edit url.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Truly inspired! :)

Thanks UXers and dawehner for working through this.

The last submitted patch, 71: vdc-1957276.patch, failed testing.

The last submitted patch, 77: vdc-1957276.patch, failed testing.

The last submitted patch, 79: vdc-1957276.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 80: vdc-1957276.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.83 KB
11.83 KB

A unit test just needs an additional injection.

webchick’s picture

AWESOME work, folks!! I'm fine to commit this as-is, since it resolves the bug in the issue title. So I will do that tomorrow.

However! Since Daniel gets up when I go to bed, I am curious about "just one more thing." :P

"Recent content" block in HEAD:

Bock title is editable, machine name is hidden

"Content" block created via adding a block display to the Front page view:

Machine name is visible and wide, title is hidden

...and with Override title checked:

Title fieldset expands, with description pointing to view edit page.

The one remaining visual problem here is the machine name field does not follow the standard pattern (also, that extra fieldset around the title is something non-standard we don't do elsewhere). So, just asking (and it's fine if this is better as a follow-up), would it be possible for this instead to look like so (standard machine name pattern with text field simply disabled):

Use standard machine name pattern, but with name field disabled at first

...and with Override title checked (field becomes non-disabled, description shows up):

Field becomes un-disabled, description shows up below.

...because then there is pretty much literally no difference between the two and so all UX concerns around Views vs. normal blocks are addressed.

webchick’s picture

Btw, now that I think about it, that dynamic description thing is probably more trouble than it's worth. So it'd just be the checkbox toggling the disabled property (and possibly some CSS; Firebug didn't seem to change it to grey when I did that) on the text field then.

webchick’s picture

Ok, didn't catch Daniel, but spoke to Tim and he confirmed this idea was not completely crack-addled, so spun off #2151731: Views blocks' machine names do not fit the standard Drupal pattern.

AAAAAAAND!

Committed and pushed to 8.x. YEAH!! :D

Thank you SO much, all, for slogging through this. Now let's get those damn blocks converted. :D

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops. :)

dawehner’s picture

Committed and pushed to 8.x. YEAH!! :D

Thank you so much more! See you in all the other issues.

Status: Fixed » Closed (fixed)

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