Description

Because of the multi line approach too few views are visible at any one time in the table forcing a lot of scrolling.

Currently looks like this:

Proposed solution

Make the displays and machine name ordinary table columns as they are elsewhere.
Removes tag and makes the name of the view link to the view (at the path)

UI changes

Before

After

CommentFileSizeAuthor
#136 new_views_listing_page-2574767-136.patch20.71 KBManjit.Singh
#136 interdiff.txt740 bytesManjit.Singh
#128 new_views_listing_page-2574767-128.patch20.55 KBManjit.Singh
#126 interdiff.txt1.64 KBManjit.Singh
#126 new_views_listing_page-2574767-126.patch20.5 KBManjit.Singh
#114 after.png111.24 KBxjm
#114 before.png98.73 KBxjm
#113 IMG_3424.jpg50.87 KBxjm
#113 IMG_3423.jpg58.44 KBxjm
#108 interdiff-2574767-101-108.txt1.92 KBdipakmdhrm
#108 new_views_listing_page-2574767-108.patch20.4 KBdipakmdhrm
#101 new_views_listing_page-2574767-101.patch22.31 KBdipakmdhrm
#101 interdiff-2574767-88-101.txt3.1 KBdipakmdhrm
#92 interdiff.txt860 bytesDom.
#92 new_views_listing_page-2574767-92.patch22.19 KBDom.
#88 new_views_listing_page-2574767-88.patch22.2 KBdipakmdhrm
#88 interdiff-2574767-76-88.txt13.9 KBdipakmdhrm
#82 interdiff-77-82.txt1.95 KBimalabya
#82 new_views_listing_page-2574767-83.patch23.16 KBimalabya
#77 interdiff-2574767-63-76.txt14.22 KBdipakmdhrm
#77 new_views_listing_page-2574767-76.patch21.31 KBdipakmdhrm
#74 new_views_listing_page-2574767-74.patch21.26 KBdipakmdhrm
#63 new_views_listing_page-2574767-63.patch21.41 KBdipakmdhrm
#61 Views | drupal 8.2.x 2016-05-24 21-27-51.png107.68 KBGábor Hojtsy
#56 new_views_listing_page-2574767-56.patch21.28 KBdipakmdhrm
#53 new_views_listing_page-2574767-53.patch20.77 KBdipakmdhrm
#51 new_views_listing_page-2574767-51.patch19.62 KBdipakmdhrm
#49 views-listing-comparison.jpeg202.89 KByoroy
#49 views-listing-2.png177.44 KByoroy
#49 views-listing-1.png287.66 KByoroy
#48 new_views_listing_page-2574767-48.patch19.46 KBdipakmdhrm
#43 views-listing2.png151.8 KByoroy
#43 views-listing.png161.3 KByoroy
#41 new_views_listing_page-2574767-41.patch18.67 KBdipakmdhrm
#36 new_views_listing_page-2574767-36.patch18.1 KBdipakmdhrm
#36 new-views-ui.png159.23 KBdipakmdhrm
#34 screenshot-localhost 2016-03-30 18-01-12.png45.13 KBdipakmdhrm
Screen Shot 2015-09-25 at 11.57.20 AM.png228.39 KBtkoleary
views_list_compact.jpg74.63 KBtkoleary
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tkoleary created an issue. See original summary.

tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
tkoleary’s picture

Issue summary: View changes
Issue tags: +Usability
saschaeggi’s picture

I would love to see that +1

yoroy’s picture

Version: 8.0.x-dev » 8.2.x-dev
dawehner’s picture

I would love to see something like this!

rszrama’s picture

It's definitely a hard page to scan, but I do often find myself coming to the Views listing and searching by a known path to find the particular View I want to edit. I'm sure I can get over it if that changes, just wanted to share my use case. : )

dawehner’s picture

@rszrama
Fair, for simplicity we could move the machine name to its own column and keep the path and maybe drop tag, given how rarely its used.

alphex’s picture

for #10

if you do the CSS right, you can just hide columns based on display width... (but I use browser 100% width admin themes, so maybe that doesn't fly).

Agreed, "/path/to/view/" is important for helping you realize which view you're wanting to deal with.

rachel_norfolk’s picture

I like the concept but, looking back at a couple of built sites, a fair few views have multiple paths. If scanning for the paths to know which view to edit is a thing, how would we facilitate that?

Oh - how about a search filter at the top that we could enter a path into that then showed all views that we active on that path or lower?

delta’s picture

I use often a ctrl +f search for views path on this listing.. lots of site have poorly named views... Please keep the path on the views listing. However I agreed the long scrolling can be an issue too, but I don't think it worth to remove the path...
#13 yeah to add a javascript search filter.

tkoleary’s picture

@dawehner

+1 to removing tag. Could we also remove displays? that seems to me not a top level item. Remember, the more items there are on the page the more cognitive load on the user. If we could have only 5 columns that would be great.

dawehner’s picture

+1 to removing tag. Could we also remove displays? that seems to me not a top level item. Remember, the more items there are on the page the more cognitive load on the user. If we could have only 5 columns that would be great.

Agreed totally, so let's replace displays with path basically.

Oh - how about a search filter at the top that we could enter a path into that then showed all views that we active on that path or lower?

Given how this instant filter works we could still allow to search by displays or tags for example, even they aren't visible on the page.

howdytom’s picture

Agreed totally, so let's replace displays with path basically.

Good idea.. Let’s also change the order e.g.

View Name
Path
Description
Machine Name
Tag

yoroy’s picture

A list like in #17 is a good idea but the last couple of comments don't add up :-)

Gábor Hojtsy’s picture

I agree the list on #17 looks good :)

dawehner’s picture

View Name
Path
Description
Machine Name
Display

I would propose this list to be honest.

tkoleary’s picture

Why do we need machine name? This is a human-readable UI after all. :)

dawehner’s picture

Why do we need machine name? This is a human-readable UI after all. :)

For examples themers uses the machine name to determine the template they want to use, but sure, if people think its not useful for many people ...

DuaelFr’s picture

As a developer I also often use the machine name for views alterations. I know I can find it in the URL but it's easier to have it on the list.

Just a thought: why isn't this list a View so anyone could alter it to see what matters for him/her?

dawehner’s picture

Just a thought: why isn't this list a View so anyone could alter it to see what matters for him/her?

No reason beside a year of development in front of us

yoroy’s picture

Fair point :)

What about the machine name in a title attribute on the View title?

View name (+ machine name in title attribute)
Path
Description
Displays

I think we usually put a description first thing after the title. Interesting to see that path is considered more helpful so far. I'm not against it, just noticing.

dawehner’s picture

+1 to that idea. Let's just ensure we don't forget the operations, but I doubt anyone disagrees with that.

Bojhan’s picture

@yoroy We should do the machine name typography then though.

eelkeblok’s picture

Would it make sense to bundle the path and display columns? Paths are an artefact of page displays, after all. E.g. one View could have as its display column "Block, Feed", another could have "Block, Page (my/fancy/url)" (Edit: I disagree to remove Display, btw. I think it's a very important high level attribute of a View. Also, I've encountered a few sites where View naming is ambiguous and the available Displays are en important clue which View you are actually dealing with.)

yoroy’s picture

Not sure what that means @Bojhan. I don't think you can style title attributes, do you mean specific spelling? Hover this link for an example:

View name

Not ideal for mobile I guess.

dipakmdhrm’s picture

I agree with #28 and would recommend:

Name (machine_name in tittle attribute)
Description
Displays (with path in paranthesis after page display)
Operation
eelkeblok’s picture

If we are putting the machine name in a title attribute for the sake of #22/#23, I doubt its usefulness. I'd say that if it's for the purpose of code, you'd want to be able to copy-paste the machine name easily, which I don't think is possible (or at least straightforward/easier than copying the link URL and trimming it) from a title attribute. I don't think it's a UX pattern anywhere else in Drupal, but maybe a little "copy to clipboard" icon would make sense. However, if Display and path are consolidated into one, maybe having a separate machine name column doesn't hurt so much.

Name
Description
Machine name
Displays (with path in parentheses after page display)
Operations
DuaelFr’s picture

We could also keep the machine name on a second line below the human readable name.
As it's likely that Displays (merged with path) go multilines, it wouldn't hurt to make the name cell on two lines.

Name Description Displays Operations
My view name
[my_view_machine_name]
My View description that can be long Block, Block, Page (path/to/my/page), RSS, Other display [Edit|▼]
My other view
[this_has_no_relation]
Page (path/to/my/other/page), Page (path/to/my/other/page/{node}), Other Display with path (path/to/the/other/display) [Edit|▼]
yoroy’s picture

Nice mockup! :-) I think I'd prefer that the first column has only 1 line with just the title. Double lines will make this harder to scan/parse for people.

For the listing of displays + path I imagine it will be easier to read if it were an actual list instead of the comma seperated items.

I do wonder wether we're gaining any space with this approach though :) Will we have a more compact listing this way?

dipakmdhrm’s picture

Well, many of these implementation are failing to address the original issue i.e. too few items...

How about something like the Extend page?

That way we can have just three columns: Name, description & operation with everything else crammed under description.

extend-page

eelkeblok’s picture

@yoroy The original is roughly 5-6 lines high. I rarely encounter views that have more than 2-3 Displays, often only one*. So I'd still say that's a net gain. One other possible optimisation is grouping non-path displays together. E.g.:

  • Block (2)
  • Page (path/to/my/page)
  • RSS (path/to/my/page/rss.xml)
  • Other display

@dipakmdhrm Too few is a relative term. I think most of the options will result in a gain. If its enough, that's up for discussion, of course. The downside of a setup like the extend page is that it initially hides certain information. I like it, but I think the filtering would need careful consideration as I think that stuff that's hidden will not show in in a "cmd-f" search.

*: That does bring to memory a printout on a wall of shame which had a screenshot for a View with several dozens of displays, though; somebody didn't know about contextual arguments.

dipakmdhrm’s picture

How about something like this?

Tag column is now removed.
Displays and path are clubbed together in a single 'displays' column.
Search field can be used to search by name, description, machine name & path while all these also being searchable by "cmd-f" search

dawehner’s picture

I really really like it!

dipakmdhrm’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: new_views_listing_page-2574767-36.patch, failed testing.

eelkeblok’s picture

Awesome. Thank you for this.

dipakmdhrm’s picture

Status: Needs work » Needs review
FileSize
18.67 KB

Fingers crossed for this one!!

Status: Needs review » Needs work

The last submitted patch, 41: new_views_listing_page-2574767-41.patch, failed testing.

yoroy’s picture

FileSize
161.3 KB
151.8 KB

Starting to look much better!

The machine name is shown in a title attribute of the view name *and* in its own column, that seems a bit too much :)

This is what it looks like without that column. I also added a list-style: none to the list of displays:

What do we think?

Gábor Hojtsy’s picture

Issue summary: View changes

Looks good to me personally. OTOH issue summary needed screenshot updating, so did that.

eelkeblok’s picture

I like the list style none, I'm not a fan of the machine name in the title attribute. From my POV, listing the machine name serves two purposes:

  • Finding a view when you've come across it in code somehow (largely covered by the filter, but people will have old habits)
  • Copying it over for use in new code.

Neither of these are served very well with the machine name hidden in a title attribute.

yoroy’s picture

If as a title attribute is not helpful, do you think we should add it as its own column or leave it out entirely?

eelkeblok’s picture

My vote would go to a separate column.

dipakmdhrm’s picture

Status: Needs work » Needs review
FileSize
19.46 KB

If this patch fails, I'm gonna cuss the nearest person...

FYI... This patch also has the fix for list-styling...

yoroy’s picture

Looks great!

Smaller screen:

Making this table responsive is something to look at in a followup.

I'm fine with keeping the machine name around, we're already winning a lot of space with this approach:

Status: Needs review » Needs work

The last submitted patch, 48: new_views_listing_page-2574767-48.patch, failed testing.

dipakmdhrm’s picture

Status: Needs work » Needs review
FileSize
19.62 KB

Status: Needs review » Needs work

The last submitted patch, 51: new_views_listing_page-2574767-51.patch, failed testing.

dipakmdhrm’s picture

Status: Needs work » Needs review
FileSize
20.77 KB

This additionally contains the modified tests to check for views split listing

dipakmdhrm’s picture

Ok, so there was a test for verifying the presence of escaped strings in the tag information of view. Since we are no longer displaying tags, it has been removed.

In another test, XPath query was updated to accommodate the new HTML structure.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Just applied this patch on my local system and its looking really great!

+++ b/core/modules/views_ui/views_ui.theme.inc
@@ -42,6 +43,14 @@ function template_preprocess_views_ui_display_tab_setting(&$variables) {
 
+function template_preprocess_views_ui_view_info(&$variables) {
+  foreach ($variables['header'] as $key => $value) {

Needs a bit of documentation.

dipakmdhrm’s picture

Added the required documentation.

yoroy’s picture

Lovely. Thank you @dipakmdhrm. Looks ready to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 56: new_views_listing_page-2574767-56.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +sprint, +VDC

Was random fail with migrate. Added a retest. Moving back to RTBC.

yoroy’s picture

Issue summary: View changes
Gábor Hojtsy’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
107.68 KB

Reviewed this in the UX meeting today. We agreed the title as a header is fine, although it is not like that elsewhere, it gives visual hierarchy. People found two things odd/unusual:

- The label was not called a label (like elsewhere). We already know its views.
- The machine name would look better as not monospaced, would be more consistent (like in field management).

yoroy’s picture

We could argue that "View name" was already there and so not in scope, but hopefully an easy additional fix.
We also specifically requested the monospaced styling earlier but while reviewing it we concluded it does indeed attract too much attention.

Most importantly: we're all really happy with this design and are eager to get it in :-)

dipakmdhrm’s picture

Status: Needs work » Needs review
FileSize
21.41 KB

'View name' -> 'Label'
machine_name -> machine_name

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Using "Label" for the column also makes sense since the filter says "Filter by label, ..." so they match up :) Tested visually and it looks good. Back to RTBC then.

dipakmdhrm’s picture

@Gabor: The filter originally said "Filter by view name...". It was changed to match the label thing.. :)

tkoleary’s picture

Looks much better to me.

alexpott’s picture

Assigned: Unassigned » star-szr

Assigning to Cottser as the frontend specialist on the committer team.

dawehner’s picture

I'm not entirely convinced of the label change:

```/admin/structure/views/add``` still calls it view name, the views edit overview calls it view name, and the editing modal calls it "administrative name". At least none of those examples call it label.

Bojhan’s picture

@dawhner Is it possible to pick one for all places?

dawehner’s picture

@Bojhan
Well for now I would just go with 'view name' and think about using "label" in the follow up?

Bojhan’s picture

Status: Reviewed & tested by the community » Needs review

Ok lets do it.

yoroy’s picture

Assigned: star-szr » Unassigned
Status: Needs review » Needs work
star-szr’s picture

I am unassigned but here's a first pass at a review. Overall this seems low risk to change markup and such since it's an admin UI but we might break a contrib admin theme or something.

  1. +++ b/core/modules/views_ui/src/ViewListBuilder.php
    @@ -242,46 +247,34 @@ public function render() {
    +          // Cast the admin label to a string since it is an object.
    +          // @see \Drupal\Core\StringTranslation\TranslatableMarkup
    +          'display' => (string) $definition['admin'],
    

    If this object has a toString (which I think it must if we're casting to a string) Twig will just print it so this seems unnecessary but I might be missing something.

  2. +++ b/core/modules/views_ui/templates/views-ui-view-info-display.html.twig
    @@ -0,0 +1,24 @@
    + *   - display : Display name.
    + *   - path : Path to display, if any.
    
    +++ b/core/themes/stable/templates/admin/views-ui-view-info-display.html.twig
    @@ -0,0 +1,24 @@
    + *   - display : Display name.
    + *   - path : Path to display, if any.
    

    There's an extra space before the : on both of these lines.

  3. +++ b/core/modules/views_ui/templates/views-ui-view-info-display.html.twig
    @@ -0,0 +1,24 @@
    +        {{ display.display }} <span data-drupal-selector="views-table-filter-text-source">({{ display.path }}</a>)</span>
    

    It looks like we're linking the opening paren but not the closing one (one's inside the a tag and one is outside), is there any reason for this? Seems inconsistent/wrong.

  4. +++ b/core/modules/views_ui/templates/views-ui-view-info.html.twig
    @@ -4,25 +4,44 @@
    +<table {{ attributes.addClass('responsive-enabled') }}>
    ...
    +        <th {{ header.attributes }}>{{ header.data }}</th>
    ...
    +      <tr {{row.attributes}}>
    
    +++ b/core/themes/stable/templates/admin/views-ui-view-info.html.twig
    @@ -1,26 +1,47 @@
    +<table {{ attributes.addClass('responsive-enabled') }}>
    ...
    +        <th {{ header.attributes }}>{{ header.data }}</th>
    ...
    +      <tr {{row.attributes}}>
    

    There's no space needed before printing attributes. Attributes print their own space, that way we don't have <table > (for example) in our markup if there are no attributes to print.

  5. +++ b/core/modules/views_ui/templates/views-ui-view-info.html.twig
    @@ -4,25 +4,44 @@
    +      {% for key,header in header %}
    ...
    +    {% for key,row in rows %}
    
    +++ b/core/themes/stable/templates/admin/views-ui-view-info.html.twig
    @@ -1,26 +1,47 @@
    +      {% for key,header in header %}
    ...
    +    {% for key,row in rows %}
    

    This is not actually in the Twig coding standards but I'm fairly sure we should have a space after the comma here. http://twig.sensiolabs.org/doc/tags/for.html#iterating-over-keys-and-values

  6. +++ b/core/modules/views_ui/views_ui.theme.inc
    @@ -43,6 +44,27 @@ function template_preprocess_views_ui_display_tab_setting(&$variables) {
    +  $variables['attributes'] = new Attribute($variables['attributes']);
    

    This line shouldn't be needed, we auto-convert attributes, title_attributes and content_attributes to Attribute objects. #2108771: Remove special cased title_attributes and content_attributes for Attribute creation.

  7. +++ b/core/themes/stable/templates/admin/views-ui-view-info.html.twig
    @@ -1,26 +1,47 @@
    - * Theme override for basic administrative info about a View.
    + * Default theme implementation for basic administrative info about a View.
    

    This should stay the same in Stable.

  8. +++ b/core/themes/stable/templates/admin/views-ui-view-info.html.twig
    @@ -1,26 +1,47 @@
    + *
    + * @ingroup themeable
    

    Stable templates shouldn't have @ingroup themeable.

dipakmdhrm’s picture

Status: Needs work » Needs review
FileSize
21.26 KB

Status: Needs review » Needs work

The last submitted patch, 74: new_views_listing_page-2574767-74.patch, failed testing.

star-szr’s picture

@dipakmdhrm an interdiff and some words, for example: "Addresses #70 and #73" would be great :) thanks.

dipakmdhrm’s picture

Status: Needs work » Needs review
FileSize
21.31 KB
14.22 KB

Addresses #70, #73 & failure in #74.
Also contains additional documentation, changes to comply with coding standard and some common-sense changes that should've been there in earlier patches.

@Cottser:
For additional reference, patch has one of the test removed from views_ui/src/Tests/XssTest.php.
This test was to check for presence of Tags on the view list page, which we're not displaying anymore.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the quick update @dipakmdhrm

yoroy’s picture

Assigned: Unassigned » star-szr
star-szr’s picture

Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Needs work

Sorry for the delayed review. Overall looks very good, I have one concern and a couple nitpicks.

  1. +++ b/core/modules/views_ui/views_ui.module
    @@ -82,9 +82,12 @@ function views_ui_theme() {
    -      'variables' => array('view' => NULL, 'displays' => NULL),
    +      'variables' => array('headers' => NULL, 'rows' => NULL, 'attributes' => array()),
    

    This seems problematic, it's an API change. Anyone overriding this template (even just copying into their admin theme for example) will end up with a very broken Views UI following this change because their views-ui-view-info.html.twig will still be expecting the old variables. Can we keep views_ui_view_info as is and make a new theme hook to suit this purpose instead? I also think we can come up with a name better than views-ui-view-info considering what it's doing now :)

  2. +++ b/core/modules/views_ui/templates/views-ui-view-info.html.twig
    @@ -1,28 +1,49 @@
    +      <tr{{row.attributes}}>
    
    +++ b/core/themes/stable/templates/admin/views-ui-view-info.html.twig
    @@ -1,26 +1,47 @@
    +      <tr{{row.attributes}}>
    

    Minor: Needs spaces inside the curly braces per http://twig.sensiolabs.org/doc/coding_standards.html - in both templates.

  3. +++ b/core/themes/stable/templates/admin/views-ui-view-info.html.twig
    @@ -1,26 +1,47 @@
    + * Theme override for views listing table..
    

    Minor: double period.

star-szr’s picture

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -89,34 +89,30 @@ public function buildRow(EntityInterface $view) {
+            '#theme' => 'views_ui_view_info_display',
+            '#displays' => $this->getDisplaysList($view)
           ),

Oh also this is missing a trailing comma now per https://www.drupal.org/coding-standards#array.

imalabya’s picture

Status: Needs work » Needs review
FileSize
23.16 KB
1.95 KB

@cottser: for point 1 in #80 the twig file (`views-ui-view-info.html.twig`) has all the new variables declared in the new theme_hook. I doubt that the views ui will break if the template is overridden.

I even tested that out to check if that breaks, but things are pretty much in place.

Am I missing something in understanding the concern?

Also, fixed all the point asked in #80 as well as #81

imalabya’s picture

Ok, so the concern was the view UI might break if a contrib admin theme already has the views-ui-view-info.html.twig with old variables. My bad. Sorry for the confusion.

imalabya’s picture

Status: Needs review » Needs work
dipakmdhrm’s picture

Assigned: Unassigned » dipakmdhrm

@Cottser:

You made a very valid point of changing the theme file name from views_ui_view_info to something new (I am going with views_ui_view_listing_table ) so that their existing copy of views_ui_view_info.html.twig ( from their admin theme and expecting older variables ) doesn't try to override our new implementation and thus doesn't brake anything.

However, I don't get why should we need to "keep views_ui_view_info as is".
In my next patch I'll be changing the theme file name & removing the views_ui_view_info unless there is a reason we should keep it.??

imalabya’s picture

@dipakmdhrm:

IMO keeping the "views_ui_view_info as is" because if any contributed admin theme is currently using the views_ui_view_info twig file then the Views listing page will be broken because it will be looking for the previous Variables. So we should keep it there as in. So, in this case the views listing will look like the changes made in this issue but the contributed theme will have the older listing view, which is kind of an issue by itself.

But again, if we have to have a new theme file for the listing views_ui_view_listing_table then the existing views_ui_view_info will become obsolete. If we remove views_ui_view_info then also it makes the same scenario.

dipakmdhrm’s picture

Assigned: dipakmdhrm » Unassigned
Status: Needs work » Needs review

@malavya:

Had we made the UI changes that are needed here in the same template (views_ui_view_info), views listing page would break as the overriden template file in someone's admin theme, or the template_preprocess_views_ui_view_info would be expecting older variables.

Now that we are changing the template from views_ui_view_info to views_ui_views_listing_table, broken view listing page shouldn't be a problem as the template_preprocess_views_ui_view_info wouldn't be invoked and the overridingviews-ui-view-info.html.twig wouldn't be used.

And thus I believe we don't need to keep views_ui_view_info as is.

dipakmdhrm’s picture

Addresses #80 & #81

Additionally, views_ui_view_info_display template is now renamed to views_ui_view_displays_list.

star-szr’s picture

Status: Needs review » Needs work

The reason to keep the 'views_ui_view_info' theme hook is that any module can currently use that to output some data so it's API at this point and we can't just blindly remove it. All that's needed to exercise that functionality is a render array like this:

[
  '#theme' => 'views_ui_view_info',
  'view' => $view,
  'displays' => $displays,
]

It's unlikely to happen but I don't want to make assumptions like that, it's not safe.

A couple nitpicks:

  1. +++ b/core/modules/views_ui/views_ui.module
    @@ -80,11 +80,18 @@ function views_ui_theme() {
    +        'rows' => NULL,
    +        'attributes' => array()
    +      ),
    

    Minor: Missing trailing comma on the attributes line.

  2. +++ b/core/modules/views_ui/views_ui.theme.inc
    @@ -43,6 +44,32 @@ function template_preprocess_views_ui_display_tab_setting(&$variables) {
    +}
    +
    +
    +/**
    

    Minor: Extra blank line here below template_preprocess_views_ui_views_listing_table().

yoroy’s picture

Who can do these final changes please? It's such a nice design update, I'd love to see this committed :-)

dipakmdhrm’s picture

Assigned: Unassigned » dipakmdhrm
Dom.’s picture

Status: Needs work » Needs review
FileSize
22.19 KB
860 bytes

Just simple patch: fixed the two nitpicks from @cottser.

dipakmdhrm’s picture

Status: Needs review » Needs work

@Dom.'s fix doesn't bring back the views_ui_view_info theme hook

dipakmdhrm’s picture

@Cottser:

Should I just bring back the views_ui_view_info theme hook or do we need the views-ui-view-info.html.twig files as well (views_ui module & stable theme).??

fgm’s picture

I might be atypical in this, but I'll miss the tag field, which I've long been using to group views by site concern.

dawehner’s picture

@fgm
Would it be enough for you to have this just searchable but not being exposed in the UI?

tkoleary’s picture

@fgm

I might be atypical in this, but I'll miss the tag field, which I've long been using to group views by site concern.

Logic clearly dictates that the needs of the many outweigh the needs of the few.
-Spock

tkoleary’s picture

@fgm

You could always write a module that makes a view of all views and configure it how you like. :)

star-szr’s picture

@dipakmdhrm you need to keep the template files as well otherwise it's still a BC break at least in my opinion.

fgm’s picture

As @tkoleary mentions, my personal wishes don't have any reason to weigh much, but there's a more general perspective to this : filtering by tags provides a navigational UI, allowing point and click selection, whereas search requires actual data input. UX-wise, requiring more typing is probably a net loss.

dipakmdhrm’s picture

Addresses #89.

dawehner’s picture

So we keep the templates files etc. around while not using them anymore? This still feels a bit like a BC break, but if @Cottser is happy with that, I'm happy :)

Status: Needs review » Needs work

The last submitted patch, 101: new_views_listing_page-2574767-101.patch, failed testing.

yoroy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 101: new_views_listing_page-2574767-101.patch, failed testing.

yoroy’s picture

Only 1 fail left to fix! Lets get this in for the 8.2 release :-)

Gábor Hojtsy’s picture

Looks like the problem is Stable has a views-ui-view-info.html-stable.twig which should be views-ui-view-info.html.twig, the test fail asserts that Stable overrides all the templates, and this way with views-ui-view-info.html-stable.twig it does not :) Easy fix? :) @dipakmdhrm can you take a look?

dipakmdhrm’s picture

Addresses #107.

And the test failure above is probably what @Cottser meant when he said that twig files should also be kept if we are keeping the theme hook. Test would have failed if we had removed the twig file.

dipakmdhrm’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Assigned: dipakmdhrm » star-szr
Status: Needs review » Reviewed & tested by the community

Yay, thanks! That indeed fixes the fail, and I think all concerns so far have been answered. Assigning to Cottser for committer review, hopefully commit :)

star-szr’s picture

Assigned: star-szr » Unassigned

I very likely won't have time to give this another thorough review in the near future but my big concerns were addressed so unassigning, this could be looked at by another committer as well. Thanks all :)

dawehner’s picture

I just want to remind people that this is not a user facing template change or anything which would break functionality for users. Its simply a change in the admin UI and help everyone to potentially feel less overwhelmed.

xjm’s picture

Issue summary: View changes
Issue tags: +Needs followup
FileSize
58.44 KB
50.87 KB

Every time we add a column to an admin listing, we should test mobile.

Here's this view on my iPhone, before and after the patch.

Before

After

So the patch actually improves that too. Nice.

It would be good to have a followup to make this table actually responsive (i.e. set some priorities on the columns). Not in scope here, though, because the problem also exists in HEAD.

I haven't reviewed the patch in depth yet; just wanted to document the mobile testing. :)

xjm’s picture

Issue summary: View changes
FileSize
98.73 KB
111.24 KB

And on not-mobile.

Before

After

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Some more feedback, some of which is actionable. Point 1 particularly.

  1. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -183,22 +183,27 @@ details.box-padding {
    +  margin-left: 0;
    

    What about RTL?

  2. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -183,22 +183,27 @@ details.box-padding {
    +  width: 20%;
    ...
    +  width: 30%;
    ...
    +  width: 15%;
    ...
    +  width: 25%;
    ...
    +  width: 10%;
    

    These numbers reassuringly add up to 100.

  3. +++ b/core/modules/views_ui/src/Tests/XssTest.php
    @@ -17,9 +17,6 @@ class XssTest extends UITestBase {
    -    $this->drupalGet('admin/structure/views');
    -    $this->assertEscaped('<script>alert("foo");</script>, <marquee>test</marquee>', 'The view tag is properly escaped.');
    

    At first I was like "Whoa wait why are we removing this coverage?" but this was coverage for the view tag, which we are no longer displaying at all. That's fair enough. Are view tags used anywhere at all in the core UI now?

  4. +++ b/core/modules/views_ui/src/ViewListBuilder.php
    @@ -196,13 +202,13 @@ public function render() {
    +      '#placeholder' => $this->t('Filter by view name, machine name, description or display path'),
    

    Minor: Missing serial comma.

  5. +++ b/core/modules/views_ui/src/Tests/DefaultViewsTest.php
    @@ -161,10 +161,11 @@ function testDefaultViews() {
    -      ':title' => t('Machine name: test_view_status'),
    +      ':title' => 'test_view_status',
    

    The "machine name" is also now its own column in the new UI, which is also more semantic, and that is why this is changed.

  6. +++ b/core/modules/views_ui/src/ViewListBuilder.php
    @@ -196,13 +202,13 @@ public function render() {
    -        'title' => $this->t('Enter a part of the view name or description to filter by.'),
    +        'title' => $this->t('Enter a part of the view name, machine name, description or display path to filter by.'),
    

    Ditto re: serial comma.

  7. +++ b/core/modules/views_ui/src/ViewListBuilder.php
    @@ -242,46 +245,33 @@ public function render() {
    -  /**
    -   * Gets a list of paths assigned to the view.
    -   *
    -   * @param \Drupal\Core\Entity\EntityInterface $view
    -   *   The view entity.
    -   *
    -   * @return array
    -   *   An array of paths for this view.
    -   */
    -  protected function getDisplayPaths(EntityInterface $view) {
    

    So we are removing this method entirely?

    It's protected, so that should be okay. CR? (Edit: for the whole change, not just this, but it could mention this.)

Thanks everyone!

xjm’s picture

BTW I also tested the new filtering on four different fields, and it works really nicely.

dipakmdhrm’s picture

@xjm: About #117:
Is #2 a suggestion for change or just a comment?
And, Did not get #7. What's a CR?

fgm’s picture

@dipakmdhrm: a Change Record. This is how we document changes to the API. They are listed at https://www.drupal.org/list-changes/drupal for core.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

Added a site builder change notice draft at https://www.drupal.org/node/2781591. Not sure what to do about a coder change notice. Should it list the new templates, or?

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh
Gábor Hojtsy’s picture

Issue tags: -Needs change record

Added a themer/developer change record too at https://www.drupal.org/node/2782031. Still needs work for the rest of @xjm's feedback.

Gábor Hojtsy’s picture

Issue tags: -Needs followup

Followup submitted at #2782047: Listing of views is not a responsive table marked as related to this one. I think all is left are issues with the patch and @Manjit is working on that I believe.

Manjit.Singh’s picture

Assigned: Manjit.Singh » Unassigned
Status: Needs work » Needs review
FileSize
20.5 KB
1.64 KB

Here are the changes as per #117.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Thanks, only found this:

+++ b/core/modules/views_ui/css/views_ui.admin.theme.css
@@ -184,10 +184,14 @@ details.box-padding {
+  margin-left: 0; /* LTR */
+  padding-left: 0; /* LTR */
   list-style: none;
 }
+[dir="rtl"] .views-ui-view-displays ul {
+  margin-right: 0;
+  padding-right: 0;

You also need to undo the left margin and padding from RTL making it auto or the value it overrode.

Manjit.Singh’s picture

yoroy’s picture

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

@Manjit.Singh
Ensure to include interdiffs in the future.

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -89,34 +89,30 @@ public function buildRow(EntityInterface $view) {
         'view_name' => array(
           'data' => array(
-            '#theme' => 'views_ui_view_info',
-            '#view' => $view,
-            '#displays' => $this->getDisplaysList($view)
+            '#plain_text' => $view->label(),

So we basically skip the existing template. I think its not a big problem, but still from a pure BC policy one could argue that its a problem,
but cotter in #2574767-111: Views listing page displays too few items on a page agreed with the proposal.

Gábor Hojtsy’s picture

Yeah we need to keep the template for backwards compatibility but we don't need to use it anymore.

Let's get this in then! :)

Gábor Hojtsy’s picture

Assigned: Unassigned » star-szr

Attempting to assign to Cottser again.

star-szr’s picture

@dawehner regarding #130:

We can change render arrays per https://www.drupal.org/core/d8-allowed-changes#minor.

This is on my list to review. Thanks all.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Reviewed & tested by the community » Needs work

This is looking very good, there's only one problem I can see: The Stable version of views_ui.admin.theme.css is missing the most recent RTL CSS changes from #128. We could have seen that easily with an interdiff, so they really are helpful!

Back to needs work for that touch-up.

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh
Manjit.Singh’s picture

Assigned: Manjit.Singh » Unassigned
Status: Needs work » Needs review
FileSize
740 bytes
20.71 KB

here we go. I have made the suggested changes, Please review.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Yay thanks, looks like it is just the same change applied to stable as requested by @Cottser, so should be back to RTBC.

  • Cottser committed 3f8aa20 on 8.3.x
    Issue #2574767 by dipakmdhrm, Manjit.Singh, malavya, Dom., yoroy, xjm,...
star-szr’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +8.3.0 release notes

Excellent, thanks @Manjit.Singh and @Gábor Hojtsy.

Committed 3f8aa20 and pushed to 8.3.x. Thanks!

dawehner’s picture

Nice, great work everyone!

yoroy’s picture

Ah great, thanks everyone for seeing this through!

tkoleary’s picture

+1 great work

Manjit.Singh’s picture

woot :) Chak de

dipakmdhrm’s picture

Glad to see this committed to core!! Yay!
Great work everyone!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: -sprint
quietone’s picture

publish change records

fgm’s picture

Issue summary: View changes