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
-
Comment | File | Size | Author |
---|---|---|---|
#119 | interdiff-1831674-118.txt | 647 bytes | lokapujya |
#118 | interdiff-1831674-118.txt | 638 bytes | lokapujya |
#118 | 1831674-118.patch | 10.84 KB | lokapujya |
#116 | interdiff-1831674-116.txt | 746 bytes | lokapujya |
#116 | 1831674-116.patch | 10.21 KB | lokapujya |
Comments
Comment #1
Bojhan CreditAttribution: Bojhan commentedComment #2
dawehnerThere 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?
Comment #3
Bojhan CreditAttribution: Bojhan commented-using solutions as issue title.
Comment #4
dawehnerThis patch enables labels by default for tables, but disables them for all other styles.
Comment #5
dawehnerGood try to bring the snapshots patch in as part of a UX improvement :)
Comment #7
dawehnerSome of tests used no label, so let's add them.
Comment #8
dawehner#7: drupal-1831674-7.patch queued for re-testing.
Comment #9
tim.plunkettWhile 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.
Comment #10
dawehnerJust 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.
Comment #11
tim.plunkettI 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.
Comment #12
dawehnerWait i even remember when i wrote that.
Comment #13
dawehnerThe current patch actually disables the feature for tables, see interdiff :)
Comment #15
dawehner#13: drupal-1831674-13.patch queued for re-testing.
Comment #17
dawehner#13: drupal-1831674-13.patch queued for re-testing.
Comment #19
dawehnerThe 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.
Comment #20
Bojhan CreditAttribution: Bojhan commentedIs there a better way to do this? Seems like this might create quite an efficiency boost.
Comment #21
dawehnerRerolled, let's see how much fails after 4 months.
Comment #23
Bojhan CreditAttribution: Bojhan commentedCould this get another reroll? It would be nice to see this in core.
Comment #24
Bojhan CreditAttribution: Bojhan commentedThis looks like something anyone could do.
Comment #25
Bojhan CreditAttribution: Bojhan commentedGoing to make this a task, since we are doing this for usability purposes. Its not really a feature.
Comment #26
Kevin Morse CreditAttribution: Kevin Morse commentedOkay lets see how this does.
Comment #28
samhassell CreditAttribution: samhassell commented#26: drupal-1831674-21-reroll.patch queued for re-testing.
Comment #30
Bojhan CreditAttribution: Bojhan commentedComment #31
drupalway CreditAttribution: drupalway commentedWe are working today with this issue during Code Sprint UA
Comment #32
drupalway CreditAttribution: drupalway commentedUnassigned
Comment #33
lokapujyaComment #35
lokapujyaTests can't even run.
Comment #37
lokapujyaTrying to get the tests to run.
Comment #39
lokapujyaComment #41
lokapujyaComment #43
lokapujyaComment #45
lokapujyaneed to see a simpletest.
Comment #47
lokapujyawrong patch last time.
Comment #49
lokapujyaComment #51
lokapujyaMoved tests to view_ui.
Comment #53
lokapujyaGood, re-roll is done and down to 3 simpletest fails.
Comment #54
dawehnerMaybe we can figure out the cryptic comment #19 as it still seems to be the problem.
Comment #55
lokapujyaComment #56
lokapujyaI 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.
Comment #58
lokapujyaComment #60
dawehnerPlease don't burn out yourself on this issue, I doubt that this is fixable.
Comment #61
lokapujyaComment #62
dawehnerWOW WOW WOW
It would be awesome if you could create an interdiff for this change: see https://drupal.org/node/1488712
Comment #63
lokapujyaModify tests to not look for default labels.
Comment #65
lokapujyaoops. shouldn't have named the interdiff ".patch". Setting back to needs review.
Comment #66
neoligero CreditAttribution: neoligero commentedDone.
Comment #68
mcrittenden CreditAttribution: mcrittenden commentedI'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.
Comment #69
Bojhan CreditAttribution: Bojhan commentedWondering what the status of this is, would be great i we can get this in.
Comment #70
lokapujya#61 needs a review. #63 is the interdiff.
Comment #71
Bojhan CreditAttribution: Bojhan commentedThis needs a small test to be fixed.
Comment #72
longwave#61 with debug() removed, some unnecessary test changes reverted, and two whitespace fixes.
Comment #74
longwave72: 1831674-views-create-a-label-72.patch queued for re-testing.
Comment #76
longwave72: 1831674-views-create-a-label-72.patch queued for re-testing.
Comment #77
lokapujya72: 1831674-views-create-a-label-72.patch queued for re-testing.
Comment #79
lokapujyare-roll of 1831674-72.
Comment #81
lokapujyaThe 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?
Comment #82
lokapujyaPossibly replacement patterns could pull the field name from admin_label instead of default.
Comment #83
lokapujyaI suppose I should attach the patch.
Comment #84
lokapujyaComment #85
jibran83: 1831674-82.patch queued for re-testing.
Comment #86
adnanc CreditAttribution: adnanc commentedmost recent patch fails testing
Comment #87
longwaveRerolled.
Comment #89
lokapujya87: 1831674-views-create-a-label-87.patch queued for re-testing.
Comment #90
lokapujya87: 1831674-views-create-a-label-87.patch queued for re-testing.
Comment #91
lokapujyaComment #92
Bojhan CreditAttribution: Bojhan commentedWhat should I review, its only a check in a checkbox right?
Comment #93
lokapujyaRegarding 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)
Comment #94
Bojhan CreditAttribution: Bojhan commentedYup, sounds good to I will leave it up to the views ui maintainer.
Comment #95
dawehnerThere is no reason to check view->style_plugin ... viewExecutable does the same. ... We could simplify all to $this->view->getStyle->defaultFieldLabels()
Comment #96
Mirakolous CreditAttribution: Mirakolous commentedThe 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
Comment #97
Mirakolous CreditAttribution: Mirakolous commentedComment #98
Mirakolous CreditAttribution: Mirakolous commentedI have rerolled this patch to reflect PSR-4 changes to file directory
Comment #99
Bojhan CreditAttribution: Bojhan commentedComment #100
daviddemello CreditAttribution: daviddemello commentedThe create-a-label checkbox is still checked by default.
Comment #101
daviddemello CreditAttribution: daviddemello commentedComment #102
jtjones23 CreditAttribution: jtjones23 commentedWhen 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.
Comment #103
Mirakolous CreditAttribution: Mirakolous commenteddaviddemello are you sure that you were not using the table format in your view?
Comment #104
Bojhan CreditAttribution: Bojhan commentedCan anyone verify this works - it did here? Then we can mark it RTBC.
Comment #106
HeikeT CreditAttribution: HeikeT commentedTested. Works as described plus improves usability for most of my site building use cases.
Ta!
(review and testing part of #Drupal8NZ)
Comment #107
Stefan Lehmann CreditAttribution: Stefan Lehmann commentedPatch 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)
Comment #109
longwaveRerolled to include the randomName -> randomMachineName change.
Comment #111
longwaveviews_get_view() -> Views::getView()
Comment #112
longwaveMoved StyleTableTest to PSR-4 and removed the getInfo() method.
Comment #113
dawehnerYou could just drop the if() to be honest.
Comment #114
longwaveAddressed #113 (and #95!)
Comment #116
lokapujyaBe safe. Check the object. initStyle() can return false with an empty object.
Comment #118
lokapujyaReroll. A new view needs the label specified, since by default the view wouldn't have labels.
Comment #119
lokapujyaThe real interdiff. The one in the previous comment is one I used for testing.
Comment #120
dawehnerLet's get it in!
Comment #121
alexpottCommitted d60d5be and pushed to 8.0.x. Thanks!
Comment #124
Gábor HojtsyThanks, removing from UX sprint now.