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
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.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#89 | drupal-replace-the-views-1903746-89.patch | 22.56 KB | markhalliwell |
#89 | interdiff.txt | 1.25 KB | markhalliwell |
#85 | drupal-replace-the-views-1903746-85.patch | 22.55 KB | markhalliwell |
#85 | interdiff.txt | 8.2 KB | markhalliwell |
#83 | drupal-replace-the-views-1903746-82.patch | 22.45 KB | markhalliwell |
Comments
Comment #1
michaelfavia CreditAttribution: michaelfavia commentedIve 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?
Comment #2
tim.plunkettdisplay: inline-block;
seems like a good candidate.Comment #3
damiankloip CreditAttribution: damiankloip commentedWouldn't/couldn't this be covered by the twig guys in their template conversion?
Comment #4
tim.plunkettNo, because they should be doing 1:1 conversions.
Also making the divs is the "easy" part compared to the CSS.
Comment #5
damiankloip CreditAttribution: damiankloip commentedMeh, 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?
Comment #6
michaelfavia CreditAttribution: michaelfavia commentedSpoke 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. ;)
Comment #7
larowlanTables for layout is poor a11y.
Comment #8
mgiffordWith 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.
Comment #9
DaneMacaulay CreditAttribution: DaneMacaulay commentedHeres 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.
Comment #10
dawehnerGreat 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.
Comment #11
DaneMacaulay CreditAttribution: DaneMacaulay commentedGood idea, this would do it:
is ctools_css_cache() available/refactored in 8.x ?
Comment #12
dasjohi,
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...
Comment #13
mgiffordI 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.
Comment #14
forestmars CreditAttribution: forestmars commentedThat's about 48 hours away.
Comment #15
dawehnerAre 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.
Comment #16
tim.plunkettIt's categorized as a task for a reason.
Comment #17
nikkubhai CreditAttribution: nikkubhai commentedOne 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?
Comment #18
dawehnerWell, wouldn't porting to divs sort of make it responsible automatically? Table views are responsible, because we ported them to be so.
Comment #19
nikkubhai CreditAttribution: nikkubhai commented@dawehner: Sorry, I have no idea about that. :/
Comment #20
nikkubhai CreditAttribution: nikkubhai commentedWe can get some help from this module http://drupal.org/project/views_responsive_grid . I will ask their maintainers.
Comment #21
iwhitcomb CreditAttribution: iwhitcomb commentedSo where do we begin getting views_responsive_grid into D8?
Comment #22
tim.plunkett@iwhitcomb, first, port the module to work in D8. Then we can revamp it as part of views itself.
Comment #23
dawehnerYeah 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.
Comment #24
derhasi CreditAttribution: derhasi commentedI 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.
Comment #25
derhasi CreditAttribution: derhasi commentedComment #26
markhalliwellJust 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.
Comment #27
markhalliwellAfterthought: 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.
Comment #28
dawehner@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.
Comment #29
markhalliwell@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.
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.
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.
Comment #30
dawehner@mark Carver
If you want to display something as a grid, the markup should be divs. That's it.
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.
Comment #31
markhalliwellI 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).
Comment #32
dawehnerNo problem. Please try out the responsive table feature in core, it's really great!
Comment #33
mgiffordIs 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.
Comment #34
tkoleary CreditAttribution: tkoleary commentedSee 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
Comment #35
tkoleary CreditAttribution: tkoleary commentedComment #36
dawehnerOne 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.
Comment #37
tkoleary CreditAttribution: tkoleary commented@dawhener
I *think* that's right.
Comment #38
mgiffordJust 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
Comment #39
mgiffordThis 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.
Comment #40
markhalliwellA 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.
Comment #41
mgiffordBrilliant. Thanks Mark!
Comment #42
markhalliwellAlrighty, here's the patch. I'll commit the same code for the 8.x branch of the views_responsive_grid module, just in case.
Comment #43
markhalliwellFixed the views style schema. Changed the twig row and column ids to
_key
to make more sense.Comment #44
markhalliwellFixed two mapped types in the views schema
Comment #46
markhalliwell#44: drupal-replace-the-views-1903746-43.patch queued for re-testing.
Comment #47
R.Hendel CreditAttribution: R.Hendel commentedI 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:
So - do I need any more patch files or is something wrong with it?
Comment #48
mgiffordTry #44.
No error messages there that I could see. Good step ahead for a grid layout.
All via SimplyTest.me!
Comment #49
R.Hendel CreditAttribution: R.Hendel commentedI 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.
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?
Comment #50
markhalliwell@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).
Comment #51
markhalliwellPS: not sure if you use Dreditor, but you can use test this patch at: http://simplytest.me/project/drupal/8.x?patch[]=https://drupal.org/files/drupal-replace-the-views-1903746-43_0.patch
Comment #52
tkoleary CreditAttribution: tkoleary commented@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.
Comment #53
tkoleary CreditAttribution: tkoleary commented@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.)
Comment #54
markhalliwell@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.
Comment #55
markhalliwellIt'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).
Comment #56
markhalliwellref: #2031447: Add new views-grid styling to Bartik
Comment #57
markhalliwellSee screenshots for #2031447-2: Add new views-grid styling to Bartik patch.
Comment #58
R.Hendel CreditAttribution: R.Hendel commented@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.
Comment #59
R.Hendel CreditAttribution: R.Hendel commentedComment #60
tkoleary CreditAttribution: tkoleary commented@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
Comment #61
dawehnerThis 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.
Let's use {@inheritdoc} instead.
Why would you need special description for the grid example?
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.
Let's not change this
Comment #62
tim.plunkett#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.
Comment #63
markhalliwellPatch submitted by the Drush iq-submit command.
Comment #64
markhalliwellGrr, didn't add the tests. Try this patch instead.
Comment #65
markhalliwellAdded some tests :)
Removed.
Fixed.
+++ 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.
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
Comment #66
markhalliwellinterdiff for #64
Comment #67
markhalliwellw00t! Drupal\views\Tests\Plugin\StyleGridTest 26 passes!
Comment #68
R.Hendel CreditAttribution: R.Hendel commentedTested 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.
Comment #69
dawehner+1
This should be better use $this->xpath, as everything else breaks way to easy.
Comment #70
dawehnerComment #71
markhalliwellConverted testing to XPath
Comment #72
ZenDoodles CreditAttribution: ZenDoodles commentedThanks 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?
Comment #73
markhalliwellUploading tests and complete patch per: https://drupal.org/contributor-tasks/write-tests
Comment #74
R.Hendel CreditAttribution: R.Hendel commentedI 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.
Comment #75
markhalliwellComment #76
tim.plunkettStill July 1st in Alaska.
Comment #76.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #76.1
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #76.2
markhalliwellUpdated issue summary.
Comment #76.3
markhalliwellUpdated issue summary.
Comment #77
markhalliwellUpdated issue summary. Do we still need an accessibility review on this?
Comment #78
dawehner+1 for the RTBC
Comment #78.0
dawehnerUpdated issue summary
Comment #78.1
markhalliwellUpdated issue summary.
Comment #79
markhalliwellComment #80
markhalliwellComment #81
alexpottNeeds a reroll...
Comment #82
markhalliwellRe-rolled
Comment #83
markhalliwellRe-rolled with added files... ugh drush_iq
Comment #84
joelpittetTwig nitpicks.
And may look into encorporating the object style later this week: #1968398: Convert Views $row_classes to $row['attributes']
Don't use the data type within the twig file (array). https://drupal.org/node/1823416#datatypes
Quite sure we aren't using views_templates anymore.
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
Comment #85
markhalliwellFixed markup to be more twig compliant
Comment #85.0
markhalliwellUpdated issue summary.
Comment #86
joelpittetExcellent work! It's all cleaned up:)
Just one more bug I found and a nitpik.
col_class_custom
Capital 'G' and period.
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'],
Comment #87
klonosJust a quick question: if this is an accessibility-related issue, then shouldn't it be backported to the 7.x branch of views?
Comment #88
klonosxpost
Comment #89
markhalliwellOk 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.
Comment #90
dawehnerAdditional changing html structure during a stable release would potentially break all CSS, so let us not even think about it.
Comment #90.0
dawehnerUpdated issue summary.
Comment #91
joelpittetGreat, I re-manually-tested #74 and it working great and the little col class saving issue is fixed!
Nice work!
Comment #92
alexpottCommitted 5beb2f9 and pushed to 8.x. Thanks!
Comment #93
markhalliwellComment #94
markhalliwellCreated change notice at: https://drupal.org/node/2043385
Comment #95
XanoComment #96
klonos...
Comment #97
alexpottThis needs a change record.
Comment #98
xjmOr rather, we just need to move https://drupal.org/node/2043385 to views instead of core.
Comment #98.0
xjmUpdated issue summary.
Comment #99
Les LimMoved change notice per #98.
Comment #100
markhalliwell