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 patchCode review- Usability review
- Commit
User interface changes
New area plugin for display links
Display link form when there are no allowed displays in the view
Display link form
Grid view
Table view
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.
Comment | File | Size | Author |
---|---|---|---|
#78 | 3025657-78.patch | 29.85 KB | seanB |
#78 | interdiff-71-78.txt | 2.37 KB | seanB |
#71 | 3025657-71.patch | 29.88 KB | seanB |
#71 | interdiff-67-71.txt | 4.42 KB | seanB |
#67 | 3025657-67.patch | 27.57 KB | seanB |
Comments
Comment #2
seanBPatch is attached. Please review!
Comment #3
LendudeLooks really nice already! Love the test coverage. Quick first review.
should be type label to make this translatable in config.
Why continue? Can there only be one display that is not allowed?
make this an early return instead of the method one giant nested if?
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'.
missing docblock and should be protected
missing docblock
white space
filters => 'pager is the same'
filters => arguments.
white space
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?
Comment #4
LendudeOne more
I don't see test coverage for this if, am I overlooking it, or is it missing?
Comment #5
seanBThanks @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.
Comment #6
starshapedThe 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.
Comment #7
phenaproxima+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!
Comment #8
LendudeJust mostly nitpicks left from me.
Ah now I get it, it's just an if-else where the else is a continue :)
2 => two
is this right? If I manage to link it to a block display, this
if
will not trigger and it will pass validation, right?Check is => Check if
2 => two
Do we really need to rebuild the router every time in the test?
1 => one
Comment #9
MrMason CreditAttribution: MrMason at Sealed Air commentedI 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.
Comment #10
phenaproximaSounds like a thing we need tests for. Thanks, MrMason!
Comment #11
xjmBumping to major feature given the impact on Media Library. Sounds like a nice feature!
Comment #12
seanBNew 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
Table view
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).Comment #14
seanBPatch seems to be green, not sure what happened. Please review.
Comment #15
phenaproximaI 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:
Advantages of doing it this way:
Comment #16
seanBImplemented the inline twig template field for the plugin (like already available in fields).
One thing though:
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:
Comment #18
seanBRandom fail was related to another issue (see #3003238-21: EntityStorageException: Default revision can not be deleted in content_moderation_entity_revision_delete()). Back to NR.
Comment #19
LendudeDiscussed with @seanB and I agree with his statement:
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...).
Comment #20
dww+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
Comment #21
seanBNew patch to implement a template file views-view-display-link.html.twig.
Comment #23
seanBAdded the template to stable as well.
Comment #24
LendudeLooking great, just some last nitpicks left for me:
page display => display with a path
do we want to do something if there are no allowed displays? Show a message (extend $message?) instead of the form options?
Comment #25
seanBFixed #24.
Comment #26
seanBD'oh!
Comment #29
seanBAdded newlines to twig files because of coding standards. Thanks Lendude for pointing this out. Adjusted the tests accordingly.
Comment #30
Lendudeforgot earlier but @phenaproxima++ for raising the issue of themability, spot on.
@seanB++ for the implementation
I got nothing to nitpick anymore, RTBC from me.
Comment #31
dwwYes, 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:(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
Comment #32
seanBNo problem, I think the nits make sense. Fixed #31.
Comment #33
seanBArghh, stupid typo. Sorry about that.
Comment #35
dww@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...
Comment #36
jibranThis 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?
Comment #37
dwwI 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
Comment #38
phenaproxima@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.
Comment #39
seanB#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
Comment #40
seanBComment #41
phenaproximaAdding release notes snippet to the IS.
Comment #42
dwwRe :#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?
I'd rather this:
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?
Comment #43
dwwMore 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!
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.
Comment #44
phenaproximaI'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.
Can we rephrase the ending here: "...you may only select a display which:"
And let's bring these in line with that change. "Has a path", "Has the same filter critiera", etc.
I don't think we need "currently".
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.)
Comment #45
webchick@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.
Comment #46
seanBThanks everyone. Will start on this tonight!
Comment #47
seanBOk 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?
Comment #48
dwwQuick skim: looks great! Thanks!!
A few initial nits:
1.
@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.
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.
No longer true. Sorry I can't wordsmith a better alternative right now (juggling too many things today!).
Comment #49
seanB#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.
Comment #50
dawehnerI 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.
❓I'm wondering whether "display_id" would be a bit more explicit of what is stored.
I'm wondering why #theme item_list isn't used here instead.
This could be improved by using
return $loaded_display instanceof PathPluginBase
which is way more readable, IMHO.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.Comment #51
benjifisherI 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".
This is the current interface text, and I have a few objections:
I am making a grammatical fix to the Release notes snippet, but it may need more substantive revision for the changes made in #47.
Comment #52
gbirch CreditAttribution: gbirch as a volunteer and at Tech-Tamer, LLC commentedDuplicate 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.
Comment #53
benjifisherAnswering 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
andfeed
, but notattachment
,block
,entity reference
, norembed
.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
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.
Comment #54
gbirch CreditAttribution: gbirch as a volunteer and at Tech-Tamer, LLC commentedPatch to improve the validation along the lines suggested in the last comment.
Comment #55
gbirch CreditAttribution: gbirch as a volunteer and at Tech-Tamer, LLC commentedAdded an interdiff.
Comment #57
benjifisherAt 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.
Comment #58
gbirch CreditAttribution: gbirch as a volunteer and at Tech-Tamer, LLC commentedUpdated the tests to reflect the improved warning messages. Includes interdiffs against #49 and my last patch.
Comment #59
seanBThis check makes sense. Nice! Supernit: Line is 81 characters.
I think these should be on the same line.
I don't see something similar for other views plugins, but still: should we not show add an actual validation error in this case?
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.
We could probably lose the 'to link to'.
Nit: We should use the 80 characters per line.
Maybe we should add a loop here, and call a
hasEqualOptions()
method that returns a bool instead of doing everything ingetDifferentOptions
. That might make this easier to extend/re-use.I think we can lose the "in each ares" in "please check that the settings in each area are the same".
Maybe "unequal_options" is a bit more descriptive then "differences"?
As mentioned above, can we change this method to return a bool and move the loop outside of the method?
We can get the titles from
Views::getHandlerTypes()
.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.
Let's get the titles from
Views::getHandlerTypes()
Should we use a comma instead of a semicolon?
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.Comment #60
gbirch CreditAttribution: gbirch as a volunteer and at Tech-Tamer, LLC commented@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?
Comment #61
seanB#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
Create a view with only block displays.
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.
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.
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.
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:#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.
Comment #62
LendudeSeems like the usability review has been done and feedback has been addressed, so removing the tag for that.
Some extremely nitty nitpicks left:
This is not helpful. "Check if the linked display hasn't been removed." or something?
that no longer exists => 'which no longer exists.'
To keep this inline with the other error
This should pass $import_test_views along
Also, the question from @benjifisher in #51 still stands:
If we do still feel they are needed, please open the needed follow ups.
Comment #63
phenaproximaI 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.
Comment #64
dwwYes, looking great! Thanks for all the work from everyone.
Re: #60 @gbirch:
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
Comment #65
seanB#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:
Removed 'to show the same results in a different way'.
Comment #67
seanBSorry about that, forgot to update the test.
Comment #68
LendudeLooks great, usability review done, product manager review done, all feedback addressed.
Comment #69
joachim CreditAttribution: joachim commented> 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.
Comment #70
seanBWe 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.
Comment #71
seanBWhen 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.
Comment #72
dwwNice 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
Comment #73
dwwBot is now happy. See previous reviews. RTBC.
Thanks,
-Derek
Comment #75
Gábor HojtsyLooks 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:
Comment #77
Gábor HojtsyWhile 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.
Comment #78
seanBRemoved the array filter with ARRAY_FILTER_USE_KEY and replaced it with a simple foreach loop. Also fixed the PHPCS errors mentioned in #75.
Comment #79
phenaproximaOK, looks good. Back to RTBC once green.
Comment #81
Gábor HojtsyYay, thanks!
Comment #82
dwwSweet! Thanks, everyone. :)
Cheers,
-Derek