Problem/Motivation

This issue is created to solve #2981044: Unify the grid/table views of the media library in a more generic way. There are a lot of situations where site builders would like an alternative way to display the current view results, specially when presenting content in a very visual way with a large thumbnails. One example is displaying a grid view and table view of the same content.

Proposed resolution

Create a new area plugin where you can link view displays together. We only allow a display to be linked if:

  • It is a page display
  • It has the same filter criteria
  • It has the same sort criteria
  • It has the same contextual filters
  • It has the same pager settings

We render links to the other displays in the view area and pass the exposed input/arguments/pager when a link is clicked. This would mean you get the exact same result, but showed differently.

Remaining tasks

  • Write patch
  • Code review
  • Usability review
  • Commit

User interface changes

New area plugin for display links
New area plugin for display links.

Display link form when there are no allowed displays in the view
Display link form when there are no allowed displays in the view.

Display link form
Display link form.

Grid view
Grid view with links

Table view
Table view with links

API changes

-

Data model changes

-

Release notes snippet

Added a new Views area plugin, which displays a link to another display of the same view while keeping the filters, sorts, contextual filters, and pager configured.

CommentFileSizeAuthor
#78 3025657-78.patch29.85 KBseanB
#78 interdiff-71-78.txt2.37 KBseanB
#71 3025657-71.patch29.88 KBseanB
#71 interdiff-67-71.txt4.42 KBseanB
#67 3025657-67.patch27.57 KBseanB
#67 interdiff-65-67.txt1.12 KBseanB
#65 3025657-65.patch27.57 KBseanB
#65 interdiff-61-65.txt2.33 KBseanB
#61 3025657-60.patch27.56 KBseanB
#61 interdiff-58-60.txt28.26 KBseanB
#58 interdiff-53-58.txt6.63 KBgbirch
#58 interdiff-49-58.txt10.84 KBgbirch
#58 3025657-update-tests-58.patch26.46 KBgbirch
#55 interdiff-49-53.txt4.21 KBgbirch
#54 3025657-53.patch25.51 KBgbirch
#51 area-plugin-form.png159.86 KBbenjifisher
#51 area-plugin.png167.7 KBbenjifisher
#49 3025657-49.patch24.24 KBseanB
#49 interdiff-47-49.txt5.77 KBseanB
#47 interdiff-42-47.txt38.71 KBseanB
#47 3025657-47.patch23.51 KBseanB
#43 3025657-43-warning-wall.png153.39 KBdww
#42 3025657-35_42.interdiff.txt1.4 KBdww
#42 3025657-42.patch37.01 KBdww
#39 area-plugin-form.png186.43 KBseanB
#39 area-plugin-form-empty.png153.15 KBseanB
#39 area-plugin.png217.07 KBseanB
#35 3025657-33_35.interdiff.txt2.17 KBdww
#35 3025657-35.patch37.02 KBdww
#33 interdiff-32-33.txt1.33 KBseanB
#33 3025657-33.patch37.1 KBseanB
#32 interdiff-29-32.txt2.74 KBseanB
#32 3025657-32.patch37.11 KBseanB
#29 interdiff-26-29.txt13.84 KBseanB
#29 3025657-29.patch36.91 KBseanB
#26 interdiff-25-26.txt510 bytesseanB
#26 3025657-26.patch36.79 KBseanB
#25 interdiff-23-25.txt2.03 KBseanB
#25 3025657-25.patch36.78 KBseanB
#23 interdiff-21-23.txt885 bytesseanB
#23 3025657-23.patch36.53 KBseanB
#21 interdiff-16-21.txt16.95 KBseanB
#21 3025657-21.patch35.67 KBseanB
#16 interdiff-12-16.txt19.01 KBseanB
#16 3025657-16.patch36.06 KBseanB
#16 views-edit-form.png247.37 KBseanB
#12 views-table.png502.81 KBseanB
#12 views-grid.png1.86 MBseanB
#12 interdiff-5-12.txt29.76 KBseanB
#12 3025657-12.patch33.93 KBseanB
#5 interdiff-2-5.txt35.02 KBseanB
#5 3025657-5.patch32.52 KBseanB
#2 3025657-2.patch23.59 KBseanB
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanB created an issue. See original summary.

seanB’s picture

Status: Active » Needs review
FileSize
23.59 KB

Patch is attached. Please review!

Lendude’s picture

Status: Needs review » Needs work

Looks really nice already! Love the test coverage. Quick first review.

  1. +++ b/core/modules/views/config/schema/views.area.schema.yml
    @@ -77,3 +77,17 @@ views.area.http_status_code:
    +      type: string
    

    should be type label to make this translatable in config.

  2. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,157 @@
    +        continue;
    

    Why continue? Can there only be one display that is not allowed?

  3. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,157 @@
    +    if ((!$empty || !empty($this->options['empty'])) && !empty($this->options['display'])) {
    

    make this an early return instead of the method one giant nested if?

  4. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,157 @@
    +    if ($loaded_display->getPluginId() !== 'page') {
    +      return FALSE;
    +    }
    

    We'd have greater flexibility if we would check if it extends \Drupal\views\Plugin\views\display\PathPluginBase so we also capture custom display types that people might use. But probably better as a 'nice to have'.

  5. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,381 @@
    +  public static $modules = ['system', 'user', 'filter'];
    

    missing docblock and should be protected

  6. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,381 @@
    +  protected function setUp($import_test_views = TRUE) {
    

    missing docblock

  7. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,381 @@
    +    $this->assertDisplayLinksNotRendered($view);
    +    ¶
    +    // Assert the links are rendered again when the filters are the same.
    

    white space

  8. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,381 @@
    +    // Assert the links are rendered again when the filters are the same.
    +    $page_2->overrideOption('pager', $pager);
    

    filters => 'pager is the same'

  9. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,381 @@
    +    // Assert the links are rendered again when the filters are the same.
    

    filters => arguments.

  10. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,381 @@
    +    $page_2_query_args = !empty($page_2_args) ? '/' . implode('/', $page_2_args) : '';
    +    ¶
    +    $view->setDisplay('page_1');
    

    white space

  11. +++ b/core/modules/views/views.views.inc
    @@ -129,6 +129,14 @@ function views_views_data() {
    +    'help' => t('Displays a link to another display of this view to present the same results in a different way, eg. a grid and table view'),
    

    This implies that this specific use is the only use for this and has some sort of special handling for switching displays which this doesn't have. Maybe add a 'for example' in there somewhere?

Lendude’s picture

One more

+++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
@@ -0,0 +1,157 @@
+    if ($loaded_display->getPluginId() !== 'page') {
+      return FALSE;
+    }

I don't see test coverage for this if, am I overlooking it, or is it missing?

seanB’s picture

Status: Needs work » Needs review
FileSize
32.52 KB
35.02 KB

Thanks @lendude!

#3
1. Fixed.
2. Continue only ends the current iteration so the next display is checked. There can be multiple allowed or disallowed displays.
3. Fixed.
4. Fixed.
5. Fixed. Can't be protected though. AreaDisplayLinkTest::$modules must be public (as in class Drupal\Tests\views\Kernel\ViewsKernelTestBase)
6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.
10. Fixed.
11. Changed the text a bit to remove the example since I could see how it might be confusing.

#4
1. Fixed. You are right, added it. Refactored the tests a bit to make it more clear we have assertions around validation, form options and rendering.

starshaped’s picture

The patch looks great!

A suggestion: I'd rename the CSS class that is created for the link. Instead of 'display_link' I'd call it 'views-display-link' to keep it within the Views namespace.

Another idea: Default icons! If you are switching between, say, a table view and a grid view, maybe there's a way to add a 'list' icon to the list link and a 'grid' icon to the grid link. I'm not sure if that can be done programmatically, but it would be pretty neat.

phenaproxima’s picture

Issue tags: +Needs followup

Another idea: Default icons! If you are switching between, say, a table view and a grid view, maybe there's a way to add a 'list' icon to the list link and a 'grid' icon to the grid link. I'm not sure if that can be done programmatically, but it would be pretty neat.

+1 for this. It can definitely be done programmatically, but I'm not sure it's in scope for this patch. Let's open a follow-up for that!

Lendude’s picture

Just mostly nitpicks left from me.

  1. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,165 @@
    +      if (!$this->isAllowedDisplay($display_id)) {
    +        unset($displays[$display_id]);
    +        continue;
    +      }
    +      $allowed_displays[$display_id] = $display['display_title'];
    

    Ah now I get it, it's just an if-else where the else is a continue :)

  2. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,165 @@
    +    // Check if there is a link between 2 page displays that will not be
    

    2 => two

  3. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,165 @@
    +    if ($this->displayHandler instanceof PathPluginBase && !$this->isAllowedDisplay($this->options['display'])) {
    

    is this right? If I manage to link it to a block display, this if will not trigger and it will pass validation, right?

  4. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,165 @@
    +   * Check is a views display is allowed to be linked.
    

    Check is => Check if

  5. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,583 @@
    +    // Add 2 page displays and a block display.
    

    2 => two

  6. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,583 @@
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    ...
    +    $this->container->get('router.builder')->rebuild();
    

    Do we really need to rebuild the router every time in the test?

  7. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,583 @@
    +    // Assert the links are not rendered when 1 of the displays has a different
    

    1 => one

MrMason’s picture

I have the tested functionality and it works as expected. The only issue that I can see is that if both displays have a filter, pager, etc... overridden but they have the same value the area will throw an error. It currently seems to only check that they're both not overridden and does not check if they have the same values.

phenaproxima’s picture

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

Sounds like a thing we need tests for. Thanks, MrMason!

xjm’s picture

Priority: Normal » Major

Bumping to major feature given the impact on Media Library. Sounds like a nice feature!

seanB’s picture

Status: Needs work » Needs review
FileSize
33.93 KB
29.76 KB
1.86 MB
502.81 KB

New patch is attached, also added some screenshots to show a grid/table example where the display links are added to the views header.

Grid view
Grid view with links

Table view
Table view with links

So what is changed in the patch:

#6. Changed the classnames, thanks! Regarding the icons, let me start by saying for the grid/table use-case I very much agree we should use them. Even though grid/table displays might be a common use-case, but we are intentionally trying to solve this in a generic way. We could add icons to the core themes for views that are shipping with grid/table displays (the Media library will definitely use icons in #2981044: Unify the grid/table views of the media library). To me it feels icons are a theming thing though, not something that should be added to the views module. Once the icons are added in #2981044: Unify the grid/table views of the media library, it will be easier for other views to adopt this.

#7.1 :)
#7.2 Fixed.
#7.3 Fixed. Blocks could inherit the header area from the default display, so we can't really stop block displays from having a display link. Since we never display the links on the block I thought it was a good idea to stop validation for it. But I guess validating if the display's are the same is still a good idea.
#7.4 Fixed.
#7.5 Fixed.
#7.6 Fixed.
#7.7 Fixed.

#9 I can't reproduce the issue you are describing? Do you maybe have some steps to reproduce this? Or maybe some more information from the error logs? I did change the tests a bit to start with default filters/sorts/pager/argument settings and explicitly override them.

Other changes:
As mentioned before, blocks could inherit display links from the default display. After talking to Lendude some more we decided that hiding those links on the block display is "magic", and probably confusing for users. This has been changed in the patch. You can now link to a path display from anywhere and the link would show up. You can still link to path based displays only. If users want to remove the links on a display they can override the area.

Added some space between display links and added an is-active class for the current display (in case the page contains a link to itself).

Status: Needs review » Needs work

The last submitted patch, 12: 3025657-12.patch, failed testing. View results

seanB’s picture

Status: Needs work » Needs review

Patch seems to be green, not sure what happened. Please review.

phenaproxima’s picture

I have a major concern with this patch as-is, and I discussed it in Slack with @seanB.

Here's my beef: if we are making only the link text and CSS classes configurable, we are limiting the theme-ability of the link. Adding icons or additional attributes will become tricker, and possibly necessitate future configuration updates -- which are nightmarish where Views is concerned.

So, I suggest we change our approach a bit here. Let's change the plugin so that it stores only three things:

  • Target display ID
  • Text of the link. This is optional and should not allow HTML, and it should ideally default to the name/title of the target display.
  • An inline Twig template for the link. It should receive the following variables:
    • An attributes object with some default CSS classes (e.g., reflecting the target display ID, whether the current display is the active one, etc.)
    • The current display ID
    • The target display ID
    • The URL of the target display (i.e., the URL of the link)
    • The configured link text, or the name/title of the target display if no text is configured

Advantages of doing it this way:

  • It's a totally sane default
  • The fact that it's Twig provides a huge amount of flexibility for site builders and is very simple to change (just edit the view)
  • It greatly reduces the chance of us needing to add additional configuration switches in the future, which means we don't have to worry about update paths
  • It doesn't introduce any new theme hooks or templates, which means there are no changes to the front-end framework or complex integration with the theme system. But if we want to add something later, we still have that option
  • There is precedent for this in Views -- field plugins also allow inline Twig templates ("Rewrite the output of this field")
seanB’s picture

Implemented the inline twig template field for the plugin (like already available in fields).

One thing though:

It doesn't introduce any new theme hooks or templates, which means there are no changes to the front-end framework or complex integration with the theme system. But if we want to add something later, we still have that option

If we would want to add a twig template later, the inline template text becomes obsolete and we would have to provide an upgrade path for that right? I think that could be very painful. Maybe adding an actual template now would be a better choice? Even though there is precedent in views, we don't have to use that pattern. I personally don't like that we are storing theming in config, but I would love to hear what other people think.

The edit form now looks like this:
Display link area edit form.

Status: Needs review » Needs work

The last submitted patch, 16: 3025657-16.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
Lendude’s picture

Discussed with @seanB and I agree with his statement:

Maybe adding an actual template now would be a better choice? Even though there is precedent in views, we don't have to use that pattern. I personally don't like that we are storing theming in config, but I would love to hear what other people think.

I'm not a fan of the inline twig feature either, and neither are the front-enders I've worked with. It might be nice for site builders to use inline twig, but for site maintainability and separation of responsibilities nothing beats a template (I feel...).

dww’s picture

Status: Needs review » Needs work

+1 to #16 and #19. I think a real template is better than inline for all the reasons already stated. Plus, if I'm using this feature on a site, it's entirely likely I'll want to reuse it. I'll want my site's specific markup, icons, styles, etc to live in a shared template, not duplicated in config across different views.

Thanks,
-Derek

seanB’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
35.67 KB
16.95 KB

New patch to implement a template file views-view-display-link.html.twig.

Status: Needs review » Needs work

The last submitted patch, 21: 3025657-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
FileSize
36.53 KB
885 bytes

Added the template to stable as well.

Lendude’s picture

Looking great, just some last nitpicks left for me:

  1. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,147 @@
    +      $this->t('It is a page display.'),
    

    page display => display with a path

  2. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,147 @@
    +      '#options' => $allowed_displays,
    

    do we want to do something if there are no allowed displays? Show a message (extend $message?) instead of the form options?

seanB’s picture

seanB’s picture

The last submitted patch, 25: 3025657-25.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 26: 3025657-26.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
FileSize
36.91 KB
13.84 KB

Added newlines to twig files because of coding standards. Thanks Lendude for pointing this out. Adjusted the tests accordingly.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

forgot earlier but @phenaproxima++ for raising the issue of themability, spot on.

@seanB++ for the implementation

I got nothing to nitpick anymore, RTBC from me.

dww’s picture

Status: Reviewed & tested by the community » Needs work

Yes, looking great! +1 to all the ++'s in #30. ;)

Only very minor nits from me:

1. + * - attributes: HTML attributes for the containing element.

Given #2917653: toolbar_preprocess_html() converts attributes from array to Attribute object and friends, and other general confusion on if theme template attributes are an array or an object, can we clearly document that for this particular template the link attributes are being passed in as an Attribute object? Also, the "containing element" is an <a> and will always be, right? Something like this:

 * - attributes: HTML attributes for the link element, instance of \Drupal\Core\Template\Attribute

(or something)

2. core/modules/views/views.theme.inc:
+ * - url: A URl object for the display link.
s/URl/URL/

3. From the pedantic documentation department...
+ * - view: The view that the area belongs to.
I think we mean a \Drupal\views\ViewExecutable object, but if I didn't already know, I might think it was a view ID or whatever. A few lines later, we do the kind thing and tell them explicitly:
+ * - url: The url of the display link, instance of \Drupal\Core\Url.
Can we do the same for the view? Something like:
* - view: The view that the area belongs to, instance of \Drupal\views\ViewExecutable.

4. While we're at it, s/url/URL/ in prose (it's an acronym, after all), e.g.:

* - url: The URL of the display link, instance of \Drupal\Core\Url.

Thanks!
-Derek

seanB’s picture

Status: Needs work » Needs review
FileSize
37.11 KB
2.74 KB

No problem, I think the nits make sense. Fixed #31.

seanB’s picture

Arghh, stupid typo. Sorry about that.

The last submitted patch, 32: 3025657-32.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dww’s picture

@seanB: Thanks for #32/33!

Here I'm harvesting the most concise yet precise docs for each argument, then using them consistently in all the places the template / preprocess is defined.

I've got no other nits, but I guess I shouldn't RTBC now...

jibran’s picture

Issue tags: +Needs usability review

This is an exciting new feature and some great work. Let's have a usability review before we set it to RTBC. Can we please update the issue summary with the latest screenshots?

dww’s picture

I don't want to hold this up, but I want to raise a use-case for non-identical arguments/pager, etc.

I'm working on a project right now that could sort of use this, but it can't because of the assumptions / tests / design around "only link to an "equivalent" display" in terms of pager, args, filters, etc. I've got events, and an "international directory" of instructors and practitioners of a given tradition. Both of these can be visualized in different ways. The international directory has a map view, and a "Search" view. Honestly, the search could be further split into table vs. grid. However, the map always shows all results, while the search is using a pager. The events can similarly be seen in search vs. map vs. calendar displays, and while the calendar is technically "show all", the arguments restrict it to a given month, etc.

I know the intention of this feature is "let people quickly toggle the display of data without ever seeing different results", and in some cases, that's what you want (e.g. the parent issue where this idea was first proposed for the media library). But in other cases, site builders will want to link displays, even if there are different results in each "view". Can we do anything about this? Can we make the enforcement of "equivalent displays" itself an option for this plugin? Let site builders use this in more cases, so long as they opt-in to a setting where they make this choice consciously? In short, do we have to enforce all the criteria spelled out in the summary to use the feature at all? Can we simply detect that there might be different results in a given configuration and warn/advise the site builder accordingly?

As it is, I'm going to have to continue to register menu tabs for these different displays, and use that to "link" them. That's not the end of the world, but I was hoping for something else. This feature almost gives me another option, but sadly comes up short in its quest to know how I should use the feature better than I do. ;)

Thoughts?

Thanks!
-Derek

phenaproxima’s picture

@dww, I think that sounds like a very valid use case, but it will likely introduce complicated usability questions and other angles that need to be discussed and resolved. As it stands, this issue is a secondary stability blocker for the Media Library module (which is due in 8.7), so I think we should continue with the patch as-is for now, and defer your excellent point to a follow-up issue.

seanB’s picture

Issue summary: View changes
FileSize
217.07 KB
153.15 KB
186.43 KB

#36 Added screenshots to the IS for easier review.

#37
@dww We could have a followup to make that configurable. So by default all arguments, filters etc are passed, but if needed you could ignore the arguments, pager or filters/sorts by selecting a checkbox or something? It does get really easy for people to make mistakes, which could lead to a lot of support request of people not knowing why the results are different after clicking the links. So I'm not really sure about this yet.

Like:
▢ Ignore filter / sort criteria
▢ Ignore pager
▢ Ignore contextual filters

seanB’s picture

Issue summary: View changes
phenaproxima’s picture

Issue summary: View changes

Adding release notes snippet to the IS.

dww’s picture

Re :#38 and #39: For all the reasons stated above why changing this later is going to be a PITA (and will probably never happen), I'm reluctant to plow forward with "we must get this in now For The Love of Media(tm)" if we can do anything about it before it's too late.

Possible options:

a) Completely rip out all the criteria and enforcement code, complication, text-that-must-be-translated, tests, etc. Let site builders use this as they see fit. Perhaps add a single sentence when selecting the display to link to reminding site builders to beware that end users will see different results if all the relevant settings don't match. In my use case, there's no harm in sending over query args that are ignored by another display (e.g. a page arg on a display showing all results). It'd make this a *much* smaller patch, with *far* fewer strings to translate, and would potentially be more useful to more site builders. Yes, people can misconfigure a view and get weird results. I'm shocked! ;)

b) Rename this thing something like "Equivalent display link" so we could add a more general one later but we don't have to migrate it.

c) Add (something like) the settings @seanB proposes in #39.

Maybe there are other solutions we could explore? I'm open to spending a little more time on this to help move it along if we can make it something that could be used beyond Media Library.

---

Meanwhile, I fixed 2 more nits in the user-facing text:

1. "... we only allow a display to be selected when:"

The Drupal UI never says "we" like this. Who's the "we"? @phenaproxima and @seanB? ;) Mean Drupal devs who know how this feature should work better than you do! ;) (j/k) Anyway, I rewrote this to:

"... you many only select a display when:"

2. "Displays a link to another display of this view while keeping the filters, sorts, page and arguments to display the same results in a different way."

All kinds of problems here. ;)

2A. "Arguments" is not user-facing in the Views UI anymore. Those are now "contextual filters". But that's just another form of filter, and for the purposes of the help text about this area plugin, I don't think anyone needs to get too bogged down in those details. Let's just refer to all of that as the "filters" and be done.

2B. It's not just "page" but really "pager options", right? E.g. if the view exposes how many items appear on a page, etc. (Does that all work and do we test for it?).

2C. The sentence uses "display" 3 times (with 2 different meanings). :/

How about this?

Displays a link to another display of this view while keeping the filters, sorts and pager options to show the same results in a different way.

I'd rather this:

Renders a link to another display of this view while keeping the filters, sorts and pager options to show the same results in a different way.

but that's inconsistent with all the other area plugin descriptions that start with "Displays...". :/ Maybe to avoid display vs. display weirdness, we can break the convention and go with my preferred text? What's the least evil choice here? Using "display" with 2 different meanings, or breaking the wall of "Displays..." at the front of all the area help texts?

dww’s picture

FileSize
153.39 KB

More thoughts on the equivalence criteria after actually clicking this together locally.

- Inside isAllowedDisplay() we go through all the trouble to enforce equivalence between the target and $this->displayHandler, but that's for an header area handler that by default applies to all displays. You have to go out of your way to remember you only want this link on the display that we did all the trouble to "enforce" equivalence on. :(

- In general, it seems we *intend* for a given view to have the same links shared across all the headers (hence the effort of detecting if we're on ourselves and setting the is-active class).

- So frankly, this whole "enforcement" thing seems like a bit of theater. You're checking vs. "current" but then the results apply everywhere.

- Given these insights, I tried to break a view using the patch. I have a page, table and grid display. Everything is using the same settings. I added 3 links to the header area, one for each display. So far so good. Then, I went to the "grid" display, and tried to change something (the sort order). Bravo for actually detecting this and giving me a bunch of warnings!

Wall of warnings with a changed sort order

However, bummer that I'm now in a weird hell of invalidity. If I try to recover, I have to be really careful about when to override things for a specific display, when to revert to defaults, etc. And that's just if I try to fix the links themselves. The *real* solution to my problem is to figure out the sort order is different (which this code could know and tell me, but it doesn't).

I'm honestly not sure all these enforcement criteria are going to save support requests. If anything, we're going to get a ton of requests from people trying to figure out how to use it. I think we're better off if we massively simplify, and punt to the site builders: "if you use this to link to displays with different settings, end users might see different results as they navigate." (approx).

Yes, *we* can be careful about this when we use this feature in core. *We* can decide we want grid vs. table to show the same results in different ways. Fine. But forcing this feature to enforce that both makes it less useful in other cases, and a lot more complicated to configure your views.

I'm not going to "needs work" this yet, as I'm probably in the minority and going to be overruled. But I'm raising now in case we can still consider solving this.

Thanks,
-Derek

p.s. In #42 I added the "String change in 8.7.0" tag since Gabor put that on my patch for #3025839: Change toolbar's permission label to 'Use the toolbar'. This is adding new strings, not changing existing ones, but either way, it seems like a kind thing to attempt to alert the translation teams about these new strings.

phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: +Needs product manager review

I'm tagging in the product managers to consider @dww's points and see if we should either proceed as-is, or rip out the validation stuff.

  1. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,154 @@
    +    $message = $this->t('To make sure the results are the same when switching to the other display, you many only select a display when:');
    

    Can we rephrase the ending here: "...you may only select a display which:"

  2. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,154 @@
    +      $this->t('It is a display with a path.'),
    +      $this->t('It has the same filter criteria as the current display.'),
    +      $this->t('It has the same sort criteria as the current display.'),
    +      $this->t('It has the same contextual filters as the current display.'),
    +      $this->t('It has the same pager settings as the current display.'),
    

    And let's bring these in line with that change. "Has a path", "Has the same filter critiera", etc.

  3. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,154 @@
    +        '#markup' => '<p><em>' . $this->t('There are currently no displays available that meet the criteria.') . '</em></p>',
    

    I don't think we need "currently".

  4. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,154 @@
    +      $form['label'] = [
    +        '#title' => $this->t('Label'),
    +        '#type' => 'textfield',
    +        '#default_value' => $this->options['label'],
    +        '#required' => TRUE,
    +      ];
    

    Let's add a description here. "The text of the link". Also, let's say whether or not HTML is allowed in here (I vote we _do_ allow it, and if we do, we'll probably want to make sure the tests cover that.)

webchick’s picture

@phenaproxima brought this up on the cross-initiative meeting, with both @Gábor Hojtsy and myself in attendance.

From a product POV, it is a bit strange for Views to be putting up "guard rails" around any particular capability. Views gives you not just a gun to shoot yourself in the foot, but a full-blown bazooka with flame thrower attachment. ;) That's kind of its charm, really.

It sounds like in a worst-case scenario here, where say a site builder has configured a switch between a display with exposed filters that are not present on a different display it's linking to, the end user could experience weird validation errors they won't understand. That is indeed a problem, but it seems like it's a problem for the site builder to sort out, vs. Views itself.

A more compatible approach might be to detect this situation once the link is created in the Views UI, and show the site builder a yellow warning dealie in the interface, similar to what we do with node displays that don't have the "published" filter applied. Like "Hey, you can do this, but it's probably a bad idea... are you sure you meant to do that?" But don't explicitly stop site builders from the behaviour.

seanB’s picture

Assigned: Unassigned » seanB

Thanks everyone. Will start on this tonight!

seanB’s picture

Status: Needs work » Needs review
FileSize
23.51 KB
38.71 KB

Ok here we go. Replaced the error messages with warnings, and changed the allowed displays to check only if a display is path based.
Refactored the tests accordingly.

Not sure what to do about the "warning wall" mentioned in #43. When you have a link from display A to display B and vise versa, the link in display A to B will add a warning, but the link from B to A will add a warning as well. If one of the links is set on the default display, that will also add a warning.
Not sure if we can simply remove that. Also thought about adding more specific warnings, but if you change the filters AND the sorts you will get twice the amount of warnings (more specific, but probably not more helpful).
Any thoughts on that?

dww’s picture

Status: Needs review » Needs work

Quick skim: looks great! Thanks!!

A few initial nits:

1.

+   * @return bool
+   *   Whether the display ID is an allowed display or not.
+   */
+  protected function hasEqualSettings($display_id) {
+

@return should be something like "Whether the display ID has equal settings to the current display."

Side note: does this API actually make sense? Should this function take 2 display_ids instead instead of always comparing against $this->displayHandler?

2.

+      $warning = $this->t('%display: The link in the %area area to the %linked_display display is invalid. Please check if the filter criteria, sort criteria, pager and contextual filters have the same settings.', [

I don't think it's fair to call such links "invalid". I think we need to be more welcoming here, but also more specific about what the problem would be. Something like:

$warning = $this->t('%display: The link in the %area area points to the %linked_display display which uses different settings than %current_display. Users clicking on these links might see different results on each page. To ensure users see the same results, check if the filter criteria, sort criteria, pager and contextual filters have the same settings.',

Or something. :/ That's too wordy, but in that direction...

3.

+    'help' => t('Displays a link to a path based display of this view while keeping the filter criteria, sort criteria and pager settings to show the same results in a different way.'),

No longer true. Sorry I can't wordsmith a better alternative right now (juggling too many things today!).

seanB’s picture

Status: Needs work » Needs review
FileSize
5.77 KB
24.24 KB

#48.1. Fixed. The return text was changed. This is an internal method for this class which always runs for the display being validated. Mostly extracted this for readability.

#48.2. Fixed. Totally agree, hopefully it is better now.

#48.3. I think this is still true? We keep all filter, sort, pager and contextual filters. Not sure if we should remove 'to show the same results in a different way', but since I think that is still what we intend to provide. We should probably keep that.

dawehner’s picture

I do like the idea, given it might get rid of the link displays of views itself in the future which were always an arbitrary added feature.

  1. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,169 @@
    +    $options['display'] = ['default' => NULL];
    

    ❓I'm wondering whether "display_id" would be a bit more explicit of what is stored.

  2. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,169 @@
    +    $message = $this->t('To make sure the results are the same when switching to the other display, it is recommended to make sure the display:');
    +    $criteria = [
    +      $this->t('Has a path.'),
    +      $this->t('Has the same filter criteria.'),
    +      $this->t('Has the same sort criteria.'),
    +      $this->t('Has the same pager settings.'),
    +      $this->t('Has the same contextual filters.'),
    +    ];
    +    $message .= '<ul><li>' . implode('</li><li>', $criteria) . '</li></ul>';
    

    I'm wondering why #theme item_list isn't used here instead.

  3. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,169 @@
    +    if (!$loaded_display instanceof PathPluginBase) {
    +      return FALSE;
    +    }
    +    return TRUE;
    

    This could be improved by using return $loaded_display instanceof PathPluginBase which is way more readable, IMHO.

  4. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    index 0000000000..d9406f7b80
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/views/templates/views-view-display-link.html.twig
    
    +++ b/core/modules/views/templates/views-view-display-link.html.twig
    +++ b/core/modules/views/templates/views-view-display-link.html.twig
    @@ -0,0 +1,19 @@
    
    @@ -0,0 +1,19 @@
    +{#
    +/**
    + * @file
    + * Default theme implementation for a views area display link.
    + *
    + * Available variables:
    + * - view: The parent view, instance of \Drupal\views\ViewExecutable.
    + * - target_display: The target display ID of the link display.
    + * - label: The label of the display link.
    + * - url: The URL of the display link, instance of \Drupal\Core\Url.
    + * - attributes: HTML attributes for the containing element, instance of
    + *   \Drupal\Core\Template\Attribute.
    + *
    + * @see template_preprocess_views_view_display_link()
    + *
    + * @ingroup themeable
    + */
    +#}
    +<a href="{{ url }}"{{ attributes }}>{{ label }}</a>
    

    Given this is pratically a #type link I'm wondering whether you could use '#type' => 'link', '#theme' => 'link__views_display_link', which falls back to '#theme' => 'link' by default. This is a pattern which is used by \Drupal\Core\Render\Element\Dropbutton::getInfo for example.

benjifisher’s picture

Issue summary: View changes
Issue tags: +ContributionWeekend2019
FileSize
167.7 KB
159.86 KB

I am starting to review this issue for usability at the ContributionWeekend2019 in Boston.

After reading through the discussion so far, I think that the issue summary, interface text, and screenshots need to be updated after the patches in #47 and #49. I have attached a couple of screenshots, using the Umami install profile, the Media view, and this patch. Maybe some of the remaining screenshots should also be updated.

Do we still need a follow-up? See #6, #7, #12. If not, then remove the issue tag "Needs followup".

Displays a link to a path based display of this view while keeping the filter criteria, sort criteria and pager settings to show the same results in a different way.

This is the current interface text, and I have a few objections:

  1. It should be "path-based".
  2. This seems redundant: "of this view ... the same results"
  3. This seems unnecessary: "in a different way"
  4. Is this still accurate after #47? "of this view"
  5. Is this still accurate after #47? "keeping ..."

I am making a grammatical fix to the Release notes snippet, but it may need more substantive revision for the changes made in #47.

gbirch’s picture

Duplicate error messages

If after creating links to page1 and page2 for all displays, you then change the settings on one display you get a validation error message for each display, including master and each display that uses master's settings. This would be confusing for a new-ish user. Ideally the messages would be limited to master and each display that overrides master's settings. Really ideally the error for Master would not be shown if Master is not being displayed in the views ui.
If you go on to change the settings on another display, you get a whole bunch of overlapping and repetitive validation messages.

Identify the area(s) in which the settings are not the same

Instead of returning TRUE or FALSE, hasEqualSettings() could return a list of the areas in which the settings differ, which would make for much more helpful error messages.

Behavior of contextual arguments is different from behavior of filters

As a note, and I'm not sure that one could or should do anything about this, but if display 1 has a filter that display 2 lacks, going from display 1 to display 2 and back again preserves the filter (e.g. ?status=1 from a display with an exposed published filter, to a display with no such filter - you would see only published stuff in display 1 no matter how many times you flip back and forth). In contrast, if display 1 has a contextual argument filter that display 2 lacks, going from display 1 with the argument set (path1/x) to display 2 (path2/x) will discard the argument, so when you go back to display 1 you are sent to path1, without the argument.

benjifisher’s picture

Answering one of my own questions (#51.4): I guess we are still limited to other displays of the same view.

What does "path-based" mean? I guess any display with a path. That would include page and feed, but not attachment, block, entity reference, nor embed.

When there is only one display, it would be nice if it were pre-selected in the select list. That may be an odd use case.

How do you get to the case where "there are no allowed displays in the view"? Isn't the current display always an option?

What is the behavior in the use case described in #37? That is, if pager settings are different, and I am looking at ?page=3 of one display, what happens when I follow the link? Do I still get ?page=3 (come what may) or do I get ?page=0 (or equivalent)? Maybe there should be a setting for this.

If I have several displays, and I want to provide links to all of them, then I have to add several "Link to display" areas. This is flexible, since I can override these for each display. But it is cumbersome. I would rather add a single "Menu of displays" than several "Link to display"s. Instead of selecting a single display, give me a column of checkboxes, one for each display. When I select one of them, I get options for that display (at least Label). I can imagine follow-up issues that add further options, such as

  1. a global option (or maybe one per display) to exclude the link to the current display
  2. default values for exposed and contextual filters
  3. a global option to display the links in a drop-down, an accordion, ...

The last one seems hard to implement given the current structure.

In fact, if we do get an option to set default values for filters, then we may want to use the same display more than once, with different defaults. Then we need something more flexible than my suggestion above ... but there are still advantages to having the links grouped.

gbirch’s picture

Patch to improve the validation along the lines suggested in the last comment.

gbirch’s picture

FileSize
4.21 KB

Added an interdiff.

Status: Needs review » Needs work

The last submitted patch, 54: 3025657-53.patch, failed testing. View results

benjifisher’s picture

At the ContributionWeekend2019 in Boston, @phenaproxima suggested that we might stick with one link per header item and then add a way to group header items in a separate issue or a contrib module.

From a usability point of view, I think that would work. If so, then we need a follow-up issue (even if we decide to implement it in contrib) for the grouping element, and we need some follow-up issues to add convenience features like a checkbox for "Display link to current page". See my comment #53 for some other features for follow-up issues.

From an implementation point of view, I am not sure what it would take. Do header elements have an option to "Exclude from display", like fields? If not, then we might have to add such an option in order to make the grouping option work smoothly. (I am thinking of another follow-up issue.) I think this would be flexible and scalable, and once the follow-up issues are created, this issue could pass usability review.

gbirch’s picture

Status: Needs work » Needs review
FileSize
26.46 KB
10.84 KB
6.63 KB

Updated the tests to reflect the improved warning messages. Includes interdiffs against #49 and my last patch.

seanB’s picture

  1. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +    // Do not add errors for the Master display if it is not displayed in the UI.
    

    This check makes sense. Nice! Supernit: Line is 81 characters.

  2. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +    if ($this->displayHandler->isDefaultDisplay()
    +      && !\Drupal::config('views.settings')->get('ui.show.master_display')) {
    

    I think these should be on the same line.

  3. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +    // This is suspenders and belt - Ajax errors can cause the plugin to be
    +    // added without any settings.
    +    $linked_display = $this->options['display'];
    +    if (!$linked_display) {
    +      return $errors;
    +    }
    

    I don't see something similar for other views plugins, but still: should we not show add an actual validation error in this case?

  4. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +    // Check if the linked display is a path-based display.
    +    // This should never happen, since the type of an existing display can't change.
    +    if (!$this->isPathBasedDisplay($linked_display)) {
    

    You could remove a path based display, save the view, and then edit again to add a block display with the same name. We should probably set an error when the linked display no longer exists as well.

  5. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +      $error = $this->t('%current_display: The link in the %area area points to the %linked_display display, which does not have a path to link to.', [
    

    We could probably lose the 'to link to'.

  6. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +    // Check if the linked display has the same options.
    +    // We "only" show a warning, because even though we recommend keeping the
    +    // display options equal, we do not want to enforce this.
    

    Nit: We should use the 80 characters per line.

  7. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +    $option_differences = $this->getDifferentOptions($linked_display);
    

    Maybe we should add a loop here, and call a hasEqualOptions() method that returns a bool instead of doing everything in getDifferentOptions. That might make this easier to extend/re-use.

  8. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +      $warning = $this->t('%current_display: The link in the %area area points to the %linked_display display which uses different settings than the %current_display display in the following area(s): %differences. To make sure users see the exact same result when clicking the link, please check that the settings in each area are the same.', [
    

    I think we can lose the "in each ares" in "please check that the settings in each area are the same".

  9. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -81,16 +81,42 @@
    +        '%differences' => implode('; ', $option_differences),
    

    Maybe "unequal_options" is a bit more descriptive then "differences"?

  10. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -152,18 +178,24 @@
    +   * @return array
    +   *   Array of areas in which the display ID has different options from the current display.
    ...
    +  protected function getDifferentOptions($display_id) {
    

    As mentioned above, can we change this method to return a bool and move the loop outside of the method?

  11. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -152,18 +178,24 @@
    +    $options = [
    +      'filters' => t('Filter Criteria'),
    +      'sorts' => t('Sort Criteria'),
    +      'pager' => t('Pager'),
    +      'arguments' => t('Advanced: Contextual Filters'),
    +    ];
    

    We can get the titles from Views::getHandlerTypes().

  12. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -294,18 +294,40 @@
    +   *   An array of areas in which differences are expected.
    

    An array of options that should be unequal? I kind of like equal/unequal better than same/different in this situation, but regardless of what we name it, the terminology should be consistent.

  13. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -294,18 +294,40 @@
    +    $options = [
    +      'filters' => t('Filter Criteria'),
    +      'sorts' => t('Sort Criteria'),
    +      'pager' => t('Pager'),
    +      'arguments' => t('Advanced: Contextual Filters'),
    +    ];
    

    Let's get the titles from Views::getHandlerTypes()

  14. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -294,18 +294,40 @@
    +    $difference_text = implode('; ', array_intersect_key($options, array_flip($differences)));
    

    Should we use a comma instead of a semicolon?

  15. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -294,18 +294,40 @@
    +    if ($config->get('ui.show.master_display')) {
    

    I think we should add a test with/without ui.show.master_display enabled and pass it as a parameter instead of getting the value from the view itself.

gbirch’s picture

@seanB Thanks for the careful review.
Re using Views::getHandlerTypes() - that function does not match the keys we are using to check for differences (e.g. 'filters' v 'filter'), does not contain a 'pager' key, and does not identify which settings are under the Advanced area (which I think we need to tell users, since those settings are hidden by default). Do you have another suggestion for getting the titles programmatically?

seanB’s picture

Issue summary: View changes
FileSize
28.26 KB
27.56 KB

#50
---------------------------------
1. Fixed.
2. Fixed.
3. Fixed.
4. Fixed.

#51
---------------------------------
I don't think we need a generic followup to provide default icons. These will be different for themes/usages of this feature. I think each time this will be used (for example the media library) the icons should be implemented by that specific issue.

Regarding the text updates:
1. Fixed.
2. I don't think it is redundant. Different displays of the same view do not necessarily show the same results.
3. This is debatable, but I think this emphasizes that the displays should differentiate in the way the results are displayed (eg. different format or fields).
4. Yes.
5. Yes.

Also updated the release notes:
Added a new Views area plugin, which displays a link to another display of the same view while keeping the filters, sorts, contextual filters, and pager configured.

#53

How do you get to the case where "there are no allowed displays in the view"? Isn't the current display always an option?

Create a view with only block displays.

What is the behavior in the use case described in #37?

We pass the current pager in the URL. So you would go to page 3 of the other display. If that display is configured to show all, or a fixed number of results, the pager is not used.

I would rather add a single "Menu of displays" than several "Link to display"s. Instead of selecting a single display, give me a column of checkboxes, one for each display.

We talked about this but decided against it because the plugin options would become a bit more complex, and you would lose flexibility. This is also not a thing that would change very often.

2. default values for exposed and contextual filters

The display links are supposed to keep all the filters, sorts, pager and contextual filter from the display the users is currently viewing, with the purpose of presenting the exact same results in a different way (eg. the grid/table display). Default filter values can be set on the filter, and should probably not be on the link.

a global option to display the links in a drop-down, an accordion, ...
The last one seems hard to implement given the current structure.

This would be an argument in favor of providing multiple links through a single plugin, but this can also be done via the theming system. So not really sure if we should do this.

#57
---------------------------------
Ah, great you had that talk. This can be done quite easily by overriding views-view.html.twig. For example:

  {% if header %}
    <div class="view-header">
      {{ header|without('display_link', 'display_link_1') }}
      <ul class="link-group">
        <li>{{ header['display_link'] }}</li>
        <li>{{ header['display_link_1'] }}</li>
      </ul>
    </div>
  {% endif %}

#59
---------------------------------
1. Fixed.
2. Fixed.
3. Fixed.
4. Fixed.
5. Fixed.
6. Fixed.
7. Fixed.
8. Fixed.
9. Fixed.
10. Fixed.
11. Didn't work, sorry about that. The actual option names are different and the pager is not in there as well.
12. Fixed.
13. See 11.
14. Fixed.
15. Added the extra tests.

Lendude’s picture

Issue tags: -Needs usability review

Seems like the usability review has been done and feedback has been addressed, so removing the tag for that.

Some extremely nitty nitpicks left:

  1. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,223 @@
    +    // The linked display could be removed.
    

    This is not helpful. "Check if the linked display hasn't been removed." or something?

  2. +++ b/core/modules/views/src/Plugin/views/area/DisplayLink.php
    @@ -0,0 +1,223 @@
    +      $errors[] = $this->t('%current_display: The link in the %area area points to the %linked_display display that no longer exists.', [
    

    that no longer exists => 'which no longer exists.'
    To keep this inline with the other error

  3. +++ b/core/modules/views/tests/src/Kernel/Handler/AreaDisplayLinkTest.php
    @@ -0,0 +1,384 @@
    +    parent::setUp();
    

    This should pass $import_test_views along

Also, the question from @benjifisher in #51 still stands:

Do we still need a follow-up? See #6, #7, #12. If not, then remove the issue tag "Needs followup".

If we do still feel they are needed, please open the needed follow ups.

phenaproxima’s picture

Issue tags: -Needs followup

I think I've had a change of heart with regard to default icons. Although it might be nice in the future, it's more important to implement this feature generically, and icons are a bit more of a case-by-case thing. We'll implement icons in the media library first, since that's specified by #2981044: Unify the grid/table views of the media library, and then possibly make them generic afterwards if there is demand. But that's low-priority. So let's not bother with it for now.

dww’s picture

Yes, looking great! Thanks for all the work from everyone.

Re: #60 @gbirch:

and does not identify which settings are under the Advanced area (which I think we need to tell users, since those settings are hidden by default).

That makes sense to me, but it's no longer in the patch, and there's no comment from @seanB about it.

I still think #48.3 isn't really addressed, either. Yes, we intend for it to be "the same results displayed differently", but we already agreed we're not enforcing it's the same results. Most obvious use-case is different displays ignore or enforce the pager. Yes, we preserve the pager value in the links across displays, but some honor it and others don't. Can we say "similar" results, instead?

Thanks,
-Derek

seanB’s picture

#62:
1. Fixed.
2. Fixed.
3. Fixed.

As phenaproxima already mentioned we have a followup to add icons for the Media library in Seven. Since the icons are probably going to be unique per feature/theme, I'm not sure we need a generic solution at the moment.

#60 / #64:
Ah, forgot to add a comment about that. There are several area and display plugins with settings that refer to contextual filters. I haven't seen it called 'Advanced: Contextual filters' anywhere. The online documentation also refers to just 'contextual filters', so I think we should be consistent here.

#64:

I still think #48.3 isn't really addressed, either.

Removed 'to show the same results in a different way'.

Status: Needs review » Needs work

The last submitted patch, 65: 3025657-65.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
27.57 KB

Sorry about that, forgot to update the test.

Lendude’s picture

Assigned: seanB » Unassigned
Status: Needs review » Reviewed & tested by the community

Looks great, usability review done, product manager review done, all feedback addressed.

joachim’s picture

> It is a page display

Why is this one of the criteria?

It seems to be if you want to embed display A inside display B, in a lot of cases, display A is going to be used only for embedding and shouldn't be seen standalone.

seanB’s picture

It is a page display Why is this one of the criteria? It seems to be if you want to embed display A inside display B, in a lot of cases, display A is going to be used only for embedding and shouldn't be seen standalone.

We are showing a link which needs a URL. I don't think this is about embedding a display. We are simply allowing users to create links to displays that have a URL.

seanB’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.42 KB
29.88 KB

When using this patch to fix #2981044: Unify the grid/table views of the media library, I noticed an issue when the view is loaded via AJAX. The _wrapper_format URL parameter causes the links to return JSON instead of opening the links. This is caused by #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs.

Views already contains a workaround for this, see #2798947: Views pagers include ajax metadata. I think we need to use a similar approach to filter some parameters from the query string, at least until #2504709: Prevent _wrapper_format and ajax_form parameters from bleeding through to generated URLs is fixed.

Attached patch should fix this.

dww’s picture

Nice catch. interdiff looks great. Comes with the fix and a test.

Thanks for addressing all my prior concerns!

If the bot is happy, this is RTBC from me.

Cheers,
-Derek

dww’s picture

Status: Needs review » Reviewed & tested by the community

Bot is now happy. See previous reviews. RTBC.

Thanks,
-Derek

  • Gábor Hojtsy committed 8b4ced1 on 8.7.x
    Issue #3025657 by seanB, dww, gbirch, benjifisher, Lendude, phenaproxima...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Looks great to me and I already reviewed this from multiple angles in the UX meeting and in the cross-initative meeting. Thanks all!

Fix this on commit:

FILE: ...commits/core/modules/views/src/Plugin/views/area/DisplayLink.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
  59 | WARNING | [x] A comma should follow the last multiline array
     |         |     item. Found: ]
 137 | ERROR   | [x] Expected 1 space after FUNCTION keyword; 0 found

  • Gábor Hojtsy committed 20948cc on 8.7.x
    Revert "Issue #3025657 by seanB, dww, gbirch, benjifisher, Lendude,...
Gábor Hojtsy’s picture

Status: Fixed » Needs work

While https://www.drupal.org/docs/8/system-requirements/php-requirements states Drupal 8.7 will require PHP 7.0 and recommend PHP 7.2, @alexpott points out that we are supposed to still support PHP 5.5 on a best effort basis (ie. only add code that requires PHP 7 if absolutely necessary). Unfortunately the ARRAY_FILTER_USE_KEY constant used here is only available in PHP 5.6 onwards. So reverted on those grounds.

seanB’s picture

Removed the array filter with ARRAY_FILTER_USE_KEY and replaced it with a simple foreach loop. Also fixed the PHPCS errors mentioned in #75.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

OK, looks good. Back to RTBC once green.

  • Gábor Hojtsy committed ba95daf on 8.7.x
    Issue #3025657 by seanB, dww, gbirch, benjifisher, Lendude, phenaproxima...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Yay, thanks!

dww’s picture

Sweet! Thanks, everyone. :)

Cheers,
-Derek

Status: Fixed » Closed (fixed)

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