Problem/Motivation

During the testing at BADcamp we noticed all the participant disabling the "Create a label" checkbox. For tables this makes sense, but for most of the other format settings it doesn't make sense.

When adding a field, “Create a label” is checked by default but we observed most people deselecting this. It’s only mostly useful for table display. (P4, P6, P7, P3).

Proposed resolution

We propose that this default is removed, arguably there are cases where a label is the 80% for instance with a table. We might want to do something more smart there, but in all other cases its likely you don't want it.

Remaining tasks

  • Remove default setting

User interface changes

The checkbox is not checked by default.

API changes

-

Files: 
CommentFileSizeAuthor
#119 interdiff-1831674-118.txt647 byteslokapujya
#118 interdiff-1831674-118.txt638 byteslokapujya
#118 1831674-118.patch10.84 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,952 pass(es). View
#116 interdiff-1831674-116.txt746 byteslokapujya
#116 1831674-116.patch10.21 KBlokapujya
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,644 pass(es), 1 fail(s), and 0 exception(s). View
#114 1831674-interdiff.txt1.13 KBlongwave
#114 1831674-views-create-a-label-114.patch10.17 KBlongwave
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#107 label_after.png19.43 KBStefan Lehmann
#107 label_before.png16.58 KBStefan Lehmann
#100 create-a-label-should-be-off-by-default-after.png61.14 KBdaviddemello
#100 create-a-label-should-be-off-by-default-before.png61.79 KBdaviddemello
create-a-label-by-default.png7.85 KBBojhan

Comments

Bojhan’s picture

Title: By default disable “Create a label” » “Create a label” by default is a confusing default
dawehner’s picture

Title: “Create a label” by default is a confusing default » By default disable “Create a label”

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?

Bojhan’s picture

Title: By default disable “Create a label” » “Create a label” by default is a confusing default

-using solutions as issue title.

dawehner’s picture

Status: Active » Needs review
FileSize
33.16 KB
FAILED: [[SimpleTest]]: [MySQL] 48,036 pass(es), 20 fail(s), and 5 exception(s). View

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

dawehner’s picture

FileSize
1.98 KB
FAILED: [[SimpleTest]]: [MySQL] 48,021 pass(es), 17 fail(s), and 2 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal-1831674-2.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.41 KB
4.85 KB
PASSED: [[SimpleTest]]: [MySQL] 48,007 pass(es). View

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

dawehner’s picture

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

tim.plunkett’s picture

Title: “Create a label” by default is a confusing default » "Create a label" should be off by default, with an opt-in for style plugins
Status: Needs review » Needs work
Issue tags: +Needs tests

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.

dawehner’s picture

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.

tim.plunkett’s picture

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.

dawehner’s picture

Assigned: Unassigned » dawehner

Wait i even remember when i wrote that.

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
4.64 KB
9.26 KB
FAILED: [[SimpleTest]]: [MySQL] 48,796 pass(es), 28 fail(s), and 0 exception(s). View

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

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

The last submitted patch, drupal-1831674-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, drupal-1831674-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

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

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

The last submitted patch, drupal-1831674-13.patch, failed testing.

dawehner’s picture

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.

Bojhan’s picture

Is there a better way to do this? Seems like this might create quite an efficiency boost.

dawehner’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
FAILED: [[SimpleTest]]: [MySQL] 54,201 pass(es), 42 fail(s), and 27 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, drupal-1831674-21.patch, failed testing.

Bojhan’s picture

Could this get another reroll? It would be nice to see this in core.

Bojhan’s picture

Issue tags: +Novice

This looks like something anyone could do.

Bojhan’s picture

Category: feature » task

Going to make this a task, since we are doing this for usability purposes. Its not really a feature.

Kevin Morse’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
FAILED: [[SimpleTest]]: [MySQL] 55,635 pass(es), 42 fail(s), and 0 exception(s). View

Okay lets see how this does.

Status: Needs review » Needs work
Issue tags: -Usability, -Novice, -VDC, -BADCamp2012UX

The last submitted patch, drupal-1831674-21-reroll.patch, failed testing.

samhassell’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Usability, +Novice, +VDC, +BADCamp2012UX

The last submitted patch, drupal-1831674-21-reroll.patch, failed testing.

Bojhan’s picture

Issue tags: +sprint
drupalway’s picture

Assigned: Unassigned » drupalway
Issue tags: +CodeSprintUA

We are working today with this issue during Code Sprint UA

drupalway’s picture

Assigned: drupalway » Unassigned
Issue tags: -CodeSprintUA

Unassigned

lokapujya’s picture

Assigned: Unassigned » lokapujya
Status: Needs work » Needs review
FileSize
8.96 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Status: Needs review » Needs work

The last submitted patch, views26-reroll.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9.04 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Tests can't even run.

Status: Needs review » Needs work

The last submitted patch, views26-reroll-35.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9.08 KB
FAILED: [[SimpleTest]]: [MySQL] 58,871 pass(es), 16 fail(s), and 0 exception(s). View

Trying to get the tests to run.

Status: Needs review » Needs work

The last submitted patch, views26-reroll-37.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9.08 KB
FAILED: [[SimpleTest]]: [MySQL] 59,079 pass(es), 16 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, views26-reroll-39.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9.07 KB
FAILED: [[SimpleTest]]: [MySQL] 58,986 pass(es), 14 fail(s), and 2 exception(s). View

Status: Needs review » Needs work

The last submitted patch, views26-reroll-41.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9.06 KB
FAILED: [[SimpleTest]]: [MySQL] 59,006 pass(es), 13 fail(s), and 1 exception(s). View

Status: Needs review » Needs work

The last submitted patch, views26-reroll-43.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
424.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-1831674-44.patch. Unable to apply patch. See the log in the details link for more information. View

need to see a simpletest.

Status: Needs review » Needs work

The last submitted patch, views-1831674-44.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
10.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch views-1831674-47.patch. Unable to apply patch. See the log in the details link for more information. View

wrong patch last time.

Status: Needs review » Needs work

The last submitted patch, views-1831674-47.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9.21 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Status: Needs review » Needs work

The last submitted patch, views-create-a-label-1831674-49.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
9.54 KB
FAILED: [[SimpleTest]]: [MySQL] 59,239 pass(es), 3 fail(s), and 0 exception(s). View

Moved tests to view_ui.

Status: Needs review » Needs work

The last submitted patch, views-create-a-label-1831674-51.patch, failed testing.

lokapujya’s picture

Assigned: lokapujya » Unassigned

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

dawehner’s picture

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

lokapujya’s picture

Assigned: Unassigned » lokapujya
lokapujya’s picture

Status: Needs work » Needs review
FileSize
10.33 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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.

Status: Needs review » Needs work

The last submitted patch, views-create-a-label-1831674-55.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
10.33 KB
FAILED: [[SimpleTest]]: [MySQL] 57,574 pass(es), 132 fail(s), and 26 exception(s). View

Status: Needs review » Needs work

The last submitted patch, views-create-a-label-1831674-58.patch, failed testing.

dawehner’s picture

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

lokapujya’s picture

Status: Needs work » Needs review
FileSize
10.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,404 pass(es). View
dawehner’s picture

WOW WOW WOW

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

lokapujya’s picture

FileSize
1.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1831674-51-61.patch. Unable to apply patch. See the log in the details link for more information. View

Modify tests to not look for default labels.

Status: Needs review » Needs work

The last submitted patch, interdiff-1831674-51-61.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review

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

neoligero’s picture

FileSize
686 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,875 pass(es), 1 fail(s), and 0 exception(s). View

Done.

Status: Needs review » Needs work

The last submitted patch, Nodefaultlabel-1831674-66.patch, failed testing.

mcrittenden’s picture

Status: Needs work » Needs review

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.

Bojhan’s picture

Wondering what the status of this is, would be great i we can get this in.

lokapujya’s picture

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

Bojhan’s picture

Assigned: lokapujya » Unassigned
Issue summary: View changes
Status: Needs review » Needs work

This needs a small test to be fixed.

longwave’s picture

Status: Needs work » Needs review
FileSize
9.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831674-views-create-a-label-72.patch. Unable to apply patch. See the log in the details link for more information. View
3.31 KB

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

The last submitted patch, 72: 1831674-views-create-a-label-72.patch, failed testing.

longwave’s picture

The last submitted patch, 72: 1831674-views-create-a-label-72.patch, failed testing.

longwave’s picture

lokapujya’s picture

The last submitted patch, 72: 1831674-views-create-a-label-72.patch, failed testing.

lokapujya’s picture

Assigned: Unassigned » lokapujya
FileSize
9.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,449 pass(es), 6 fail(s), and 0 exception(s). View

re-roll of 1831674-72.

Status: Needs review » Needs work

The last submitted patch, 79: 1831674-79.patch, failed testing.

lokapujya’s picture

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?

lokapujya’s picture

FileSize
1.08 KB

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

lokapujya’s picture

FileSize
10.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,953 pass(es). View

I suppose I should attach the patch.

lokapujya’s picture

Status: Needs work » Needs review
jibran’s picture

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

adnanc’s picture

Status: Needs review » Needs work

most recent patch fails testing

longwave’s picture

Status: Needs work » Needs review
FileSize
10.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,265 pass(es). View

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 87: 1831674-views-create-a-label-87.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
lokapujya’s picture

lokapujya’s picture

Issue tags: +Needs usability review
Bojhan’s picture

What should I review, its only a check in a checkbox right?

lokapujya’s picture

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)

Bojhan’s picture

Assigned: lokapujya » dawehner

Yup, sounds good to I will leave it up to the views ui maintainer.

dawehner’s picture

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

Mirakolous’s picture

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

Mirakolous’s picture

Status: Needs review » Needs work
Mirakolous’s picture

FileSize
10.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,460 pass(es), 3 fail(s), and 0 exception(s). View

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

Bojhan’s picture

Status: Needs work » Needs review
daviddemello’s picture

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

daviddemello’s picture

Assigned: dawehner » Unassigned
jtjones23’s picture

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.

Mirakolous’s picture

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

Bojhan’s picture

Issue tags: -Needs usability review

Can anyone verify this works - it did here? Then we can mark it RTBC.

HeikeT’s picture

Issue tags: +#Drupal8nz

Tested. Works as described plus improves usability for most of my site building use cases.
Only local images are allowed.
Only local images are allowed.

Ta!

(review and testing part of #Drupal8NZ)

Stefan Lehmann’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
16.58 KB
19.43 KB

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

Before:
Label checked before.

After:
Label unchecked after

Setting it to RBTC now.

(review and testing part of #Drupal8NZ)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 98: 1831674-views-create-a-label-96.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
10.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,504 pass(es), 1 fail(s), and 0 exception(s). View
2.88 KB

Rerolled to include the randomName -> randomMachineName change.

Status: Needs review » Needs work

The last submitted patch, 109: 1831674-views-create-a-label-109.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
10.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,342 pass(es). View
1.64 KB

views_get_view() -> Views::getView()

longwave’s picture

FileSize
10.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,595 pass(es). View
910 bytes

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

dawehner’s picture

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

longwave’s picture

FileSize
10.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.13 KB

Addressed #113 (and #95!)

Status: Needs review » Needs work

The last submitted patch, 114: 1831674-views-create-a-label-114.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
10.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,644 pass(es), 1 fail(s), and 0 exception(s). View
746 bytes

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

Status: Needs review » Needs work

The last submitted patch, 116: 1831674-116.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
10.84 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,952 pass(es). View
638 bytes

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

lokapujya’s picture

FileSize
647 bytes

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's get it in!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d60d5be and pushed to 8.0.x. Thanks!

  • alexpott committed d60d5be on 8.0.x
    Issue #1831674 by lokapujya, longwave, dawehner, Mirakolous, neoligero,...

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Version: 8.0.x-dev » 8.0.0
Issue tags: -sprint

Thanks, removing from UX sprint now.