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] move media query cell placements to come after cell styles #1048

Closed
Dan503 opened this Issue May 19, 2018 · 19 comments

Comments

3 participants
@Dan503
Copy link
Contributor

Dan503 commented May 19, 2018

Currently if I write this:

/* How I would like to structure my scss file */

.grid {
  display: grid;
  grid-template-columns: repeat(2, 1fr);
  grid-template-areas:
    "a   b"
    "c   d";

  // Placing media query close to the other .grid code
  @media (min-width: 600px){
    grid-template-areas:
      "a   b   c   d";
    grid-template-columns: repeat(4, 1fr);
  }

  &__cell {
    &--a {
      grid-area: a;
    }
    &--d {
      grid-area: d;
    }
  }
}

auto-prefixer outputs this:

/* How Autoprefixer outputs that code */

.grid {
 display: -ms-grid;
 display: grid;
     -ms-grid-columns: (1fr)[2];
     grid-template-columns: repeat(2, 1fr);
     grid-template-areas: "a   b"
"c   d";
}

@media (min-width: 600px) {
 .grid {
       grid-template-areas: "a   b   c   d";
   -ms-grid-columns: (1fr)[4];
   grid-template-columns: repeat(4, 1fr);
 }
 .grid__cell--a {
   -ms-grid-row: 1;
   -ms-grid-column: 1;
 }
 .grid__cell--d {
   -ms-grid-row: 1;
   -ms-grid-column: 4;
 }
}

.grid__cell--a {
 -ms-grid-row: 1;
 -ms-grid-column: 1;
 grid-area: a;
}
.grid__cell--d {
 -ms-grid-row: 2;
 -ms-grid-column: 2;
 grid-area: d;
}

The issue is that the media query code is being overwritten by the default styles so the media query ends up having no effect in IE.

You can write the input code like this instead which fixes the issue:

/* How I have to structure my code to not cause the issue */

.grid {
  display: grid;
  grid-template-columns: repeat(2, 1fr);
  grid-template-areas:
    "a   b"
    "c   d";

  &__cell {
    &--a {
      grid-area: a;
    }
    &--d {
      grid-area: d;
    }
  }

  // Placing media query AFTER the cell styles
  @media (min-width: 600px){
    grid-template-areas:
      "a   b   c   d";
    grid-template-columns: repeat(4, 1fr);
  }
}

However it means you have to write half of your .grid styles at the top of the scss file and write the other half of the styles at the bottom. It is much nicer having them all centralised in one location.

I'd prefer to have the autoprefixer output for the first example to be this:

/* How I would like Autoprefixer to output the code from the 1st example */

.grid {
 display: -ms-grid;
 display: grid;
     -ms-grid-columns: (1fr)[2];
     grid-template-columns: repeat(2, 1fr);
     grid-template-areas: "a   b"
"c   d";
}

@media (min-width: 600px) {
 .grid {
       grid-template-areas: "a   b   c   d";
   -ms-grid-columns: (1fr)[4];
   grid-template-columns: repeat(4, 1fr);
 }
}

.grid__cell--a {
 -ms-grid-row: 1;
 -ms-grid-column: 1;
 grid-area: a;
}
.grid__cell--d {
 -ms-grid-row: 2;
 -ms-grid-column: 2;
 grid-area: d;
}

@media (min-width: 600px) {
 .grid__cell--a {
   -ms-grid-row: 1;
   -ms-grid-column: 1;
 }
 .grid__cell--d {
   -ms-grid-row: 1;
   -ms-grid-column: 4;
 }
}
@ai

This comment has been minimized.

Copy link
Member

ai commented May 19, 2018

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented May 19, 2018

@Dan503 We can fix it by adding a media query to each selector with grid-area. Then you will need to use https://github.com/hail2u/node-css-mqpacker to merge media queries.

@Dan503

This comment has been minimized.

Copy link
Contributor

Dan503 commented May 19, 2018

I've used media query merging software before, they do far more harm than good :(

Is it not possible to save them into an array then output them at the end into one media query block?

I don't really like the idea of each one getting wrapped in a separate media query :(

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented May 19, 2018

It's possible too. To my mind, we have 3 solutions for this case:

  1. Duplicate query for each area
  2. Duplicate query after the last area
  3. Append everything to parent query, then move this query after last area (or to the end of all CSS)
@Dan503

This comment has been minimized.

Copy link
Contributor

Dan503 commented May 19, 2018

Duplicate query for each area

Creates way too much extra junk in the css file.

Duplicate query after the last area

That is the option I think works best. I think that this is what I requested in the issue isn't it?

Append everything to parent query, then move this query after last area (or to the end of all CSS)

Too much risk of specificity breaking due to drastic changes in the source order.

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented May 19, 2018

I think, if we want insert media after the last area, the third solution would be the best.

@Dan503

This comment has been minimized.

Copy link
Contributor

Dan503 commented May 19, 2018

.... maybe.

There is a bit of risk in that the media query is going to be holding more than just grid styles in it.

Moving site styles away from where the author wrote them is a bit dangerous.

Moving an extra media query that was generated through autoprefixer that only holds IE prefixes is a lot less risky in my mind.

@Dan503

This comment has been minimized.

Copy link
Contributor

Dan503 commented May 19, 2018

Yeah it's not as neat and tidy but shifting an authors styles around scares me a bit.

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented May 19, 2018

Agreed with you. It's not right to change authors styles.
Let's try to implement the second solution.

@ai ai added the support label May 22, 2018

@yepninja yepninja added this to Inbox in Autoprefixer May 23, 2018

@Dan503

This comment has been minimized.

Copy link
Contributor

Dan503 commented May 26, 2018

@yepninja this is a high priority issue that should get fixed before the article is published.

@yepninja yepninja moved this from Inbox to Need decision in Autoprefixer Jun 1, 2018

@ai

This comment has been minimized.

Copy link
Member

ai commented Jun 3, 2018

@yepninja do you need help with it from Cult of Martians?

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented Jun 4, 2018

@ai Let's try

@ai

This comment has been minimized.

Copy link
Member

ai commented Jun 4, 2018

@yepninja I will create a task tomorrow. Can you write on Russian current plan?

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented Jun 4, 2018

Yep, I'll write plan today

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented Jun 6, 2018

  1. Ознакомиться с обсуждением задачи
  2. Форкнуть Autoprefixer
  3. Поправить или добавить тесты в файлах:
  4. Поменять логику переопределения позиций областей в функции insertAreas
@ai

This comment has been minimized.

Copy link
Member

ai commented Jun 6, 2018

@yepninja а что именно сделать надо?

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented Jun 6, 2018

Нужно поменять логику переопределения областей для media. Сейчас новые позиции областей добавляются к медиа-выражению, где было переопредлено grid-template-areas, соответсвенно если это медиа-выражение указаны вышо каких-то областей, то переопределние не стработает. Поэтому нужно дублировать медиа-выражение с переопределениями, и вставлять его всего после последней области.

@ai

This comment has been minimized.

Copy link
Member

ai commented Jun 6, 2018

@yepninja

This comment has been minimized.

Copy link
Collaborator

yepninja commented Jun 7, 2018

Released in 8.6.1

@yepninja yepninja closed this Jun 7, 2018

Autoprefixer automation moved this from Waiting for release to Done Jun 7, 2018

adgad added a commit to Financial-Times/n-ui that referenced this issue Jun 7, 2018

Pin autoprefixer version
postcss/autoprefixer#1048 broke the article build with
[1] TypeError: Cannot read property 'type' of undefined
[1]     at /home/ubuntu/next-article/node_modules/@financial-times/n-ui/node_modules/autoprefixer/lib/hacks/grid-utils.js:348:25

adgad added a commit to Financial-Times/n-ui that referenced this issue Jun 7, 2018

Pin autoprefixer version (#1256)
postcss/autoprefixer#1048 broke the article build with
[1] TypeError: Cannot read property 'type' of undefined
[1]     at /home/ubuntu/next-article/node_modules/@financial-times/n-ui/node_modules/autoprefixer/lib/hacks/grid-utils.js:348:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment