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.

Files: 
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
PASSED: [[SimpleTest]]: [MySQL] 59,238 pass(es).
[ View ]
#86 interdiff.txt1.83 KBdawehner
#80 vdc-1957276.patch10 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,242 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#80 interdiff.txt1.34 KBdawehner
#79 vdc-1957276.patch9.96 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,205 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#79 interdiff.txt1.3 KBdawehner
#77 vdc-1957276.patch9.96 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,261 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#77 interdiff.txt1.39 KBdawehner
#72 ui.png5.43 KBdawehner
#71 vdc-1957276.patch9.89 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,913 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#71 interdiff.txt3.73 KBdawehner
#66 vdc-1957276.patch7.91 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,220 pass(es).
[ View ]
#66 interdiff.txt978 bytesdawehner
#62 vdc-1957276.patch7.92 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,355 pass(es).
[ View ]
#62 interdiff.txt561 bytesdawehner
#57 vdc-1957276.patch7.87 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,259 pass(es), 0 fail(s), and 79 exception(s).
[ View ]
#57 interdiff.txt1.3 KBdawehner
#55 interdiff.txt2.39 KBdawehner
#55 vdc-1957276.patch7.74 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,168 pass(es).
[ View ]
#53 vdc-1957276.patch6.16 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,181 pass(es).
[ View ]
#53 interdiff.txt4.12 KBdawehner
#49 interdiff.txt851 bytesdawehner
#49 vdc-1957276.patch4.7 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,091 pass(es).
[ View ]
#44 interdiff.txt4.99 KBdawehner
#44 block-1957276.patch4.72 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es).
[ View ]
#42 title.png8.2 KBdawehner
#42 block0.png14.46 KBdawehner
#42 block-1957276.patch5.08 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,965 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-block-title-1957276-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#23 view-block-title-1957276-23.patch3.47 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 55,423 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#23 override_title.png12.71 KBxjm
#7 views-override-title-1957276-7.patch5.83 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 53,830 pass(es), 4 fail(s), and 7 exception(s).
[ View ]
#7 using_view_title.png5.4 KBxjm
#7 override_title.png11.31 KBxjm
#5 views-1957276-5.patch4.85 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 53,864 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
views-block-title-be-free.patch831 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] 53,865 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
configure_views_block.png46.86 KBxjm

Comments

Issue tags:+Usability, +VDC

Status:Needs review» Needs work

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new4.85 KB
FAILED: [[SimpleTest]]: [MySQL] 53,864 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new11.31 KB
new5.4 KB
new5.83 KB
FAILED: [[SimpleTest]]: [MySQL] 53,830 pass(es), 4 fail(s), and 7 exception(s).
[ View ]

Something like this.
using_view_title.pngoverride_title.png

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.

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.

@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.

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

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

Ah crosspost

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

oops

Issue summary:View changes

Updated issue summary.

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

Issue summary:View changes

Updated issue summary.

Title:Let users set the block instance title for Views blocksLet 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.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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.

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

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?

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.

Status:Needs work» Needs review
StatusFileSize
new12.71 KB
new3.47 KB
FAILED: [[SimpleTest]]: [MySQL] 55,423 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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?

Status:Needs work» Needs review
Issue tags:+Needs usability review
StatusFileSize
new7.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-block-title-1957276-26.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new13.75 KB

block_title_view_title.png

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.

Issue tags:+Needs tests

Thanks @Bojhan!

Tagging for tests just so I don't forget.

Issue tags:-Needs usability review
StatusFileSize
new30.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.

Assigned:Bojhan» Unassigned

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.

@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.

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

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

@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.

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

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.

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.

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.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes
Priority:Major» Critical
Status:Needs work» Needs review
StatusFileSize
new5.08 KB
FAILED: [[SimpleTest]]: [MySQL] 58,965 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
new14.46 KB
new8.2 KB

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

  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?

StatusFileSize
new4.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es).
[ View ]
new4.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.

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

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

λ 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);

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

StatusFileSize
new4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,091 pass(es).
[ View ]
new851 bytes

I should better not comment that at all.

Assigned:xjm» Unassigned
Issue tags:+VDC

Adding tags.

  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.

Status:Needs review» Needs work
Issue tags:+Needs tests

Status:Needs work» Needs review
StatusFileSize
new4.12 KB
new6.16 KB
PASSED: [[SimpleTest]]: [MySQL] 59,181 pass(es).
[ View ]

Thank you for the review!

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.

Status:Needs work» Needs review
StatusFileSize
new7.74 KB
PASSED: [[SimpleTest]]: [MySQL] 59,168 pass(es).
[ View ]
new2.39 KB

Okay okay ... more test coverage needed

  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.

StatusFileSize
new1.3 KB
new7.87 KB
FAILED: [[SimpleTest]]: [MySQL] 59,259 pass(es), 0 fail(s), and 79 exception(s).
[ View ]

Good points!

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new561 bytes
new7.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,355 pass(es).
[ View ]

Ha.

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new978 bytes
new7.91 KB
PASSED: [[SimpleTest]]: [MySQL] 59,220 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

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

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.

Status:Reviewed & tested by the community» Needs review

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.

StatusFileSize
new3.73 KB
new9.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,913 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Here is the suggestion implemented.

StatusFileSize
new5.43 KB

Here is also a screenshot of the screen.

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.

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.

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.

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].)

Status:Needs work» Needs review
StatusFileSize
new1.39 KB
new9.96 KB
FAILED: [[SimpleTest]]: [MySQL] 59,261 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

New version.

+++ 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().

StatusFileSize
new1.3 KB
new9.96 KB
FAILED: [[SimpleTest]]: [MySQL] 59,205 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Good catch.

StatusFileSize
new1.34 KB
new10 KB
FAILED: [[SimpleTest]]: [MySQL] 59,242 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.83 KB
new11.83 KB
PASSED: [[SimpleTest]]: [MySQL] 59,238 pass(es).
[ View ]

A unit test just needs an additional injection.

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.

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.

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

Status:Reviewed & tested by the community» Fixed

Oops. :)

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.