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] Implement autoplacement (#1148) #1151

Merged
merged 46 commits into from Dec 3, 2018

Conversation

Projects
None yet
4 participants
@bogdan0083
Contributor

bogdan0083 commented Oct 24, 2018

This PR implements #1148

It can already be tested, but the following things need to be done/considered:

  • Add warning for grid-auto-flow: dense
  • We have a warning that's showed when grid-auto-flow is used. Do we remove it? Do we show it only when grid-template(-areas) is present?
  • Refactoring
  • General checks: compiler must not break, all values should be correct

/cc @Dan503

@ai

This comment has been minimized.

Member

ai commented Oct 24, 2018

Note, that I will have vacation in November, so I will not be able to release it quick (but I think it is not the problem, it is better to think about it longer)

@bogdan0083 bogdan0083 force-pushed the bogdan0083:feature/grid-autoplacement branch from 29f8ed6 to 6a94f69 Oct 25, 2018

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 25, 2018

We have a warning that's showed when grid-auto-flow is used. Do we remove it? Do we show it only when grid-template(-areas) is present?

I think showing it when grid-template(-areas) is present is the correct answer to that question.

We don't support auto-flow if the grid-template-areas algorithm is being used.

@bogdan0083 bogdan0083 force-pushed the bogdan0083:feature/grid-autoplacement branch 2 times, most recently from c906f5d to b88959e Oct 25, 2018

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 25, 2018

@evandiamond Autoplacement feature is ready for testing! 🤠

I would be happy if you could test the feature in your spare time. Here are the things you need to do:

  1. Clone my branch: git clone -b feature/grid-autoplacement https://github.com/bogdan0083/autoprefixer
  2. run yarn inside cloned repository
  3. run npx gulp play, add the code to ./playground/input.css and save the file
  4. the output be in ./playground/output.css

@bogdan0083 bogdan0083 force-pushed the bogdan0083:feature/grid-autoplacement branch from b88959e to 94c09fa Oct 25, 2018

@evandiamond

This comment has been minimized.

evandiamond commented Oct 25, 2018

@bogdan0083 Thank you so much! Going to do some testing later today and I'll report back with feedback. Cheers!

@evandiamond

This comment has been minimized.

evandiamond commented Oct 25, 2018

@bogdan0083 At first glance, this is working nicely. Thank you so much for putting the time into this as I think you're solving a LOT of problems most people are experiencing with CSSGrid.

I am noticing a couple oddities. I'll show with a few examples below.

For all examples I am using a very simple HTML markup for testing:

<div class="Form">
	<div>col 1</div>
	<div>col 2</div>
	<div>col 3</div>
	<div class="button-container">button</div>
</div>

Basic Example

Input:

.Form {
    display: grid;
    grid-template-columns: 1fr 1fr 1fr;
    grid-template-rows: auto;
    grid-gap: 80px;
    grid-row-gap: 0;
}

Output:

.Form {
    display: -ms-grid;
    display: grid;
    -ms-grid-columns: 1fr 80px 1fr 80px 1fr;
    grid-template-columns: 1fr 1fr 1fr;
    -ms-grid-rows: auto;
    grid-template-rows: auto;
    grid-gap: 80px;
    grid-row-gap: 0;
}

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

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

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

Intermediate Example

Input:

.Form {
    display: grid;
    grid-template-columns: 1fr 1fr 1fr;
    grid-template-rows: auto;
    grid-gap: 80px;
    grid-row-gap: 0;
}

.Form .button-container {
    grid-column: 1/4;
    grid-row: 2;
}

Output: ⚠️
I've left comments /* */ below inside the markup.

.Form {
    display: -ms-grid;
    display: grid;
    -ms-grid-columns: 1fr 80px 1fr 80px 1fr;
    grid-template-columns: 1fr 1fr 1fr;
    -ms-grid-rows: auto;
    grid-template-rows: auto;
    grid-gap: 80px;
    grid-row-gap: 0;
}

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

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

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

.Form .button-container {
    -ms-grid-column: 1;
    -ms-grid-column-span: 3;
    /*-ms-grid-column-span: 6; (I think it should be 6, since there are now 5 columns in IE)*/
    grid-column: 1/4;
    -ms-grid-row: 2;
    grid-row: 2;
}


Advanced Example

Input:
Note: I'm changing the order of columns here...

.Form {
    display: grid;
    grid-template-columns: 1fr 1fr 1fr;
    grid-template-rows: auto;
    grid-gap: 80px;
    grid-row-gap: 0;
}

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

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

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

.Form .button-container {
    grid-column: 1/4;
    grid-row: 2;
}

Output:
I've left comments /* */ below inside the markup.
Autoprefixer is properly adding the nth-children first, and then I'm assuming if the user puts overrides in they are being placed after. I guess my question is, should the overrides be spitting out the proper column for IE as well. I think so?

.Form {
    display: -ms-grid;
    display: grid;
    -ms-grid-columns: 1fr 80px 1fr 80px 1fr;
    grid-template-columns: 1fr 1fr 1fr;
    -ms-grid-rows: auto;
    grid-template-rows: auto;
    grid-gap: 80px;
    grid-row-gap: 0;
}

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

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

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

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

.Form > *:nth-child(2) {
    -ms-grid-column: 3;
    grid-column: 3;
    /*-ms-grid-column: 5; (the third column is now technically the 5th column in IE)*/
    -ms-grid-row: 1;
    grid-row: 1;
}

.Form > *:nth-child(3) {
    -ms-grid-column: 2;
    grid-column: 2;
    /*-ms-grid-column: 3; (the second column is now technically the 3rd column in IE)*/
    -ms-grid-row: 1;
    grid-row: 1;
}

.Form .button-container {
    -ms-grid-column: 1;
    -ms-grid-column-span: 3;
    grid-column: 1/4;
    -ms-grid-row: 2;
    grid-row: 2;
    /*-ms-grid-column-span: 6; (I think it should be 6, since there are now 5 columns in IE)*/
}

From an MVP standpoint, the Basic Example should fit the bill for most.

The Intermediate Example, I think needs to be tweaked a hair, if my math is correct. I believe when separating via 1/3, that means the first 2 columns, since the 3 is the column stop. In IE, if we are adding the additional "gaps" as columns, it would be one more than 5.

I think things start to get tricky when moving items around like in the Advanced Example.

@Dan503 Let me know if the above makes sense. These were very simple tests I provided. Hopefully they are helpful!

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 25, 2018

@evandiamond I should have probably mentioned this while we were discussing the issue.

Like with everything else in Autoprefixer's grid translations, Autoprefixer has many limitations in what it is able to achieve. People need to work within those limitations. Autoprefixer can't magically fix everything (just most things 😉).

Autoprefixer can do only the most bare basic of autoplacement functionality. If you try to manually place grid cells in the grid, Autoprefixer will make the grid cells overlap in IE rather than push the other content out of the way.

You can either let content place itself automatically inside the grid OR you can manually place grid cells inside the grid using grid-template-areas.

You cannot manually place grid cells inside the explicit grid of an auto-placement grid!

For example think about this:

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

.grid-two {
    display: grid;
    grid-template-columns: repeat(4, 1fr);
    grid-template-rows: repeat(4, 1fr);
    grid-gap: 30px;
}

/* Is grid-cell placed in grid-one or grid-two? */
.grid-cell {
  grid-column: 2;
  grid-row: 2;
}

In the example above, we have absolutely no way of knowing what grid the user intends for .grid-cell to go in so we are not going to factor it into the equation while building the output code.

However Autoprefixers auto-placement algorithm only works within the explicit grid. Placing things in the implicit grid will work just fine. 😊

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

.grid-cell {
  grid-column: 1 / span 2;
  grid-row: 4; /* <-- This places the grid cell in the implicit grid */
}

@bogdan0083 I'm going to do some testing on this over the weekend. I don't expect I'll find any issues though since the grid doesn't need to take grid cells into consideration like grid-areas do.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 25, 2018

You also can't make any grid cells span multiple columns/rows if they are placed inside the explicit grid for the same reason.

Autoprefixers auto-placement is only to be used in that one use case where every grid-cell only takes up one grid-cell worth of space and they are all placed automatically in the grid. No manual grid-cell placement or spanning grid-cells over multiple columns/rows.

Thanks for getting this implemented @bogdan0083, this feature is awesome! 😁

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 25, 2018

@bogdan0083 I've found a bug.

In the MDN docs for grid-auto-flow, it says that you can provide the dense keyword alongside row or column.

grid-auto-flow: row dense;
grid-auto-flow: column dense;

This CSS does output a warning:

.grid {
  display: grid;
  grid-auto-flow: dense;
  grid-template-columns: repeat(2, 1fr);
  grid-template-rows: repeat(2, 1fr);
}

This CSS does not output a warning:

.grid {
  display: grid;
  grid-auto-flow: column dense; /* <-- no warning */
  grid-template-columns: repeat(2, 1fr);
  grid-template-rows: repeat(2, 1fr);
}

Also, if the user does this, it should output a warning saying that grid-auto-flow needs to be used in the same rule as grid-template-columns and grid-template-rows:

.grid {
  grid-auto-flow: column;
}
@evandiamond

This comment has been minimized.

evandiamond commented Oct 25, 2018

You also can't make any grid cells span multiple columns/rows if they are placed inside the explicit grid for the same reason.

Autoprefixers auto-placement is only to be used in that one use case where every grid-cell only takes up one grid-cell worth of space and they are all placed automatically in the grid. No manual grid-cell placement or spanning grid-cells over multiple columns/rows.

@Dan503 Sure, but in the Intermediate Example with the .button-container being a nested child of .Form, if I am mention in the code grid-column: 1/4; I feel like Autoprefixer should know how many columns there are to give me the correct output of:

.Form .button-container {
    -ms-grid-column: 1;
    -ms-grid-column-span: 3; <--- this should be 6, since there are 5 columns in IE
    grid-column: 1/4;
    -ms-grid-row: 2;
    grid-row: 2;
}

Right? Since IE is counting the columns, and we know how many columns from the parent, it should output the correct span if we are manually putting this in the CSS. So, that's possibly a bug.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Oct 25, 2018

Ahh I see what you're saying.

Yeah that makes sense. We state the parent grid as part of the selector.

...that makes this way more complicated though :/

I think that is out of scope for this initial feature release. It adds loads of complications into the mix.

I think we can do a release with the basic autoplacement functionality first and then create a new issue for tackling grid cells that are placed inside the explicit grid.

@bogdan0083 bogdan0083 force-pushed the bogdan0083:feature/grid-autoplacement branch from 123f2e4 to 9a6b9ad Oct 26, 2018

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Oct 26, 2018

@evandiamond Thank you for testing and suggestions! 😀 Your ideas are good and I'll try to improve grid autoplacement to handle complex cases in the next release 🤔

I also fixed the warning that Dan was mentioning above.

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Nov 7, 2018

Does it pay any attention to the media query? I'm guessing not.

Yeah, this is the thing that should be fixed in the next release. We also need to change grid cells position if grid-gap value is changed from grid-gap: 30px to grid-gap: 0 for example.

@ai

This comment has been minimized.

Member

ai commented Nov 7, 2018

will we be able to release this while you are on vacation

Maybe

Does it pay any attention to the media query?

Let’s focus only on important parts

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 7, 2018

Maybe

I'm trying to figure out what to do with that unpublished grid areas article.

It turns out Chris (CSS Tricks) was just super busy at the time and was taking a while to get back to me. I told him that we will probably be releasing the next version of Autoprefixer this week and it would be best for him to wait for that before upgrading CodePen.

If it isn't going to be released until the end of November though then I'll tell Chris to go ahead and upgrade to the current version of Autoprefixer and proceed with publishing the grid areas article.

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Nov 8, 2018

@Dan503 Here's the possible release message:

Message

Autoprefixer 9.4 brings limited autoplacement support in IE for Grid Layout

Grid Autoplacement

Coat of Arms of Oklahoma

@Dan503, @evandiamond and @bogdan0083 implemented Autoplacement feature

If the grid option is set to "autoplace", limited autoplacement support is added to Autoprefixers grid translations. You can also use the /* autoprefixer grid: autoplace */ control comment to enable autoplacement

/* Input CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
    display: grid;
    grid-template-columns: 1fr 1fr;
    grid-template-rows: auto auto;
    grid-gap: 20px;
}
/* Output CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 1fr 20px 1fr;
  grid-template-columns: 1fr 1fr;
  -ms-grid-rows: auto 20px auto;
  grid-template-rows: auto auto;
  grid-gap: 20px;
}

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

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

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

.autoplacement-example > *:nth-child(4) {
  -ms-grid-row: 3;
  -ms-grid-column: 3;
}

Check out Grid Autoplacement support in IE section for more details.

Markdown code
Autoprefixer 9.4 brings limited **autoplacement** support in IE for Grid Layout

## Grid Autoplacement
Coat of Arms of Oklahoma

@Dan503, @evandiamond and @bogdan0083 implemented Autoplacement feature

If the grid option is set to `"autoplace"`, limited autoplacement support is added to Autoprefixers grid translations. You can also use the `/* autoprefixer grid: autoplace */` control comment to enable autoplacement


```css
/* Input CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
    display: grid;
    grid-template-columns: 1fr 1fr;
    grid-template-rows: auto auto;
    grid-gap: 20px;
}
```

```css
/* Output CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 1fr 20px 1fr;
  grid-template-columns: 1fr 1fr;
  -ms-grid-rows: auto 20px auto;
  grid-template-rows: auto auto;
  grid-gap: 20px;
}

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

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

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

.autoplacement-example > *:nth-child(4) {
  -ms-grid-row: 3;
  -ms-grid-column: 3;
}
```

Check out [Grid Autoplacement support in IE ](https://github.com/postcss/autoprefixer#grid-autoplacement-support-in-ie) section for more details.

I took most of the text from your newly created autoplacement section in readme.

@ai

This comment has been minimized.

Member

ai commented Nov 8, 2018

Good. I especially like that you investigated and copied origin style. Let’s just add your and main testers name to the release notes. “Homeland must know hero names” ☭

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Nov 8, 2018

Alright, done 👍. I kinda feel that the message doesn't really described what autoplacement feature for. I wanna know what Dan thinks.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 8, 2018

I'm at work at the moment. I'll take a closer look tonight. It looks pretty good though. Maybe just some minor grammer fixes needed.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 8, 2018

I've finished reviewing the release notes message. This is what I came up with:

Release notes preview

Coat of Arms of Oklahoma

Autoprefixer 9.4.0 brings limited autoplacement support to the IE CSS Grid translations.

Grid Autoplacement

If the grid option is set to "autoplace", limited autoplacement support is now added to the Autoprefixer CSS Grid translations. You can also use the /* autoprefixer grid: autoplace */ control comment to enable autoplacement directly in your CSS.

In order to use the new autoplacement feature, you must define both rows and columns when declaring the grid template.

/* Input CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
    display: grid;
    grid-template-columns: 1fr 1fr;
    grid-template-rows: auto auto;
    grid-gap: 20px;
}
/* Output CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 1fr 20px 1fr;
  grid-template-columns: 1fr 1fr;
  -ms-grid-rows: auto 20px auto;
  grid-template-rows: auto auto;
  grid-gap: 20px;
}

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

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

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

.autoplacement-example > *:nth-child(4) {
  -ms-grid-row: 3;
  -ms-grid-column: 3;
}

Autoplacement support in Autoprefixer is currently very limited in what it can do. Please read the Grid Autoplacement support in IE section before using this new feature.

Thanks to @bogdan0083 for implementing the new feature, @Dan503 for documenting it, and @evandiamond for coming up with the initial idea.

Markdown

<a target="_blank" rel="noopener noreferrer" href="https://user-images.githubusercontent.com/19343/47355871-8df3cd00-d690-11e8-8ca3-5574d0104ff3.png"><img src="https://user-images.githubusercontent.com/19343/47355871-8df3cd00-d690-11e8-8ca3-5574d0104ff3.png" alt="Coat of Arms of Oklahoma" width="200" height="202" align="right" style="max-width:100%;"></a>

Autoprefixer 9.4.0 brings limited **autoplacement** support to the IE CSS Grid translations.

## Grid Autoplacement

If the `grid` option is set to `"autoplace"`, limited autoplacement support is now added to the Autoprefixer CSS Grid translations. You can also use the `/* autoprefixer grid: autoplace */` control comment to enable autoplacement directly in your CSS.

In order to use the new autoplacement feature, you **must define both rows and columns** when declaring the grid template.

```css
/* Input CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
    display: grid;
    grid-template-columns: 1fr 1fr;
    grid-template-rows: auto auto;
    grid-gap: 20px;
}
```

```css
/* Output CSS */

/* autoprefixer grid: autoplace */

.autoplacement-example {
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 1fr 20px 1fr;
  grid-template-columns: 1fr 1fr;
  -ms-grid-rows: auto 20px auto;
  grid-template-rows: auto auto;
  grid-gap: 20px;
}

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

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

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

.autoplacement-example > *:nth-child(4) {
  -ms-grid-row: 3;
  -ms-grid-column: 3;
}
```

Autoplacement support in Autoprefixer is currently very limited in what it can do. Please read the  [Grid Autoplacement support in IE ](https://github.com/postcss/autoprefixer#grid-autoplacement-support-in-ie) section before using this new feature.

Thanks to @bogdan0083 for implementing the new feature, @Dan503 for documenting it, and @evandiamond for coming up with the initial idea.

 

I placed people in order of their contribution. @bogdan0083 first for building the thing. @Dan503 next for convincing Bogdan to build it and I also wrote the autoplacement guide in the readme. @evandiamond third for coming up with the initial idea and helping us with some testing.

By the way @bogdan0083 I added an extra section to the autoplacement guide about re-declaring columns and rows if editing the size of a grid-gap. Can you merge with my branch again?

@evandiamond

This comment has been minimized.

evandiamond commented Nov 9, 2018

This is great! I appreciate the mention :)

In the next week or so when I have some more time, I'm going to create a layout that we can use to test a bunch of different things down the line. You guys are awesome for putting so much attention and detail into this feature. I really can't thank you enough!

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 9, 2018

I've edited my release entry. previously it said:

@bogdan0083, @Dan503, and @evandiamond implemented the new CSS Grid Autoplacement feature.

Now it says this to make it clearer what each person did:

Thanks to @bogdan0083 for implementing the new feature, @Dan503 for documenting it, and @evandiamond for coming up with the initial idea.

Also @bogdan0083 don't forget to merge with my branch again. It doesn't look like you've done it yet.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 9, 2018

@ai this is all set and ready to go. The release notes have been written and the testing is all looking good.

Can we release this thing now?

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 9, 2018

I talked with @ai.

We decided it would be best that we release the grid areas article while 9.3.1 is still the latest version. It didn't make sense releasing the grid areas article when the exciting new feature is autoplacement.

Andre gets to enjoy a stress free vacation that way as well. He will release 9.4.0 when he gets back at the end of November.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 10, 2018

@bogdan0083 I started writing the auto-placement article and I've found a bug.

Input CSS

.autoplacement-disabled {
  /* autoprefixer grid: no-autoplace */
  display: grid;
  grid-template-columns: 1fr 1fr;
  grid-template-rows: auto auto;
  grid-gap: 20px;
}

Current Output

(It warns the user not to use grid-gap)

.autoplacement-disabled {
  /* autoprefixer grid: no-autoplace */
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 1fr 20px 1fr;
  grid-template-columns: 1fr 1fr;
  -ms-grid-rows: auto 20px auto;
  grid-template-rows: auto auto;
  grid-gap: 20px;
}

Expected output
(It should still warn the user not to use grid-gap)

.autoplacement-disabled {
  /* autoprefixer grid: no-autoplace */
  display: -ms-grid;
  display: grid;
  -ms-grid-columns: 1fr 1fr;
  grid-template-columns: 1fr 1fr;
  -ms-grid-rows: auto auto;
  grid-template-rows: auto auto;
  grid-gap: 20px;
}

Though it's kind of cool applying the grid gap, this would be an accidental breaking change if it got into the final release.

Dan503 added some commits Nov 10, 2018

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.
@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 10, 2018

Also, I've added another bit to the auto-placement guide in my repo. It points out that auto-fit and auto-fill are still not supported. I think users need to be told that explicitly to prevent them getting too excited.

When the auto-placement article I'm writing is published, we will probably scrap the guide in the readme in favour of a link to the article.

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 11, 2018

Found another bug @bogdan0083. This is just a tiny one.

This will produce the warning:

Autoplacement does not work without grid-template-rows property

.autoplacement-enabled {
  display: grid;
  grid-template-columns: 1fr 1fr;
  /* grid-template-rows: auto auto; */
}

However if I do this:

.autoplacement-enabled {
  display: grid;
  /* grid-template-columns: 1fr 1fr; */
  grid-template-rows: auto auto;
}

There is no warning to say that columns need to be defined.

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Nov 11, 2018

Seems like it's fixed now. Can you check it out?

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 11, 2018

Yep, that's better 😊

@Dan503

This comment has been minimized.

Contributor

Dan503 commented Nov 11, 2018

I've made a fix to the "Beware of enabling auto-placement in old projects" section of the readme. The previous recommendation was incorrect.

.grid-cell only has a strength of 1 class. .grid > *:nth-child(1) has a strength of 2 classes. It doesn't matter where .grid-cell is placed, .grid > *:nth-child(1) will always override it.

I now recommend people upgrading just leave auto-placement disabled by default and opt into it using a control comment when they want to use it. It's the safest way to ensure that the site wont break when upgrading.

@bogdan0083 I'll need you to merge my branch again. Are you able to give me write access to your branch/repo? I can push directly to this pull request then instead of requesting merges when I make changes.

@bogdan0083

This comment has been minimized.

Contributor

bogdan0083 commented Nov 17, 2018

@Dan503 I'm sorry, I've been busy these days. I've sent you an invite to join as collaborator for my fork.

@ai ai merged commit 2e82382 into postcss:master Dec 3, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@ai

This comment has been minimized.

Member

ai commented Dec 3, 2018

Thanks to everyone for amazing work. 9.4 was finally released.

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