Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[css-grid] move media query cell placements to come after cell styles #1048

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

[css-grid] move media query cell placements to come after cell styles #1048

Dan503 opened this issue May 19, 2018 · 19 comments
Labels
support

Comments

@Dan503
Copy link
Contributor

Dan503 commented May 19, 2018 •
edited
Loading

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
Copy link
Member

ai commented May 19, 2018

cc @yepninja

@yepninja
Copy link
Collaborator

@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
Copy link
Contributor Author

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
Copy link
Collaborator

yepninja commented May 19, 2018 •
edited
Loading

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
Copy link
Contributor Author

Dan503 commented May 19, 2018 •
edited
Loading

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
Copy link
Collaborator

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

@Dan503
Copy link
Contributor Author

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
Copy link
Contributor Author

Dan503 commented May 19, 2018

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

@yepninja
Copy link
Collaborator

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
@Dan503
Copy link
Contributor Author

Dan503 commented May 26, 2018

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

@ai
Copy link
Member

ai commented Jun 3, 2018

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

@yepninja
Copy link
Collaborator

yepninja commented Jun 4, 2018

@ai Let's try

@ai
Copy link
Member

ai commented Jun 4, 2018

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

@yepninja
Copy link
Collaborator

yepninja commented Jun 4, 2018

Yep, I'll write plan today

@yepninja
Copy link
Collaborator

yepninja commented Jun 6, 2018

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

@ai
Copy link
Member

ai commented Jun 6, 2018

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

@yepninja
Copy link
Collaborator

yepninja commented Jun 6, 2018

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

@ai
Copy link
Member

ai commented Jun 6, 2018 •
edited
Loading

https://cultofmartians.com/tasks/autoprefixer-grid-media.html

@YozhikM YozhikM mentioned this issue Jun 6, 2018
Closed
@An-Tu An-Tu mentioned this issue Jun 7, 2018
Closed
@yepninja yepninja mentioned this issue Jun 7, 2018
Merged
@yepninja
Copy link
Collaborator

yepninja commented Jun 7, 2018

Released in 8.6.1

@yepninja yepninja closed this as completed Jun 7, 2018
adgad added a commit to Financial-Times/n-ui that referenced this issue Jun 7, 2018
@adgad
Pin autoprefixer version
6b73a57 
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 adgad mentioned this issue Jun 7, 2018
Merged
adgad added a commit to Financial-Times/n-ui that referenced this issue Jun 7, 2018
@adgad
Pin autoprefixer version (#1256)
d13a689 
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
@Dan503 Dan503 mentioned this issue Jul 31, 2018
Closed
@Dan503 Dan503 mentioned this issue Aug 12, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
support
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ai @Dan503 @yepninja