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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l 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

Merged
merged 3 commits into from Aug 2, 2018

Conversation

bogdan0083
Copy link
Contributor

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 馃憤

@Dan503
Copy link
Contributor

Dan503 commented Aug 1, 2018

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:

  • -ms-grid-row
  • -ms-grid-row-span
  • -ms-grid-column
  • -ms-grid-column-span

Or is there some smart discrimination going on in your code that is purposely leaving the extra span values out?

@bogdan0083
Copy link
Contributor Author

bogdan0083 commented Aug 1, 2018

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 -ms-grid-row-span: 1)

Basically when grid-areas are being used, Autoprefixer should always output:
-ms-grid-row
-ms-grid-row-span
-ms-grid-column
-ms-grid-column-span

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)

@Dan503
Copy link
Contributor

Dan503 commented Aug 1, 2018

This is very easy to implement actually.

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 :)

@bogdan0083
Copy link
Contributor Author

bogdan0083 commented Aug 1, 2018

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

@Dan503
Copy link
Contributor

Dan503 commented Aug 1, 2018

You should look into the Travis CLI issues. @ai isn't going to accept the pull request with Travis CLI errors in it.

@ai
Copy link
Member

ai commented Aug 1, 2018

Yeap, you need improve tests, so all code should be executed durting test.

Just run npx jest --coverage and then check HTML page in coverage/

@bogdan0083
Copy link
Contributor Author

@ai Alright, sorry. Now all checks are fine I guess.

@ai
Copy link
Member

ai commented Aug 2, 2018

@bogdan0083 thanks! great work

@ai ai merged commit ef59cd3 into postcss:master Aug 2, 2018
@ai
Copy link
Member

ai commented Aug 2, 2018

Sorry, I will take few days to fix this issue #1081 and release they all together

@ai
Copy link
Member

ai commented Aug 3, 2018

Released in 9.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grid Column / Row Span for IE11
3 participants