"Create a label" should be off by default, with an opt-in for style plugins

There is currently one way of behavior for checkboxes in views: they all just toggles options in the actual UI.

Let's say when you uncheck the label checkbox and hit save, the actual value of the label internally get's removed (if the user might have entered something before).

When we do now hide the label by default you will never have a proper default value for the label, which would
be probably a problem. It seems to be that we really have to store the actual checkbox as well?

This patch enables labels by default for tables, but disables them for all other styles.

Good try to bring the snapshots patch in as part of a UX improvement :)

Some of tests used no label, so let's add them.

#7: drupal-1831674-7.patch queued for re-testing.

While manually testing this, creating a view with table as the style plugin didn't create a label by default. We should add tests for that, and then fix it.

Just to be sure, did you created a view and choosed table in the wizard or did you created
a view and then switched to table? It feels odd that when you switch to table that the lables are switched then.

I tried both, I didn't expect it to change for a built view, but from the wizard it didn't work and I expected it to.

Wait i even remember when i wrote that.

The current patch actually disables the feature for tables, see interdiff :)

#13: drupal-1831674-13.patch queued for re-testing.

#13: drupal-1831674-13.patch queued for re-testing.

The problem seems to be that by initializing the Style Plugin and querying for defaultFieldLabels() you miss the right configuration of the fields handlers, because they actually uses the override method of handlers. It seems to that we might have to find a better way to initialize the groupby handlers but well this is for sure out of scope of this issue.

Rerolled, let's see how much fails after 4 months.

Okay lets see how this does.

#26: drupal-1831674-21-reroll.patch queued for re-testing.

We are working today with this issue during Code Sprint UA

Unassigned

Tests can't even run.

Trying to get the tests to run.

need to see a simpletest.

wrong patch last time.

Moved tests to view_ui.

Good, re-roll is done and down to 3 simpletest fails.

Maybe we can figure out the cryptic comment #19 as it still seems to be the problem.

I think the problem is that the GroupByTest is looking for the label. Easy way out is to change the GroupByTest to use field labels so that assertLink will work.

Please don't burn out yourself on this issue, I doubt that this is fixable.

WOW WOW WOW

It would be awesome if you could create an interdiff for this change: see https://drupal.org/node/1488712

Modify tests to not look for default labels.

oops. shouldn't have named the interdiff ".patch". Setting back to needs review.

Done.

I'm not sure what #66 is for but as far as I can tell, #61 still needs review since tests are passing on that one.

#61 needs a review. #63 is the interdiff.

#61 with debug() removed, some unnecessary test changes reverted, and two whitespace fixes.

72: 1831674-views-create-a-label-72.patch queued for re-testing.

72: 1831674-views-create-a-label-72.patch queued for re-testing.

72: 1831674-views-create-a-label-72.patch queued for re-testing.

re-roll of 1831674-72.

The new tests in #2137837: Current field is not displayed in replacement patterns reveal that replacement patterns uses [field handler]->label() to get the label, but since this patch would hide the labels, then we won't get labels for replacement patterns. Any recommendations?

Possibly replacement patterns could pull the field name from admin_label instead of default.

I suppose I should attach the patch.

83: 1831674-82.patch queued for re-testing.

most recent patch fails testing

Rerolled.

87: 1831674-views-create-a-label-87.patch queued for re-testing.

87: 1831674-views-create-a-label-87.patch queued for re-testing.

Regarding the usability flag, just verify that it's ok/desirable to have "Create a label" off by default (except for tables)?

I took this on as a Novice issue a while back. I think my interdiff in #82 could use a review. (Explained in #81)

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.php
@@ -410,7 +410,16 @@ public function useStringGroupBy() {
+    if (!isset($this->view->style_plugin)) {
+      $this->view->initStyle();
+    }
+
+    if (isset($this->view->style_plugin) && $this->view->style_plugin->defaultFieldLabels()) {

There is no reason to check view->style_plugin ... viewExecutable does the same. ... We could simplify all to $this->view->getStyle->defaultFieldLabels()

The patch needs to be rerolled for the new directory structure. FieldPluginBase.php is now located in a/core/modules/views/src/Plugin/views/field/FieldPluginBase.php

I have rerolled this patch to reflect PSR-4 changes to file directory

The create-a-label checkbox is still checked by default.

When I applied #98, the create-a-label checkbox remained only for the Table format. When I changed the format to HTML list or a Unformatted list the checkbox was not checked by default. I think the patch is working as advertised.

daviddemello are you sure that you were not using the table format in your view?

Tested. Works as described plus improves usability for most of my site building use cases.

Ta!

(review and testing part of #Drupal8NZ)

Patch from #98 works as expected. I checked all formats. It's only checked for the Table format after I applied the patch.

Before:

After:

Setting it to RBTC now.

(review and testing part of #Drupal8NZ)

Rerolled to include the randomName -> randomMachineName change.

views_get_view() -> Views::getView()

Moved StyleTableTest to PSR-4 and removed the getInfo() method.

+++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
@@ -428,7 +428,16 @@ public function useStringGroupBy() {
+    if (!isset($this->view->style_plugin)) {
+      $this->view->initStyle();
+    }

You could just drop the if() to be honest.

Addressed #113 (and #95!)

Be safe. Check the object. initStyle() can return false with an empty object.

Reroll. A new view needs the label specified, since by default the view wouldn't have labels.

The real interdiff. The one in the previous comment is one I used for testing.

Let's get it in!

Committed d60d5be and pushed to 8.0.x. Thanks!

Thanks, removing from UX sprint now.