New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-grid] Autoplacement #1148

Closed
evandiamond opened this Issue Oct 19, 2018 · 25 comments

Comments

Projects
None yet
4 participants
@evandiamond

evandiamond commented Oct 19, 2018

There's an issue with IE11 and Edge if using grid-gap, where the proper -ms- prefixes and additions aren't added. There's a mixin (linked below), which makes things better, however the author @ksenzee mentioned it needs to be more generalized.

Grid Gap IE fix mixin:
https://gist.github.com/ksenzee/276d60f3e251b1dfafaf52ed8dbdb0de

Codepen created to illustrate issue:
https://codepen.io/hyperfy/pen/ReyzdY (editor mode isn't working in IE)
https://codepen.io/hyperfy/full/ReyzdY

How code should look:

.grid {
    display: grid;
    grid-template-columns: 1fr 1fr 1fr;
    -ms-grid-columns: 1fr 30px 1fr 30px 1fr;
    grid-template-rows: auto;
    grid-gap: 30px;

    > *:nth-child(1) {
        grid-column: 1;
        grid-row: 1;
        -ms-grid-column: 1;
        -ms-grid-row: 1;
    }

    > *:nth-child(2) {
        grid-column: 2;
        grid-row: 1;
        -ms-grid-column: 3;
        -ms-grid-row: 1;
    }

    > *:nth-child(3) {
        grid-column: 3;
        grid-row: 1;
        -ms-grid-column: 5;
        -ms-grid-row: 1;
    }
}

Basically, the grid children (or columns) need to be properly mapped out as IE11 sees each gap as an additional column. Columns must be moved to accommodate IE11.

How can we integrate this into Autoprefixer? I think it's a life-saver and an important step forward for CSSGrid adoption in terms of browser compatibility.

Thank you so much!

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 19, 2018

Read this: https://css-tricks.com/css-grid-in-ie-css-grid-and-the-new-autoprefixer/

Autoprefixer already has grid-gap support but you need to define grid-areas for it to work.

The issue is that without access to the DOM, we can't know what grid cell belongs to what grid unless the user uses area names.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 19, 2018

...although your suggestion isnt bad.

We don't support auto-placement at the moment but your code sample could potentially be implemented :)

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 19, 2018

The number of rows would need to be defined. To dictate the limit to which Autoprefixer generates nth-child selectors.

In your example grid-template-rows: auto; generates 3 nth child selectors.

If you used grid-template-rows: auto auto; Autoprefixer would generate 6 nth-child selectors.

I like this idea, it makes it possible for Autoprefixer to handle autoplacement! 😀

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 19, 2018

@bogdan0083 how difficult do you think this would be to implement?

@ai ai changed the title from [css-grid] IE11 grid-gap support to [css-grid] Autoplacement Oct 19, 2018

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 20, 2018

This would be opening up a HUGE can of worms but it would be great if we can get the basic functionality implemented and then slowly deal with the edge cases as they come up.

The autoplacement algorithm would need to kick in if both grid-template-rows and grid-template-columns have been defined but not grid-template-areas. If the user uses grid-template-areas then it needs to switch to the grid areas algorithm.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 20, 2018

This is what the output code would actually look like:

.grid {
    display: -ms-grid;
    display: grid;
    -ms-grid-columns: 1fr 30px 1fr 30px 1fr;
    grid-template-columns: 1fr 1fr 1fr;
    -ms-grid-rows: auto;
    grid-template-rows: auto;
    grid-gap: 30px;
}
.grid > *:nth-child(1) {
    -ms-grid-column: 1;
    -ms-grid-row: 1;
}

.grid > *:nth-child(2) {
    -ms-grid-column: 3;
    -ms-grid-row: 1;
}

.grid > *:nth-child(3) {
    -ms-grid-column: 5;
    -ms-grid-row: 1;
}
@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 20, 2018

@bogdan0083 how difficult do you think this would be to implement?

I don't think it would be very difficult to implement actually, but I don't have much time/desire for this issue. 😕

For someone who wants to implement this feature, here's my tips and thoughts:

  • To make this feature, try to understand how Autoprefixer adds prefixes for grid-template-area property. Go to lib/hacks/grid-template-areas.js and play with the code, use console.log.
  • We now have playground, you can add the code to playground/input.css, run gulp play and see the results at playground/output.css. The code will be updated on every save. That's awesome! 😄
  • We already have lots of utility functions implemented for grids in lib/hacks/grid-utils.js. Again, check out grid-template-area implementation. You mostly need to reuse the functions that are used there.
  • Refer to postcss docs to understand how to create and append new rules.
  • Add tests.
  • Ask questions. We are always eager to help! 😀
@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 20, 2018

Wahhh?!? 😮

How can you not be excited by the prospect of CSS Grid autoplacement in IE?

Being able to set up a quick 3 column layout with grid gaps with just 4 lines of CSS would be so awesome!

I can't make you do anything you don't want to, but I can't fathom how you aren't excited by the idea of autoplacemt support in Autoprefixer. 😕

Thanks for the tips though, that will help anyone interested in trying to implement this feature. 😄

@evandiamond

This comment has been minimized.

evandiamond commented Oct 20, 2018

Agree completely @Dan503 - it's exciting! Being able to create the Grid with cross-browser compatibility baked in (especially for IE11+) would allow for native Autoplacement. You could essentially strip Bootstrap out completely!

@bogdan0083 do you think you could implement some of this to get us started? 😇

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 20, 2018

Agree completely @Dan503 - it's exciting! Being able to create the Grid with cross-browser compatibility baked in (especially for IE11+) would allow for native Autoplacement. You could essentially strip Bootstrap out completely!

We already have grid-template and grid-template-areas, where grid-gaps are fully supported. I feel like using grid-template(-areas) is way more intuitive and handy (in my opinion 😀). But I agree that this feature is also interesting and useful.

@bogdan0083 do you think you could implement some of this to get us started? 😇

I might take a look at it later on (in a week or two). 🙂

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 20, 2018

I feel like using grid-template(-areas) is way more intuitive and handy.

Autoplacement is for a completely different use case.

grid-template-areas is useful when you are laying out different types of elements into a known grid layout. Like putting the header in the header area and the footer in the footer area.

Autoplacement is for when you have a repeating pattern of the same type of element and you often aren't even sure of how many elements you have. Think of a news listing where each news item is placed into a card in a 3 column grid with a 20px gap between each card. I'm not going to want to write CSS that looks like this:

.news {
  display: grid;
  grid-template-columns: repeat(3, 1fr);
  grid-gap: 20px;
  grid-template-areas:
    "news-a news-b news-c"
    "news-d news-e news-f"
    "news-g news-h news-i";
}
.news-item:nth-child(1) {
  grid-area: news-a;
}
.news-item:nth-child(2) {
  grid-area: news-b;
}
.news-item:nth-child(3) {
  grid-area: news-c;
}
.news-item:nth-child(4) {
  grid-area: news-d;
}
.news-item:nth-child(5) {
  grid-area: news-e;
}
.news-item:nth-child(6) {
  grid-area: news-f;
}
.news-item:nth-child(7) {
  grid-area: news-g;
}
.news-item:nth-child(8) {
  grid-area: news-h;
}
.news-item:nth-child(9) {
  grid-area: news-i;
}

I would MUCH rather write CSS that looked like this:

.news {
  display: grid;
  grid-template-columns: repeat(3, 1fr);
  grid-template-rows: repeat(3, auto);
  grid-gap: 20px;
}

Does the idea of that really not excite you @bogdan0083?


@bogdan0083 do you think you could implement some of this to get us started? 😇

I might take a look at it later on (in a week or two). 🙂

That would help a lot, thanks. 😊

Maybe get us to the point where it is outputting a single nth-child selector and we can figure out the functionality for calculating the rest of them. I'd really like you to manage the functionality for switching between the areas algorithm (rows + areas defined), the autoplacement algorithm (rows + columns and not areas defined), and no algorithm. I think that will require a lot of understanding of how the areas system works.

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 21, 2018

Does the idea of that really not excite you @bogdan0083?

Hmm, now it looks way more promising 🤔. Thanks Dan! 😀

Maybe get us to the point where it is outputting a single nth-child selector and we can figure out the functionality for calculating the rest of them. I'd really like you to manage the functionality for switching between the areas algorithm (rows + areas defined), the autoplacement algorithm (rows + columns and not areas defined), and no algorithm. I think that will require a lot of understanding of how the areas system works.

I don't work with css grids very often nowadays, so just to make sure we're on the same line here:

  • We use areas algorithm if grid-template-areas (+ rows) or grid-template defined.
  • We use autoplacement algorithm if rows and columns defined and no grid-template-areas or grid-template defined.
  • If we use autoplacement, the rows are required. If user defined only columns, we must warn him to define rows, otherwise autoplacement won't work.

Is that right?

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 21, 2018

Yep, those are exactly the rules I have in mind. 😁

Thanks Bogdan, you're way better at adding new features than I am and this is a complicated one. 😊

Now that I've got you on board, I think I will wait for this feature to be completed before publishing the CSS Tricks article so I can include this in it. CSS Tricks aren't going to want to publish 2 Autoprefixer articles so close to one another so I would rather merge them into one big article.

@ai, this is a big enough new feature that it would count as a minor (feature) update.

@ai

This comment has been minimized.

Member

ai commented Oct 21, 2018

this is a big enough new feature that it would count as a minor (feature) update.

Of course. “New feature” according to SemVer.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 21, 2018

  • We use areas algorithm if grid-template-areas (+ rows) or grid-template defined.

Sorry, it's actually grid-template-areas (+columns) or grid-template is defined.

Rows are optional when using the areas technique because they default to auto.

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 23, 2018

@Dan503 I think I've just found a bug in Autoprefixer, but I'm not sure.

If I use a grid with this grid-template-rows/columns value:

.grid {
  display: grid;
  grid-template-columns: 123px repeat(3, 1fr 2fr);
  grid-template-rows: auto;
  grid-gap: 20px;
}

It compiles to the following:

.grid {
  display: -ms-grid;
  display: grid;
  /* note the commas */
  -ms-grid-columns: 123px 20px 1fr, ,2fr 20px 1fr, ,2fr 20px 1fr, ,2fr;
  grid-template-columns: 123px repeat(3, 1fr 2fr);
  -ms-grid-rows: auto;
  grid-template-rows: auto;
  grid-gap: 20px;
}

But if I comment out the grid-gap value:

.grid {
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 123px (1fr 2fr)[3];
  grid-template-columns: 123px repeat(3, 1fr 2fr);
  -ms-grid-rows: auto;
  grid-template-rows: auto;
  /* grid-gap: 20px; */
}

Are those commas an expected behaviour? I expect the value to be:

-ms-grid-columns: 123px 20px 1fr 20px 2fr 20px 1f 20px 2fr 20px 1fr 20px 2fr;
@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 23, 2018

The commas are not supposed to be there.

You have definitely found a bug.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 23, 2018

Can we also add support for grid-auto-flow: column;?

https://developer.mozilla.org/en-US/docs/Web/CSS/grid-auto-flow

grid-auto-flow: row is the default value and it is what this issue has covered so far. When the user uses grid-auto-flow: column; it switches the direction of the auto-placement from left to right to top to bottom.

/* Using grid-auto-flow:  row; */

.grid {
  display: grid;
  grid-auto-flow:  row; /* <- set to row */
  grid-template-columns: repeat(3, 1fr);
  grid-template-rows: repeat(3, auto);
  grid-gap: 20px;
}

/*
-------->
-------->
-------->
*/
/* Using grid-auto-flow:  column; */

.grid {
  display: grid;
  grid-auto-flow:  column; /* <- set to column */
  grid-template-columns: repeat(3, 1fr);
  grid-template-rows: repeat(3, auto);
  grid-gap: 20px;
}

/*
| | |
| | |
V V V
*/

I don't think we can support the dense keyword though. We will need to warn users not to use that.

@ai ai added the enhancement label Oct 23, 2018

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 23, 2018

Writing mode also has an effect on what direction the grid cells are placed in.

https://developer.mozilla.org/en-US/docs/Web/CSS/writing-mode

We might want to save that for a future release though after the initial feature has been built.

Writing mode support is needed for some non-English speaking countries. Arabia and Japan are countries that come to mind that don't use left to right, top to bottom writing mode.

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 23, 2018

Alright, I fixed the comma bug, but I faced another one.

The code grid-template-columns: 123px repeat(2, 1fr 2fr); should compile to:

-ms-grid-columns: 123px 20px 1fr 2fr 20px 1fr 2fr;

or

-ms-grid-columns: 123px 1fr 20px 2fr 20px 1fr 20px 2fr;

Right now it compiles to the first example, and I don't think it is right.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 23, 2018

Neither is right.

The gap needs to be placed between every column and every row like this:

-ms-grid-columns: 123px 20px 1fr 20px 2fr 20px 1fr 20px 2fr;
@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 23, 2018

Oh, I made a typo in the second example, it supposed to be like yours 😀

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 23, 2018

Once you get that bug totally fixed, can you open a PR for it so that it can be released as a patch?

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 23, 2018

Yeah, I was planning to send a separate PR for this bug

ai added a commit that referenced this issue Dec 3, 2018

[css-grid] Implement autoplacement (#1148) (#1151)
* [css-grid] Implement Autoplacement feature

* [css-grid] Add test cases for autoplacement

* [css-grid] fix grid-template-rows/columns values

* [css-grid] add support for 'grid-auto-flow' property in autoplacement

* [refactor] move part of functionality to grid-rows-columns hack

* [css-grid] Add test cases for grid-auto-flow and for rows warning

* update size limit

* [css-grid] Comment out grid-auto-flow warning (at least for now)

* [css-grid] Show warning when grid-auto-flow: dense is used

* [css-grid] add FIXME about commented out warning

* update size limit again to make CI pass

* [css-grid] Show grid-auto-flow warning only if grid template is present

* [css-grid] Fix warnings

* [refactor] Add comments

* [css-grid] Add support for grid: 'autoplace'

* [css-grid] Remove the unneeded output

Now as we support grid: 'autoplace' option, autoplacement works only where defined,
so autoplacement won't work in grid-status.css now.

* update size limit value

* [css-grid] Add autoplace conditional comment

* [css-grid] Add test cases with autoplace conditional comments

* [playground] Add support for grid autoplacement

* [refactor] Remove todo comment

* [css-grid] Change autoplace conditional comment and fix test cases

* update size limit value

* update readme.md

* update readme 2

* Adding a table of contents

(install this VS Code extension to easily keep it up to date: https://marketplace.visualstudio.com/items?itemName=yzhang.markdown-all-in-one#overview)

* Fixing FAQ heading levels

The TOC wasn't generating correctly because the FAQ heading levels were incorrect

* Grid autoplacement readme updates

* (alias for deprecated value `on`) text in readme

* [css-grid] fix grid areas algorithm

* [css-grid] Add test to check the output for different grid options

* Updating readme with "no-autoplace" grid setting.

* Readme "Do not create `::before` and `::after` pseudo elements" section

* Fixing ::before/::after example in readme

* Removed word "both" (there are 3 items)

* Minor readme fixes

* [css-grid] Add warning when grid-gap is used without template or autoplacement

* [css-grid] fix grid-gap warnings

* update size limit

* Added section about changing the grid-gap value to the Autoplacement guide

* Adding note about the lack of auto-fit and auto-fill support

I think we need to point this out specifically since people will get excited when they here auto-placement support is coming and think _all_ of auto-placement support is coming.

* Fixing inconsistent indentation in Readme

I'm used to having an indent size of 2 spaces. The rest of the readme uses an indent size of 4 spaces though so I've increased any 2 space indents to 4 spaces.

* [css-grid] fix rows/columns values

* [css-grid] Add warning for grid-template-columns property
@Dan503

This comment has been minimized.

Contributor

Dan503 commented Dec 5, 2018

@ai you can close this issue now that 9.4 has been released.

@ai ai closed this Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment