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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-grid] Fix bug when grid-rows/columns are prefixed with wrong values #1149

Merged
merged 2 commits into from Oct 24, 2018

Conversation

bogdan0083
Copy link
Contributor

@bogdan0083 bogdan0083 commented Oct 24, 2018

We discussed the bug here: #1148

When repeat function were used with multiple values (e.g repeat(1, 1fr 2fr)) it output the wrong values.

Input:

.grid {
  grid-template-columns: 50px repeat(2, 1fr 2fr) 50px;
  grid-template-rows: repeat(1, auto 100px);
  grid-gap: 20px;
  grid-template-areas:
    ". . . . . ."
    ". . . . . .";
}

Before fix:

.grid {
  /* gaps are place wrongly and commas should not be here */
  -ms-grid-columns: 50px 20px 1fr, ,2fr 20px 1fr, ,2fr 20px 50px;
  grid-template-columns: 50px repeat(2, 1fr 2fr) 50px;
  /* gaps are place wrongly and commas should not be here */
  -ms-grid-rows: auto, ,100px;
  grid-template-rows: repeat(1, auto 100px);
  grid-gap: 20px;
  grid-template-areas:
    ". . . . . ."
    ". . . . . .";
}

After fix:

.grid {
  -ms-grid-columns: 50px 20px 1fr 20px 2fr 20px 1fr 20px 2fr 20px 50px;
  grid-template-columns: 50px repeat(2, 1fr 2fr) 50px;
  -ms-grid-rows: auto 20px 100px;
  grid-template-rows: repeat(1, auto 100px);
  grid-gap: 20px;
  grid-template-areas:
    ". . . . . ."
    ". . . . . .";
}

/cc @Dan503

@Dan503
Copy link
Contributor

Dan503 commented Oct 24, 2018

If you remove grid-template-areas it ignores the grid gap (I know, we don't support grid-gap if areas are not defined, well not yet)

It didn't produce any warning though that grid-gap was being ignored. It's probably not worth adding a warning that will soon need to be removed though I guess.

Other than that, I couldn't find any issues.

@ai if you are happy for there to not be a warning, you can review and merge.

The no warning thing is a bit of a separate issue.

@bogdan0083
Copy link
Contributor Author

bogdan0083 commented Oct 24, 2018

If you remove grid-template-areas it ignores the grid gap (I know, we don't support grid-gap if areas are not defined, well not yet)

Yeah. removing grid-template-areas produces incorrect values, because initially grid-template-rows/columns prefixes were added only in grid-template-areas hack.

I'm planning to move the piece of code that adds grid-template-rows/columns prefixes to their own hack, to make these properties independent of grid-template-areas. After that, grid-gap values will be supported.

@ai ai merged commit c0eb6c6 into postcss:master Oct 24, 2018
@bogdan0083 bogdan0083 deleted the fix/grid-rows-columns branch October 24, 2018 11:43
@ai
Copy link
Member

ai commented Oct 24, 2018

Released in 9.3.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.

None yet

3 participants