FremyCompany / css-grid-polyfill

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

Feature Request:add column and row gap #26

Closed delebash closed 5 years ago

delebash commented 9 years ago

Add column and row gap, these properties where just added to the editors draft to enable easy gutters.

FremyCompany commented 9 years ago

Seems like a relatively easy yet reasonable change, but I would be interested in pairing with someone on this one to avoid having to continue updating this polyfill alone forever :-)

Would you be interested in working with me on the fix, if I provide you with enough details on how it should be done?

delebash commented 9 years ago

If you can point me in the right direction. I am prob only average on my javascript skills, but I am willing to have a go at it :)

FremyCompany commented 9 years ago

STEP 1: Recognizing and parsing the properties The starting point is adding two new properties, row-gap and column-gap. You can do this here by adding their names to the list of tracked properties:

https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/polyfill.js#L20

Then, you have to take those in consideration during the layout. I guess the best place to add the row-gap/column-gap layout exclusion is here (maybe something like this.rowGap and this.colGap whose initial value would be 0):

https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/lib/grid-layout.js#L522

STEP 2: Computing the used value of the properties To fill these fields, we can use the updateFromElement method:

https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/lib/grid-layout.js#L611

To convert into pixels, we can call cssUnits.convertToPixels as in https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/lib/grid-layout.js#L681 (the source is over there if you need help with the options: https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/core/css-units.js#L73; if you want to take a shortcut and not read the code, the first parameter should be the property value, the second the grid element, and the options should be { isHeightRelated: true } for "rowGap" and { isWidthRelated: true } for "colGap").

STEP 3: Adapt the layout algorithm From there, you have to take the rowGap/colGap values in consideration during the track breath computation. You don't have to mess with the sizing algorithm in this case, hopefully, as you can simply adjust the available width by decreasing it by ((the amount of rows - 1) * the row gap) saved previously. You could do this here:

https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/lib/grid-layout.js#L1736

Finally, you want to modify how items are positioned here by adding the rowGap/columnGap each time we cross a column. This should happen there:

https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/lib/grid-layout.js#L2408 https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/lib/grid-layout.js#L2435

To make the algorithm yields the correct result, it is also necessary to (if you don't see why or how, it's okay, I can do this part):

How the rowGap/colGap affects the layout

Effect of rowGap/colGap on min- and max-width

SUMMARY OF ACTIONS: That's not too hard, but it's some not-null piece of work. If you start working on this and decide to stop at some point, I can take it from there. I can also answer all your questions, it's great for me to do so, because it can serve as a form of documentation. The more questions people ask, the more I have to take time documenting stuff, which is great!

If you manage to do everything mentioned previously, then I'll only write test cases, maybe fix some weird edge cases I didn't think about right now, and ultimately be a happy guy :-)

FremyCompany commented 9 years ago

Oh, I'll also take on me the code generating "-ms-grid" values because it will face important changes once we support gaps. This code is currently not used but I would like to maintain it because it is convenient when I'm testing stuff on IE.

Just because it's great to document things, this code is there: https://github.com/FremyCompany/css-grid-polyfill/blob/master/src/css-grid/lib/grid-layout.js#L2365

The fix I've in mind right now is to multiply all x/y values by two, and append the row-gap/column-gap between the size of the rows/columns (ySizes/xSizes). That's not too hard but I've to test this carefully, there may be unexpected issues I don't think about now.

delebash commented 9 years ago

Thanks for the write up, I will start looking at the code.

FremyCompany commented 9 years ago

Are you by chance still interested to work on this, or are you already busy with other stuff elsewhere?

delebash commented 9 years ago

Sorry just been busy with main projects right now, still interested but it may be awhile before I get a chance to work on this.

FremyCompany commented 9 years ago

Okay, no worry. I don't need the feature right now, but I encourage other people needing it to chime in so I know whether to work on this or just let other people some time to be able to work on it. Feedback is more important for me than features right now.

lozandier commented 9 years ago

@FremyCompany It'd be a splendid edition to this polyfill. :+1:

FremyCompany commented 9 years ago

@lozandier I'm pretty sure it is the case, but I would like the code to be contributed by the community. I've laid out the plan to write the implementation, and believe someone else may be able to do it and learn in the process. I'm committed to fix bugs in the polyfill and provide support to anyone trying to use or modify it, but adding new features is also partly the responsibility of the users (and I'm not one myself at this time).

FremyCompany commented 9 years ago

However, if enough people comment on the issue, I may revisit this decision, but I still think it's best for the future of the project to get someone else write the code ;-)

RichiCoder1 commented 9 years ago

I might be interested in assisting. Been curious about both web standards and the mechanics of this polyfill. Also really want to use CSS Grid ^_^

FremyCompany commented 9 years ago

Great :-)

The plan is layed out here: https://github.com/FremyCompany/css-grid-polyfill/issues/26#issuecomment-118553399

I think it is sufficiently explicit, but I'm here to provide any help :-) I just updated it to include a bit more info, and also to keep line numbers in sync with the recent changes. If I didn't forget anyting, it shouldn't be necessary to edit any line further than 5 lines of code to any location I gave the link to.

Also, I think this is a perfect way to start learning about the polyfill, because it covers many importants steps without actually going into the details for the css-grid spec, which can somewhat hard to grasp from time to time.

RichiCoder1 commented 9 years ago

I'm relatively stacked on items, but I'll see about getting an at least [WIP] PR in place next week :). Great explanation and steps!

RichiCoder1 commented 9 years ago

Spec Link for Reference: https://drafts.csswg.org/css-grid/#gutters

Buslowicz commented 8 years ago

I want this, I want this, I really want this :). Do you have any estimates on when could that be finished?

FremyCompany commented 8 years ago

Basically how long as it will take for someone else to step in and implement the plan outlined before. I decided I won't work on new features myself anymore in the interest of my personal time ;-)

Buslowicz commented 8 years ago

Ok this is not cool :(. I might try doing something with it, but not sooner than October (if not later). Damn I hate math :/.

aronduby commented 8 years ago

@FremyCompany I'm attempting to tackle this, but realistically if I can't get it done within the next day or two I probably won't be able to. Unfortunately it looks like one of the npm tools has been unpublished grunt-contrib-concat-sourcemaps. I have never used grunt before so I'd have no clue what I'm doing to try and replace that. Any chance you could fix so I can pull that down for working on this?

FremyCompany commented 8 years ago

Well, that's annoying. Let's have a look at that...

FremyCompany commented 8 years ago

Fixed in latest commit. I should fork the repo to be sure it won't be unpublished from github next, but okay in the mean time this does the trick.

codypearce commented 7 years ago

+1

npointercbt commented 7 years ago

+1

raphaelgoetter commented 7 years ago

+1 too :)

shinobi5 commented 7 years ago

My guess is there's no one currently contributing to this fix? I'm not sure how much free time I have but I can have a go at this in my spare time if no one else is working on it... Would be a useful addition to the polyfill.

FremyCompany commented 7 years ago

I am not aware of any active effort toward this, and do not have time to commit myself (though I'd not mind answering questions or helping fix some issues at late-stage developement phase of a fix)

shinobi5 commented 7 years ago

Ok. I'll take a look at it and ask questions when they arise.

katerlouis commented 5 years ago

How's it looking? We need the gaaaap! +1

FremyCompany commented 5 years ago

This issue can be closed, gaps should be working now per #51. If you encounter issues with this, please feel free to open an issue with an example though.