Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

‘All displays” is the default option, even when there’s only a single display. This is unnecessary and is one more decision a new user has to fret over.

Possible solutions:

Hide “all display/This page” options e.g. filtering when there is only one.
Significantly decrease the visual importance of this functionality.

(This came out from the usability study at BADCamp 2012)

Files: 
CommentFileSizeAuthor
#64 after.png5.41 KBpguillard
#64 before.png10.18 KBpguillard
#62 1836384-62.patch3.84 KBLendude
#62 interdiff-1836384-55-62.txt2.34 KBLendude
#55 interdiff-1836384-49-55.txt762 bytesLendude
#55 1836384-55.patch2.37 KBLendude
#49 interdiff-1836384-45-49.txt687 bytespguillard
#49 1836384-49.patch1.87 KBpguillard
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,731 pass(es), 1 fail(s), and 0 exception(s). View
#45 1836384-45.patch2.37 KBpguillard
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,422 pass(es), 2 fail(s), and 0 exception(s). View
#42 1836384-42.patch2.44 KBLendude
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1836384-42.patch. Unable to apply patch. See the log in the details link for more information. View
#42 interdiff-1836384-36-42.txt1.31 KBLendude
#36 1836384-36.patch2.1 KBhynnot
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,988 pass(es). View
#33 1836384-33.patch2.08 KBhynnot
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,928 pass(es), 0 fail(s), and 1 exception(s). View
#31 interdiff-26-31.txt997 bytestadityar
#20 1836384-20.patch2.11 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,898 pass(es). View
#20 interdiff-1836384-20.txt846 bytesdamiankloip
#3 hidden2.png22.77 KBdawehner
#1 hidden.png55.76 KBdawehner

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
55.76 KB
819 bytes
FAILED: [[SimpleTest]]: [MySQL] 47,865 pass(es), 2 fail(s), and 0 exception(s). View

Let's do that, unless the user wants to see the master display all the time.

Bojhan’s picture

A side from the clear visual bug, which seems to be from another issue. Just hiding this functionality when there are no other displays, or master display sounds sensible.

dawehner’s picture

FileSize
22.77 KB

Oh right, i worked on two issues at the same time.

For clarity, when you have hidden this field, the button will be called "Apply" all the time.

Status: Needs review » Needs work

The last submitted patch, drupal-1836384-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
PASSED: [[SimpleTest]]: [MySQL] 50,081 pass(es). View

Let's fix the test.

renat’s picture

It would be nice to have a possibility to show this option even if there is just one display (probably admin/structure/views/settings is a right place for appropriate switch). It is useful in case you know other displays will be added in the future, using it you can decide what should be cloned to this displays, and what should not.

In principle it should be possible with Master display, though... Would be interesting to hear a word from other people about their workflows in such cases.

dawehner’s picture

It would be nice to have a possibility to show this option even if there is just one display (probably admin/structure/views/settings is a right place for appropriate switch). It is useful in case you know other displays will be added in the future, using it you can decide what should be cloned to this displays, and what should not.

Another classical example that you can't oversimplify what people are doing with views.

dawehner’s picture

Issue tags: -Usability, -d8ux, -VDC, -BADCamp2012UX

#5: drupal-1836384-5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1836384-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#5: drupal-1836384-5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1836384-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#5: drupal-1836384-5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-1836384-5.patch, failed testing.

dawehner’s picture

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

#5: drupal-1836384-5.patch queued for re-testing.

Bojhan’s picture

Issue tags: +sprint

tagging

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.58 KB
FAILED: [[SimpleTest]]: [MySQL] 55,904 pass(es), 1 fail(s), and 0 exception(s). View

rerolled, looks good and works fine.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1836384-16.patch, failed testing.

Bojhan’s picture

@damain could you reroll this?

damiankloip’s picture

Yep, sure.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
846 bytes
2.11 KB
PASSED: [[SimpleTest]]: [MySQL] 59,898 pass(es). View

Patch applied ok, this should fix the test failure.

klonos’s picture

Issue summary: View changes
Related issues: +#1836392: In the Views UI, the interaction pattern of “All displays”/ “Override this display” is confusing
jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
jain_deepak’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,743 pass(es), 44 fail(s), and 20 exception(s). View

Rerolled

Status: Needs review » Needs work

The last submitted patch, 23: 1836384-23.patch, failed testing.

gaurav_varshney’s picture

Status: Needs work » Needs review
Issue tags: +SprintWeekend2015
FileSize
2.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,833 pass(es), 44 fail(s), and 20 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 25: 1836384-25.patch, failed testing.

dawehner’s picture

Now we just have to fix the failures :)

anil280988’s picture

FileSize
2.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,816 pass(es), 44 fail(s), and 20 exception(s). View

Rerolled

anil280988’s picture

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

Status: Needs review » Needs work

The last submitted patch, 28: 1836384-26.patch, failed testing.

tadityar’s picture

Status: Needs work » Needs review
FileSize
2.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,913 pass(es), 51 fail(s), and 24 exception(s). View
997 bytes

Trying to pass.

Status: Needs review » Needs work

The last submitted patch, 31: 1836384-31.patch, failed testing.

hynnot’s picture

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

Needs review.

alimac’s picture

Issue tags: -sprintweekendDelhi

Removing location tag -- there was a discussion (see 2407325#comment-9513533 and decided to use one tag to keep the autocomplete list of tags short.

Status: Needs review » Needs work

The last submitted patch, 33: 1836384-33.patch, failed testing.

hynnot’s picture

Status: Needs work » Needs review
FileSize
2.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,988 pass(es). View

Needs review.

dawehner’s picture

+++ b/core/modules/views_ui/admin.inc
@@ -312,6 +312,13 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta
+  }

You could show that in just a single line, right? '#access'] = !\Drupal::config('views.settings')->get('ui.show.master_display') && count($displays) > 2;

manningpete’s picture

Issue tags: -Needs reroll

Patch applies.

tadityar’s picture

@dawehner, isn't that an if statement?

Lendude’s picture

FileSize
656 bytes
2.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,930 pass(es), 9 fail(s), and 6 exception(s). View

I agree with @tadiyar, it's an IF statement so it should be in curly brackets.

Could I add one more twist to this? Only remove the field when the section is not currently overridden.

Scenario:
- Add View
- Add two Pages
- Override the Field section in Page 2
- Delete Page 1

With the patch in #36 the user would now NOT get the option to revert the Field section in Page 2 back to default (because there is only one display left).

So add an additional check to see if the current section in overridden, and if so. don't hide the field.

Status: Needs review » Needs work

The last submitted patch, 40: 1836384-40.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
2.44 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 1836384-42.patch. Unable to apply patch. See the log in the details link for more information. View

The failed tests show another issue with the fix in #36. Setting #access to FALSE actually translates to 'overridden' and not 'default'.
getOverrideValues() only checks if a value is set, and doesn't check #access. So when adding anything in the UI you will end up with a section that no longer uses the defaults.

The new patch does the check earlier and doesn't add the override field at all, like it is done for the default display.

Interdiff is against #36

Let's see what this does...

Lendude queued 42: 1836384-42.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 42: 1836384-42.patch, failed testing.

pguillard’s picture

Status: Needs work » Needs review
FileSize
2.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,422 pass(es), 2 fail(s), and 0 exception(s). View

A light re-roll from #42...

Status: Needs review » Needs work

The last submitted patch, 45: 1836384-45.patch, failed testing.

The last submitted patch, 45: 1836384-45.patch, failed testing.

dawehner’s picture

@pguillard Oh cool, thank you for rerolling this patch. I

+++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
index 6170ed4..fd0434f 100644
--- a/core/modules/views_ui/admin.inc

--- a/core/modules/views_ui/admin.inc
+++ b/core/modules/views_ui/admin.inc

+++ b/core/modules/views_ui/admin.inc
+++ b/core/modules/views_ui/admin.inc
@@ -230,7 +230,11 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta

@@ -230,7 +230,11 @@ function views_ui_standard_display_dropdown(&$form, FormStateInterface $form_sta
     $form['progress']['#weight'] = -1001;
   }

I think we need some additional testing for the new behaviour. Maybe in \Drupal\views_ui\Tests\DisplayTest we already have some tests about the dropdown

pguillard’s picture

Status: Needs work » Needs review
FileSize
1.87 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,731 pass(es), 1 fail(s), and 0 exception(s). View
687 bytes

Cool

I guess we have dropdown tests :

// Test that the override element is only displayed on pager plugin selection form.
$this->drupalGet('admin/structure/views/nojs/display/test_store_pager_settings/page_1/pager');
$this->assertFieldByName('override[dropdown]', 'page_1', 'The override element is displayed on plugin selection form.');
$this->drupalGet('admin/structure/views/nojs/display/test_store_pager_settings/page_1/pager_options');
$this->assertNoFieldByName('override[dropdown]', NULL, 'The override element is not displayed on plugin settings form.');

I just corrected one test, I don't know for the remaining error!

pguillard’s picture

to not consider..

Status: Needs review » Needs work

The last submitted patch, 49: 1836384-49.patch, failed testing.

The last submitted patch, 49: 1836384-49.patch, failed testing.

Lendude’s picture

+++ b/core/modules/views/src/Tests/Plugin/PagerTest.php
@@ -71,7 +71,7 @@
-    $this->drupalPostForm('admin/structure/views/nojs/display/test_store_pager_settings/default/pager', $edit, t('Apply'));
+    $this->drupalPostForm('admin/structure/views/nojs/display/test_view/default/pager', $edit, t('Apply'));

The change in the interdiff in #49 doesn't appear to be in the patch? Am I missing something?

pguillard’s picture

@Lendude : You are right, that line was braking the tests, it was mentioning test_store_pager_settings before we get the view.
Feel free to take control!

Lendude’s picture

Status: Needs work » Needs review
pguillard’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

I guess this is RTBC !

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 55: 1836384-55.patch, failed testing.

pguillard’s picture

Status: Needs work » Needs review
pguillard’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC...

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I might be wrong but I can't see any tests confirming that the "all Displays" option is not displayed as expected.

Lendude’s picture

@alexpott you are absolutely right.

Added tests and removed unrelated extra newline.

pguillard’s picture

pguillard’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
10.18 KB
5.41 KB

I guess this is RTBC.

Before patch :
Before

After patch :
After

dawehner’s picture

+1

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed c536cbf and pushed to 8.1.x and 8.2.x - as a task this is not eligible for 8.0.x but this is safe for 8.1.x-beta1. Thanks!

  • alexpott committed a888a69 on 8.2.x
    Issue #1836384 by Lendude, pguillard, dawehner, damiankloip, hynnot,...

  • alexpott committed c536cbf on 8.1.x
    Issue #1836384 by Lendude, pguillard, dawehner, damiankloip, hynnot,...

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks, removing from UX sprint now.