backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 39 forks source link

Update the views Grid style to use the CSS grid property #2811

Closed jenlampton closed 5 years ago

jenlampton commented 7 years ago

This is a follow-up to https://github.com/backdrop/backdrop-issues/issues/479.

UPDATE: This Issue is postponed on browser support: https://caniuse.com/#feat=css-grid

Currently, the Views Grid style uses an HTML table that is hard to make responsive. The grid element markup should be rewritten to use DIV tags so that it can be more easily rearranged on smaller screen devices.

Most modern browsers now support the CSS grid property: https://css-tricks.com/snippets/css/complete-guide-grid/


PR https://github.com/backdrop/backdrop/pull/1988 PR https://github.com/backdrop/backdrop/pull/2447

sacarney commented 7 years ago

Hello, new to Backdrop, but experienced with flexbox and grid.

Why has it been decided to use CSS grid instead of flexbox? In my experience with things like this, flexbox is a better choice. An advantage flexbox has over grid is flexbox's flexibility with column number and column width. This allows us to set up a layout with...

Grid has a much harder time with this.

We use flexbox for our Drupal 7 views and we really like it. A flexbox solution here wouldn't need to be as mighty as Bootstrap 4's which allows a lot of atomic control - it could just be initial # of columns.

jenlampton commented 7 years ago

Hi @sacarney, and welcome! Thanks for your feedback. :) We aren't 100% committed to grid yet but we need to decide soon. The problems we ran into with flex are outlined below.

This allows us to set up a layout with...

We are already using flexbox (and the Bootstrap4 grid system) for Layouts in Backdrop. This issue is specifically about fixing the Views "Grid" style, which (most of the time) will not be used for "layout". (Not sure if that's what you meant by "layout"... probably not?)

That said, in the original issue we did discuss using either the existing grid system, or adding new CSS using flexbox without the grid system, but we ran into a few problems.

First, modifying the existing Views style means that if we added any additional settings to the view, update hooks would need to be run for all existing sites using the Views grid style. And afterward, the rendered output should appear the same way after the change as it did before.

Here are the existing settings we're working with:

screen shot 2017-08-25 at 5 43 49 pm

I'm not sure we'd be able to get flexbox to look the same way the HTML table does, even with making assumptions for that update hook. For example, Flexbox does not behave the same way as grid if there aren't enough columns in a row. (this might be what you meant by decent control over leftover items.) Here's an example of how flex divs span too many columns instead of retaining the intended width: flex

At a later time we could easily add another "Flex" Views style that used flexbox instead of grid. That way people who know their CSS could choose the Grid style when they want grid and choose the Flex style when they want flex.

The goal for this issue is to end up with markup that can be made responsive, we don't need to do the responsive bit in core. As long as we are using divs, themes could change it from grid to flex if they wanted!

We just need to get the heck away from using these hard-to-make-responsive HTML tables, and preferably by the release of backdrop 1.8! :)

sacarney commented 7 years ago

This makes total sense! I appreciate your taking the time to break it down. I'll fiddle with it and see what I can come up with.

jenlampton commented 7 years ago

Hi @sacarney did you have any luck on this task over the weekend? Even if you didn't get it done we'd love to see any code you may have, as it could still be valuable for the next person to pick this up :) Or, feel free to join us in Gitter if you want to chat about it!

sacarney commented 7 years ago

Thank you for the reminder. I got caught up with work things.

I started playing with a display: grid to mimic a grid View I set up in a test install of Backdrop.

https://codepen.io/biology-it/pen/e1cc35a621fa4b9943f52d089d9f352c (one may wish to fork this pen to fiddle with it.)

Notes:

  1. All grid items are direct children of the grid container.
  2. The number of columns is controlled by a class on the grid container. Currently I have 3 and 4 columns.
  3. The grid is flexible, filling available space, but not responsive, wrapping columns as the screen narrows.
  4. I can't quite remember if I've properly mimicked existing grid View markup...

I hope this is helpful! The CSS for Grid is so clean! I quite like it.

jenlampton commented 7 years ago

This looks fantastic, thanks @sacarney :) I'll see if I can't get this into a Backdrop sandbox for further testing.

jenlampton commented 7 years ago

I just did a quick review of browser support for css grid, and we're looking pretty good, but it may still be a little early to put this in core: http://caniuse.com/#feat=css-grid

Nevertheless, we can start testing this PR: https://github.com/backdrop/backdrop/pull/1988 I haven't done the vertical-alignment bit, but the number of columns should be working (up to 12, anyway)

jenlampton commented 7 years ago

I'm going to bump this issue to 1.9 so we have some more time for browsers to be updated to versions that support grid

quicksketch commented 6 years ago

This is currently marked as a bug report, but it might just be a feature(?)

The PR code looks excellent in implementation! I am worried about changing the markup out from under people. Should this be an option instead of wholesale replacing the existing table-based grid markup?

jenlampton commented 6 years ago

It's a bug that the current approach can't be made responsive, is it also a feature that we're fixing the bug? :/ Not sure about that.

I am worried about changing the markup out from under people.

If the end result is the same (three column grid remains three column grid) I'm not so worried abut it. It's likely that people who have styled the three column grid have also already fixed the bug by overriding the template, if that was needed. If they've done that, nothing would change for them with this fix. If they haven't overridden the template, then the three column grid remains three columns, and the only problems introduced would be because the CSS can't find the old table selectors.

What I have a bigger problem with would be including 2 ways to make a grid layout for a view in core.

Since we don't yet have an timeframe for when browser support would be sufficient to make this change, I'd also be okay with kicking it to 2.0.

quicksketch commented 6 years ago

Good points on the template overriding. I had been concerned about users that may have added CSS to style the table itself. You're right that users who overrode the HTML would not be affected, but if CSS was used to increase padding between items in the table cells, that would probably be targeted against the HTML table markup. e.g.

.views-view-my-view td {
  padding: 20px;
}

Though using CSS to style the grid layout is a less-likely scenario, it may still be possible. Perhaps a compromise here is just to make sure that the old tpl.php files would still work if needed. That is, a developer could drop in the old table-based template file into their theme if needed to restore the old behavior. I'll do another pass on the code to see if it accommodates this possibility.

jenlampton commented 6 years ago

bumping milestone.

jenlampton commented 6 years ago

Since IE11 still doesn't support CSS grid pushing this issue to 1.11.

docwilmot commented 6 years ago

The plan for this is to wait till Grid has more browser support? I really would like to get rid of those tables, and we really don't know when Microsoft will get their act together.

klonos commented 5 years ago

Please refer to my comment in https://github.com/backdrop/backdrop-issues/issues/214#issuecomment-434863843 re IE global usage stats from various sources.

For css grid specifically, here's what caniuse.com has to say:

screen shot 2018-11-01 at 9 05 38 am

...so basically ~90% of the web can use it.

"Partial support" in IE refers to supporting an older version of the specification.

Perhaps we should use that older version along with the commonly-used one and actually get this done?

herbdool commented 5 years ago

Good point @klonos. According to https://medium.com/@elad/supporting-css-grid-in-internet-explorer-b38669e75d66 we could add prefixes via a tool to make it compatible with ie 11.

Though we'd still need to ensure that it's backwards compatible for people who have styled their site expecting the table HTML.

herbdool commented 5 years ago

When I put the CSS in the autoprefixer I get this:

/* Views grid styles */
        .views-view-grid {
          display: -ms-grid;
          display: grid;
        }
        .views-view-grid-cols-1 {
          -ms-grid-columns: (1fr)[1];
              grid-template-columns: repeat(1, 1fr);
        }
        .views-view-grid-cols-2 {
          -ms-grid-columns: (1fr)[2];
              grid-template-columns: repeat(2, 1fr);
        }
        .views-view-grid-cols-3 {
          -ms-grid-columns: (1fr)[3];
              grid-template-columns: repeat(3, 1fr);
        }
        .views-view-grid-cols-4 {
          -ms-grid-columns: (1fr)[4];
              grid-template-columns: repeat(4, 1fr);
        }
        .views-view-grid-cols-5 {
          -ms-grid-columns: (1fr)[5];
              grid-template-columns: repeat(5, 1fr);
        }
        .views-view-grid-cols-6 {
          -ms-grid-columns: (1fr)[6];
              grid-template-columns: repeat(6, 1fr);
        }
        .views-view-grid-cols-7 {
          -ms-grid-columns: (1fr)[7];
              grid-template-columns: repeat(7, 1fr);
        }
        .views-view-grid-cols-8 {
          -ms-grid-columns: (1fr)[8];
              grid-template-columns: repeat(8, 1fr);
        }
        .views-view-grid-cols-9 {
          -ms-grid-columns: (1fr)[9];
              grid-template-columns: repeat(9, 1fr);
        }
        .views-view-grid-cols-10 {
          -ms-grid-columns: (1fr)[10];
              grid-template-columns: repeat(10, 1fr);
        }
        .views-view-grid-cols-11 {
          -ms-grid-columns: (1fr)[11];
              grid-template-columns: repeat(11, 1fr);
        }
        .views-view-grid-cols-12 {
          -ms-grid-columns: (1fr)[12];
              grid-template-columns: repeat(12, 1fr);
        }
        .views-view-grid .views-row {
          padding: 1rem;
          border: 1px solid #ccc;
        }

Excuse the extraneous spaces added by the tool.

herbdool commented 5 years ago

Perhaps for backwards compatibility (for users like me that used CSS to modify the table grid) there could be a toggle to allow users to stick with the old table but default to CSS grid. It seems we can't just leave the tpl but also need to preserve the options users made with the grid.

herbdool commented 5 years ago

I took a stab at this to see if we can have it sooner. I've added prefixes for IE11, which was what was holding this up. I haven't tested it on IE11.

And I've added a toggle so that people can still use the old table if they absolutely need it. From the UI the old settings are hidden unless toggled. The toggle is an alternative to creating a separate format which was what @jenlampton wanted to avoid. The toggle will enable the old fields which we want to keep around since people might have been using them.

While the code is not quite as clean, it does help by not breaking anyone's existing websites which is what @quicksketch was concerned about. In my first iteration people need to edit their view if they want to use the css grid, but if we were to be more bold we could force the css grid on people and require them to edit the view to enable the deprecated table again. I'm not sure how people feel about either approach.

docwilmot commented 5 years ago

Working on this

docwilmot commented 5 years ago

I've done a new PR, with minor changes. There never were tests for grid views and I cant think how I'd test this, so can we get an eyeball and a RTBC and we can make an issue for tests as a followup? I think this has languished long enough, and the option for existing sites to switch back to old time tables is a good compromise

jenlampton commented 5 years ago

I've found an issue when using the views wizard to set grid view, it still defaults to rendering in a HTML table. STR left on the PR, and an example in the sandbox.

docwilmot commented 5 years ago

Fixed all issues, thanks.

jenlampton commented 5 years ago

testing again now! :)

jenlampton commented 5 years ago

confirmed fixed! :) RTBC.

quicksketch commented 5 years ago

This looks great! Should we update existing views to use the deprecated table? Or should we convert those over to flexbox upon updating to 1.12.0 (which would happen automatically if no update hook is created)? I think we could merge this in and then decide on that in the remaining time before Jan 15.

herbdool commented 5 years ago

I guess I'll close my PR too https://github.com/backdrop/backdrop/pull/2351 since @docwilmot extended it with his PR.

quicksketch commented 5 years ago

I've merged https://github.com/backdrop/backdrop/pull/2447 into 1.x for 1.12. Thanks @docwilmot, @jenlampton, @sacarney, and @herbdool!

herbdool commented 5 years ago

@quicksketch with this line in views.theme.inc: $variables['deprecated_table'] = (!isset($options['deprecated_table']) || $options['deprecated_table']) ? TRUE : FALSE; we are assuming it's using the deprecated table unless they edit the view and select otherwise.

jenlampton commented 5 years ago

This seems to have broken all views grid themes that used the previous render array structure. See https://github.com/backdrop/backdrop-issues/issues/3494