FremyCompany / css-grid-polyfill

A working implementation of css grids for current browsers.
MIT License
1.11k stars 82 forks source link

Add support for grid-gap, grid-row-gap and grid-column-gap #51

Closed RamIdeas closed 6 years ago

RamIdeas commented 7 years ago

This PR adds support for the gap properties mentioned 🎉

This only took under an hour to implement and a bit longer to (manually) test a little thanks to your comment

If you could offer similar help for any other issues mentioned in #50 I would be glad to have another go!

FremyCompany commented 7 years ago

Awesome! Let me take a look

FremyCompany commented 7 years ago

By the way, if you have some demo pages you were trying your gap implementation on, feel free to add it/them to the demo/css-grid folder!

magicznyleszek commented 7 years ago

@FremyCompany @RamIdeas any ETA on this, any problems to be solved?

FremyCompany commented 7 years ago

See the PR comments, I think some of them are still pending

FremyCompany commented 7 years ago

https://github.com/FremyCompany/css-grid-polyfill/pull/51#discussion_r139486370

RamIdeas commented 6 years ago

First off, apologies for letting this linger for so long. I had trouble finding spare time to pick this up again.

I think I've successfully accounted for *-gap percentage values now by introducing fullDistributableSize -- is what I've done what you had in mind?

I also noticed a slight issue in regards to grid-column-gap being specified when the grid container does not have an explicit height (what the spec calls the "logical height" having a value of auto). The first version of the spec mentions the expected behaviour under "length-percentage" here. I've forked Rachel Andrews' pen with an explanation here.

FremyCompany commented 6 years ago

Thanks! There is still one more thing to check about the negative sizes, and then I think we can merge this.

Percentage resolution with auto height is probably still broken, but making this work doesn't seem very critical to me to be able to merge this. If someone ends up using this and finds the issue, it can be corrected then.

RamIdeas commented 6 years ago

Hey have you had a chance to review the changes? This should be ready to merge 😄

FremyCompany commented 6 years ago

I'm looking at this sometime this weekend :)

FremyCompany commented 6 years ago

Thanks! 💚