-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[grid-template] fix adding grid span for IE (fixes #1084) #1086
Conversation
I don't really like the idea of always outputting grid-column/row-span values, but I can also see that trying to pick and choose when to output the span value is going to be a nightmare. If we go down the always-output-a-span-value route, then I think there is an issue in your pull request. Your span output seems to be inconsistent. Current output: .head {
-ms-grid-row: 1;
-ms-grid-column: 1;
-ms-grid-column-span: 1;
}
.nav {
-ms-grid-row: 2;
-ms-grid-row-span: 1;
-ms-grid-column: 1;
} expected output: .head {
-ms-grid-row: 1;
-ms-grid-row-span: 1; /* added row span */
-ms-grid-column: 1;
-ms-grid-column-span: 1;
}
.nav {
-ms-grid-row: 2;
-ms-grid-row-span: 1;
-ms-grid-column: 1;
-ms-grid-column-span: 1; /* added column span */
} Basically when grid-areas are being used, Autoprefixer should always output:
Or is there some smart discrimination going on in your code that is purposely leaving the extra span values out? |
Actually there is :). The logic is simple: If we find grid-template(-area) inside media rule, search the rule above with the same selector and compare -ms-grid-row/column-span values. If values differ then we can add property. Before I started working on this issue there was a simple condition that didn't add -ms-grid-row/column-span if one of the values was 1 (e.g
This is very easy to implement actually. The question is: should we add -ms-grid-row/column-span for EVERY rule with grid-template(-areas) or only for the rules inside media rule? (right now it doesn't add property if -ms-grid-row/column-span is 1) The idea with always outputting -ms-span values is bulletproof and straightforward but adds unnecessary properties sometimes (which is not a big problem in my opinion) |
Nah I prefer keeping the Autoprefixer output as slim as possible. I made that suggestion thinking you might have missed something. If you have logic to only add the span values when needed then that is a much better solution :) |
Alright! 😄 So we can now go for a code review I guess :) /cc @ai |
You should look into the Travis CLI issues. @ai isn't going to accept the pull request with Travis CLI errors in it. |
Yeap, you need improve tests, so all code should be executed durting test. Just run |
@ai Alright, sorry. Now all checks are fine I guess. |
@bogdan0083 thanks! great work |
Sorry, I will take few days to fix this issue #1081 and release they all together |
Released in 9.1 |
This PR fixes #1084.
@Dan503 Can you please review grid-template.out.css in you spare time? I think now it should add -ms-grid-(row|column)-span just fine 👍