Commit Message

Issue #1903746 by Mark Carver, mgifford, DaneMacaulay: Replace the views grid table template with one using divs

Problem/Motivation

Nowadays I think that providing a grid using tables doesn't seem to be really modern.
Tables are bad for accessibility.

There are probably thousands of techniques out there to provide a grid using css, let's discuss which one to use,
and then implement that. This sounds like a really good task to get some new people involved with a strong theming background.

Proposed resolution

Replace the grid style plugin with one that uses <div/> tags. Create tests to ensure they show up correctly.

Patch

Complete patch: #89
Test only patch: #73

Completed tasks

  • [done] Accessibility Review - (last done in #48 by @mgifford.
  • [done] Manual Testing - (last done in #74)
  • [done] Code Review (last done in #61 and #69)

Remaining tasks

None

User interface changes

Before

The views grid style used tables to create a grid of content.

After from #48 (screenshot shows special classes which was removed in #64, interdiff.txt is at #66

Screen Shot with div's and articles for grid rather than table.

API changes

No API changes. Worth mentioning that the grid style options have changed a bit though, sites and themes will have to update their overridden code accordingly.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

michaelfavia’s picture

Ive been looking to get involved on the theming side of Drupal 8 with twig, etc. If this is an opportunity to do so count me in. Any preferred method?

tim.plunkett’s picture

display: inline-block; seems like a good candidate.

damiankloip’s picture

Wouldn't/couldn't this be covered by the twig guys in their template conversion?

tim.plunkett’s picture

No, because they should be doing 1:1 conversions.
Also making the divs is the "easy" part compared to the CSS.

damiankloip’s picture

Meh, I know nothing about this stuff. I did mention in the twig sandbox queue that we have no visibility of the work they are doing. I think we should have an issue per template maybe?

michaelfavia’s picture

Spoke with timplunkett in #dc and here is our suggested solution:

Reimplement as divs (either float: left or display: block-inline) and use entity #attached to include some dynamically generated css that sets the width of the items as the correct percentage (100%/5columns = 20%) of 100%. Otherwise I'll also assume that we want to carry over the "first-row", "last-row" and similar css classes that were attached. Unless of course #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors lands. ;)

larowlan’s picture

Tables for layout is poor a11y.

mgifford’s picture

With proper use of caption & summary elements, tables can be fine for accessibility, but we still aren't there yet - #843708: Add option to set caption & remove summary in the html table (Accessibility) (help needed btw)

One advantage of moving to div's though for the grid is that it would be much easier to make it mobile friendly.

There's also the general rule to eliminate div's for formatting and that's essentially what we've got here.

DaneMacaulay’s picture

Heres something to start with.
I'm adding a variable called style which is the percentage width of the grid and outputting it as inline styling for the floated grid elements.
I think we could do this as defined css classes, but we would have to define each of those classes and limit column count to a fixed set of choices.

dawehner’s picture

Great work so far. I'm wondering whether we could somehow either generate the css file on the fly (ctools had some utilities for that) or just define a limited amount of grid-sizes as css, in order to avoid inline style.

DaneMacaulay’s picture

Good idea, this would do it:

<?php
  $width = (100/$columns);
  $css = ".views-grid-item{width:$width%;}";
  $css_file = ctools_css_cache($css, FALSE);
  drupal_add_css($css_file);

?>

is ctools_css_cache() available/refactored in 8.x ?

dasjo’s picture

hi,

i don't want to derail this issue, but i'm interested so here are some thoughts.

when using mobile-first, responsive design you start with just one column on mobile devices and add multiple columns for larger device widths.

this is easy to achieve using sass and for example the susy grid framework, here's an example where 2 & 3 block column layouts are established for narrow and wide device widths.

the previous comments suggest to generate css which sets the item widths. this sounds feasible, still if you want it in a responsive way you need some extra logic. i'm not aware of what road people want to go for in drupal core, but with breakpoint module in core, looking for integration options might be interesting. i guess the plan is to generate css based on breakpoint configurations.

also i'd like to reference this article
http://www.leveltendesign.com/blog/ian-whitcomb/using-views-responsive-g...

mgifford’s picture

I like the idea of modernizing the grid and pulling out ideas from this sandbox:
http://drupal.org/sandbox/iwhitcomb/1879012

I don't see that as derailing this issue, just making it kick ass.

That being said, there's a lot of work to make it happen in D8 by the feature freeze.

forestmars’s picture

That being said, there's a lot of work to make it happen in D8 by the feature freeze.

That's about 48 hours away.

dawehner’s picture

Are we sure this is a feature and not just a simple markup cleanup?
Sure you could aruge this as a feature, but from my perspective this should be still possible.

tim.plunkett’s picture

It's categorized as a task for a reason.

nikkubhai’s picture

One point which I would like to bring to notice is that grids in views are not responsive. So, even a defualt drupal 8 site displaying views grid will not be accessible on mobile.
Shall I add a task mentioning make views grid responsive?

dawehner’s picture

Well, wouldn't porting to divs sort of make it responsible automatically? Table views are responsible, because we ported them to be so.

nikkubhai’s picture

@dawehner: Sorry, I have no idea about that. :/

nikkubhai’s picture

We can get some help from this module http://drupal.org/project/views_responsive_grid . I will ask their maintainers.

iwhitcomb’s picture

So where do we begin getting views_responsive_grid into D8?

tim.plunkett’s picture

@iwhitcomb, first, port the module to work in D8. Then we can revamp it as part of views itself.

dawehner’s picture

Yeah we should really take a look at that module and find out what is the core of it. There seems to be quite a lot of settings,
so it feels like we don't want/need them all. If someone needs all these features, they can still use that module.

derhasi’s picture

I just posted a D8-port patch in the contrib module: #1932830: D8 Port.

views_responsive_grid itself does not provide any CSS to make the layout work. Im wondering if the core themes provide default styles for building grids. If so we could use views_responsive_grid as the base for a grid to repsonsiveGrid conversion.

derhasi’s picture

Issue tags: +SprintWeekend2013
markhalliwell’s picture

Just adding my $0.02 worth: #1931466-5: Converting views grid table to bootstrap div grid.

I think there will always be use-cases for tables. Granted, its the original intent of a table that should be considered (data, and lots of it). Despite the fact that anti-table layouts have been around for years, I think that the latest responsive craze tends to want to completely eliminate them. We really shouldn't. Just to re-iterate what I posted in the issue mentioned above:

I agree that using a table for a layout is bad, so don't. Consider though: You need to display large amounts of actual data (1000's of rows, 30+ columns), complete with headers (maybe even a column header)... tables just make sense, why re-invent the wheel? Also, I seriously doubt you'd willingly look at that much data on a mobile device (responsive or otherwise) and would probably head over to a desktop anyway. I am really against replacing the grid. Instead, there should just be another option for layouts so they can be responsive.

Also, most modern browsers handle copy & paste for tables.... not so much with divs.

markhalliwell’s picture

Afterthought: I'd much rather have just one "Grid" style that has the option inside the style settings to toggle between using a div or table. In the spirit of crushing tables though, we could easily it to default to a div layout. At least then we have an available option and not another blog post on how to hack views.

dawehner’s picture

@Mark Carver
Thanks for your comment. One major point we should consider is that we have the possibility to do it right now. If you want to display a table, and not show something in a "tabled"-way views will always ship with a table style, which will, not only for semantic reasons, output a table. For the grid, which looks totally different, we should do it in a proper way, which from my standpoint seems to be a responsive grid. If contrib really wants to be able to switch between tables and divs, go for it (there is no technical problem with that), though for core this seems to be a really bad pattern.

markhalliwell’s picture

@dawehner: I'm not entirely sure if I completely understand what you're trying to say. Are you saying that you believe the grid style should, by default, display divs and if you want tables you'll have to use a contrib module?

If so, I'm rather disappointed to hear this.

One major point we should consider is that we have the possibility to do it right now.

This is exactly the point I was trying to make above: "I think that the latest responsive craze tends to want to completely eliminate them [tables]". This is a very poor way to rationalize the removal of tables. Responsive is simply just the current solution for displaying content on a mobile device. It does not, nor should it, negate the need for using tables where the use case is appropriate.

If contrib really wants to be able to switch between tables and divs, go for it (there is no technical problem with that), though for core this seems to be a really bad pattern.

Am I missing something? How is it a bad pattern to provide the ability to toggle between two, actively, used style configurations? The grid style already has settings to toggle between horizontal and vertical data directions. By that statement, then those settings shouldn't be in core either... but they will be because it's already part of the grid plugin. So why can't we take a little time to make to make sure it's right and supply a setting to determine the type of grid: div or table (unless that's what you meant and if so, then I misunderstood... sorry lol).

Also, I agree with #23. The additional class settings from http://dgo.to/views_reponsive_grid should not be migrated. Those can be altered with preprocessing, no need to have them in core and make it even more difficult to configure.

dawehner’s picture

@mark Carver
If you want to display something as a grid, the markup should be divs. That's it.

, negate the need for using tables where the use case is appropriate.

Okay just to be clear. If the user want's to display a table, views still outputs a table. Views tables are responsible due to drupal core. We really just talk about the grid style here.

markhalliwell’s picture

I apologize. I honestly simply forgot there is an actual table style, separate from the grid. I guess I tend to mesh them both in my head because they both use tables for output. I had to go look at the provided styles again. Ok, don't mind me then lol I'm ok with the grid being converted. Just wanted to make sure that tables weren't completely coming out (mainly due to the issues I mentioned).

dawehner’s picture

No problem. Please try out the responsive table feature in core, it's really great!

mgifford’s picture

Is there a way to do this in a simpler way? I am just worried that there really hasn't been much progress on this issue since it was introduced earlier in the year.

tkoleary’s picture

See attached excerpt from designs in: #2022297: [META] Unified toolset for Views in core

The Pinterest style image grid in the grid view in files offers a great advantage to the user in that for images or videos with a variety of aspect ratios no part of the image is cropped, while a consistent look is also maintained.

This effect would require masonry.js. I am *not* suggesting that we add another library dependency to core. The reason I show it is that I think it's important to write the markup in such a way that plugging something like Masonry into the grid view is simple for someone creating an alternate admin theme or a module that leverages core admin views.

For core I *think* the proper solution is to use

image

tkoleary’s picture

dawehner’s picture

One really big advantage of using some kind of library is that we would have a responsible version out of the box,
but I guess we need at least some fallback based on css only? Sadly noone stood up yet and really get momentum behind this issue.

If I understand this library right, there is no real requirement for the html beside being based on divs.

tkoleary’s picture

@dawhener

If I understand this library right, there is no real requirement for the html beside being based on divs.

I *think* that's right.

mgifford’s picture

Status: Active » Needs review
FileSize
1.76 KB

Just to help move grids away from tables I'm updating the patch from #9 above. That's not what I've done here, but time is running out and Grids are still pretty ugly.

I do think it would be great if we could get something that looked as sharp as the masonry.js layout in D8. So much of the new interface comes from being able to scroll like this.

Plus, it looks like it's only 160 links of javascript under a MIT license:
https://github.com/desandro/masonry/blob/master/masonry.js

mgifford’s picture

This doesn't work yet, but, I'm having to head off.

It looks like it would simplify much of the PHP code if we went to mason.js too. So much that just isn't required to be defined.

markhalliwell’s picture

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

A masonry grid is definitely not in the scope of this issue. That's a separate feature all together. Had a talk with @tim.plunkett and @iwhitcomb in IRC just now. Between @iwhitcomb and I, we'll get an RTBC patch here before Monday's deadline.

mgifford’s picture

Brilliant. Thanks Mark!

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
16.09 KB

Alrighty, here's the patch. I'll commit the same code for the 8.x branch of the views_responsive_grid module, just in case.

markhalliwell’s picture

Fixed the views style schema. Changed the twig row and column ids to _key to make more sense.

markhalliwell’s picture

Fixed two mapped types in the views schema

Status: Needs review » Needs work
Issue tags: -Accessibility, -Needs accessibility review, -VDC, -SprintWeekend2013

The last submitted patch, drupal-replace-the-views-1903746-43.patch, failed testing.

markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: +Accessibility, +Needs accessibility review, +VDC, +SprintWeekend2013
R.Hendel’s picture

I did apply latest patch (drupal-replace-the-views-1903746-42.patch).
Before applying the patch on a naked current 8.x views returns me six items in my testing system.
After applying patch I get no results displayed in views and following warnings:

User warning: row_classes could not be found in _context in "core/themes/bartik/templates/page.html.twig" at line 170 in Drupal\Core\Template\TwigTemplate->getContextReference() (line 51 of core/lib/Drupal/Core/Template/TwigTemplate.php).
User warning: row_classes could not be found in _context in "core/themes/bartik/templates/page.html.twig" at line 170 in Drupal\Core\Template\TwigTemplate->getContextReference() (line 51 of core/lib/Drupal/Core/Template/TwigTemplate.php).

So - do I need any more patch files or is something wrong with it?

mgifford’s picture

Try #44.

No error messages there that I could see. Good step ahead for a grid layout.

Screen Shot with div's and articles for grid rather than table.

All via SimplyTest.me!

R.Hendel’s picture

I repeated following test scenario three times this morning:
- I removed complete installation of all files and database,
- cloned current version of drupal 8.x
- created content and view so that I have a grid-based view displaying six items on an unpatched drupal
- applied patch from #44
- cleared all cache and run cron
- reloaded views page

I always get user warnings and no items selected using grid.

    User warning: row_classes could not be found in _context in "core/themes/bartik/templates/page.html.twig" at line 170 in Drupal\Core\Template\TwigTemplate->getContextReference() (line 51 of core/lib/Drupal/Core/Template/TwigTemplate.php).
    User warning: row_classes could not be found in _context in "core/themes/bartik/templates/page.html.twig" at line 170 in Drupal\Core\Template\TwigTemplate->getContextReference() (line 51 of core/lib/Drupal/Core/Template/TwigTemplate.php).

When I change output format to e.g. "unformatted", I will get result set of six items.
After changing output format back to "grid", I'll get no results displayed.

So I think there could be a problem with that patch.

My php version is 5.4.10.
Maybe another user could try this to be sure?

markhalliwell’s picture

@R.Hendel, can you apply the patch before creating the grid view? I think your process is causing Twig to hang up or something because you're creating an old grid view first before applying the patch.

I'm not getting these errors, which doesn't even make sense (ln 107 of page.html.twig in Bartik is just a ending strong tag).

markhalliwell’s picture

tkoleary’s picture

@Mark Carver

Looks great. I would apply a default min width on the views grid column of 10rem so you don't get one word per line on smaller viewports.

tkoleary’s picture

@Mark Carver

Looks great. I would apply a default min width on the views grid column of 10rem so you don't get one word per line on smaller viewports.

(10rem because that's about 20 characters at the base font size. Any fewer looks janky in just about every context.)

markhalliwell’s picture

@tkoleary: I'm not sure about that. I know it may look a little janky, but was talking to @tim.plunkett last night in irc, that should really be left to the theme at that point. I don't think it should be up to the plugin to determine any extra styling. The only reason there is styling right now is to deal with the floated divs, structurally. Many themes/grid systems (like bootstrap) override these common properties, which min-width isn't.

markhalliwell’s picture

It's part of the reason I didn't add any media queries either. In reality the columns shouldn't float on mobile, but that's a separate issue (probably need to create an issue for Bartik to support styling of views grids).

markhalliwell’s picture

markhalliwell’s picture

R.Hendel’s picture

@Marc Carver
Thanks for your hint given in #50. Error messages seems to be caused by creating the view with an unpatched drupal and rendered that same view with a patched drupal.

After creating a grid-based-view on an patched drupal I couldn't reproduce that error. So I think everything is fine with your patch given in #44.

R.Hendel’s picture

Status: Needs review » Reviewed & tested by the community
tkoleary’s picture

@MarkCarver

You're right. It should be dealt with in Bartik, but also in Seven for things like media browser. Starting an issue for that

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

This patch didn't received a proper review yet, sorry. It would be great to test the automatic generation of the size at least.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.phpundefined
@@ -44,26 +44,39 @@ class Grid extends StylePluginBase {
+ $options['automatic_width'] = array('default' => TRUE, 'bool' => TRUE);
...
+ $options['default_col_class'] = array('default' => TRUE, 'bool' => TRUE);
+ $options['col_class_special'] = array('default' => TRUE, 'bool' => TRUE);

'bool' => TRUE is not needed anymore in drupal 8, so let's skip it.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.phpundefined
@@ -44,26 +44,39 @@ class Grid extends StylePluginBase {
   /**
-   * Render the given style.
+   * Build the options form.
    */
   public function buildOptionsForm(&$form, &$form_state) {

Let's use {@inheritdoc} instead.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.phpundefined
@@ -44,26 +44,39 @@ class Grid extends StylePluginBase {
+    $form['default_row_class']['#description'] = t('Add the default row classes like views-row, row-1 and clearfix to the output. You can use this to quickly reduce the amount of markup the view provides by default, at the cost of making it more difficult to apply CSS.');
+    $form['row_class_special']['#description'] = t('Add css classes to the first and last rows, as well as odd/even classes for striping.');

Why would you need special description for the grid example?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.phpundefined
@@ -71,20 +84,28 @@ public function buildOptionsForm(&$form, &$form_state) {
+    $form['col_class_special'] = array(
+      '#title' => t('Add striping (odd/even), first/last column classes'),
+      '#description' => t('Add css classes to the first and last columns, as well as odd/even classes for striping.'),
+      '#type' => 'checkbox',
+      '#default_value' => $this->options['col_class_special'],

I thought that you don't need to support that anymore in Drupal 8, as the supported browsers support everything which is needed so that a CSS only solution can be used.

+++ b/core/modules/views/templates/views-view-grid.html.twigundefined
@@ -4,32 +4,47 @@
-  <h3>{{ title }}</h3>
+<h3>{{ title }}</h3>

Let's not change this

tim.plunkett’s picture

#1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors hasn't been changed yet, so not sure what we should do here.

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
16.54 KB

Patch submitted by the Drush iq-submit command.

markhalliwell’s picture

Grr, didn't add the tests. Try this patch instead.

markhalliwell’s picture

Added some tests :)

'bool' => TRUE is not needed anymore in drupal 8, so let's skip it.

Removed.

Let's use {@inheritdoc} instead.

Fixed.

Why would you need special description for the grid example?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.php
@@ -33,28 +33,22 @@ class Grid extends StylePluginBase {
- protected $usesRowClass = TRUE;

I just went ahead and took this out because TBH, it's just easier to use grid based options instead of trying to fight with the base plugin form.

I thought that you don't need to support that anymore in Drupal 8, as the supported browsers support everything which is needed so that a CSS only solution can be used.

Per reading #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors, I just went ahead and took out all this first/last/zebra logic (which was a lot!). That issue basically says that, yes, we should take these out. The only reason that issue is still open is because there's a lot of core that still relies on that stuff (JS). I think we'll be fine taking them out here though, if anyone wants, they can preprocess via a theme and put them back in lol

markhalliwell’s picture

FileSize
17.14 KB

interdiff for #64

markhalliwell’s picture

w00t! Drupal\views\Tests\Plugin\StyleGridTest 26 passes!

R.Hendel’s picture

Status: Needs review » Reviewed & tested by the community

Tested patch #64 at simplytest.me:
- did create 50 nodes with devel
- created a view with a page which uses grid layout
- visited the page (and tested also pager)
- tried to find "table" or "< td >" in html-source, but there wasn't. Only < div >s...

So I think, this is fine.

dawehner’s picture

+1

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/StyleGridTest.phpundefined
@@ -0,0 +1,96 @@
+      $this->assertTrue(strpos($output, "<div class=\"views-view-grid $alignment cols-$columns clearfix\">") !== FALSE, ucfirst($alignment) . " grid markup displays correctly.");
...
+    $this->assertTrue(strpos($output, "views-col col-$columns$clearfix\" style=\"width: $width") !== FALSE, ucfirst($alignment) . " $columns column grid: automatic width calculated correctly.");

This should be better use $this->xpath, as everything else breaks way to easy.

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
markhalliwell’s picture

Status: Needs work » Needs review
FileSize
3.27 KB
22.32 KB

Converted testing to XPath

ZenDoodles’s picture

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

Thanks for your help on this @Mark Carver.

Since this changes tests (and the full patch adds tests) can we get a tests only version please?

markhalliwell’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll, -SprintWeekend2013 +VDC-cleanup
FileSize
22.35 KB
6.3 KB

Uploading tests and complete patch per: https://drupal.org/contributor-tasks/write-tests

R.Hendel’s picture

Status: Needs review » Reviewed & tested by the community

I repeated my test described in #68 and think, that's all ok now:

Tested patch #73 (1903746-complete.patch) at simplytest.me:
- did create 50 nodes with devel
- created a view with a page which uses grid layout
- visited the page (and tested also pager)
- tried to find "table" or "< td >" (without blanks of course...) in html-source, but there wasn't. Only < div >s...

So I think, this is alright.

markhalliwell’s picture

tim.plunkett’s picture

Issue tags: +RTBC July 1

Still July 1st in Alaska.

YesCT’s picture

Project: Drupal core » Views (for Drupal 7)
Issue summary: View changes

Updated issue summary.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Project: Views (for Drupal 7) » Drupal core
Issue tags: -Needs issue summary update

Updated issue summary. Do we still need an accessibility review on this?

dawehner’s picture

+1 for the RTBC

dawehner’s picture

Project: Drupal core » Views (for Drupal 7)
Issue summary: View changes

Updated issue summary

markhalliwell’s picture

Issue summary: View changes

Updated issue summary.

markhalliwell’s picture

Project: Views (for Drupal 7) » Drupal core
Issue tags: -Needs accessibility review
markhalliwell’s picture

Assigned: markhalliwell » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll...

git ac https://drupal.org/files/1903746-complete.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 22891  100 22891    0     0  21938      0  0:00:01  0:00:01 --:--:-- 42947
error: patch failed: core/modules/views/views.theme.inc:746
error: core/modules/views/views.theme.inc: patch does not apply
markhalliwell’s picture

Status: Needs work » Needs review
FileSize
15.82 KB
16.48 KB

Re-rolled

markhalliwell’s picture

Re-rolled with added files... ugh drush_iq

joelpittet’s picture

Twig nitpicks.
And may look into encorporating the object style later this week: #1968398: Convert Views $row_classes to $row['attributes']

+++ b/core/modules/views/templates/views-view-grid.html.twig
@@ -4,32 +4,47 @@
  * Available variables:
- * - attributes: HTML attributes for the table element.
+ * - attributes: HTML attributes for the wrapping div element.
  * - title: The title of this group of rows.
- * - rows: A list of rows. Each row contains a list of columns.
- * - row_classes: HTML classes for each row including the row number and first
- *   or last.
- * - column_classes: HTML classes for each column including the row number and
- *   first or last.
+ * - view: The view object.
+ * - rows: The rendered view results array.
+ * - options: The view plugin style options.
+ * - items: Contains a nested array of grid items. The structure of this array
+ * - depends on the value of options.alignment.
+ * - row_attributes: HTML attributes for each grid row. The structure of this
+ *   array depends on the value of options.alignment.
+ * - col_attributes: HTML attributes for each grid column. The structure of this
+ *   array depends on the value of options.alignment.
...
+      <div{{ row_attributes[col_key][row_key] }}>

Don't use the data type within the twig file (array). https://drupal.org/node/1823416#datatypes

+++ b/core/modules/views/templates/views-view-grid.html.twig
@@ -4,32 +4,47 @@
+ * @ingroup views_templates

Quite sure we aren't using views_templates anymore.

+++ b/core/modules/views/views.theme.inc
@@ -746,56 +746,98 @@ function template_preprocess_views_view_table(&$variables) {
+  $variables['attributes'] = new Attribute(array(

For performance reasons, this is done late and lazy. No need to instantiate a new Attribute, just leave it as an array.
#1982024: Lazy-load Attribute objects later in the rendering process only if needed

markhalliwell’s picture

Fixed markup to be more twig compliant

markhalliwell’s picture

Project: Drupal core » Views (for Drupal 7)
Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Project: Views (for Drupal 7) » Drupal core
Status: Needs review » Needs work

Excellent work! It's all cleaned up:)

Just one more bug I found and a nitpik.

col_class_custom

+++ b/core/modules/views/css/views.module.css
@@ -22,3 +22,13 @@
+/* grid style column align */

Capital 'G' and period.

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/style/Grid.php
@@ -72,20 +72,36 @@ public function buildOptionsForm(&$form, &$form_state) {
+      '#default_value' => $this->options['col_class'],

This line should be because it's saving just not showing the col class field value when re-opened:

+ '#default_value' => $this->options['col_class_custom'],

klonos’s picture

Status: Needs work » Needs review

Just a quick question: if this is an accessibility-related issue, then shouldn't it be backported to the 7.x branch of views?

klonos’s picture

Status: Needs review » Needs work

xpost

markhalliwell’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
22.56 KB

Ok fixed two issues in #86. @klonos: no, there is no need for a D7 backport. There is already a module that exists for D7 (http://dgo.to/views_responsive_grid). This issue only pertains to D8 with views in core.

dawehner’s picture

Additional changing html structure during a stable release would potentially break all CSS, so let us not even think about it.

dawehner’s picture

Project: Drupal core » Views (for Drupal 7)
Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Project: Views (for Drupal 7) » Drupal core
Status: Needs review » Reviewed & tested by the community

Great, I re-manually-tested #74 and it working great and the little col class saving issue is fixed!

Nice work!

  • [done] Accessibility Review - (last done in #48 by @mgifford.
  • [done] Manual Testing - (last done in #74)
  • [done] Code Review (last done in #61 and #69)
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5beb2f9 and pushed to 8.x. Thanks!

markhalliwell’s picture

Title: Replace the views grid table template with one using divs » [Change notice] Replace the views grid table template with one using divs
Priority: Normal » Critical
Status: Fixed » Active
Issue tags: -Accessibility, -VDC, -VDC-cleanup, -RTBC July 1 +Needs change record
markhalliwell’s picture

Status: Active » Fixed
Issue tags: -Needs change record

Created change notice at: https://drupal.org/node/2043385

Xano’s picture

Title: [Change notice] Replace the views grid table template with one using divs » Replace the views grid table template with one using divs
klonos’s picture

Priority: Critical » Normal

...

alexpott’s picture

Title: Replace the views grid table template with one using divs » [Change notice] Replace the views grid table template with one using divs
Project: Drupal core » Views (for Drupal 7)
Version: 8.x-dev » 8.x-3.x-dev
Component: views.module » Code
Status: Fixed » Active
Issue tags: +Needs change record

This needs a change record.

xjm’s picture

Or rather, we just need to move https://drupal.org/node/2043385 to views instead of core.

xjm’s picture

Issue summary: View changes

Updated issue summary.

Les Lim’s picture

Status: Active » Fixed
Issue tags: -Needs change record

Moved change notice per #98.

markhalliwell’s picture

Title: [Change notice] Replace the views grid table template with one using divs » Replace the views grid table template with one using divs

Status: Fixed » Closed (fixed)

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