Fix gutterless, closes #134

Financial-Times / o-grid

Responsive grid system

http://registry.origami.ft.com/components/o-grid

93 stars 14 forks source link

Fix gutterless, closes #134 #135

Closed triblondon closed 8 years ago

triblondon commented 8 years ago

kaelig commented 8 years ago

This unfortunately breaks sub grids.

My recommendation would be to have a modifier on containers, removing left and right padding.

Something like this:

    .o-grid-container--bleed {
        padding-left: 0;
        padding-right: 0;
    }

triblondon commented 8 years ago

@kaelig can you elaborate a bit on how it breaks subgrids?

OK, so the bleed idea would leave us with a solution where:

There is a default padding on container...
removed by the modifier...
then the row has negative margin which pushes it outside the container...
and the column has padding that brings it back to 0 again

I'd be concerned about possible overflow scrollbars coming in, unless there was overflow hidden on the container. But if that's the only solution that works for all of us and it doesn't cause any side effects, fair enough.

kaelig commented 8 years ago

The body should have overflow-x: hidden on it. It's pretty common to do it on responsive sites to make sure no third party content gets out of the desired viewport and adds horizontal scrollable areas.

Subgrids are rows inside of cells, there are a few on the test page, but not for compact rows (yet). With the technique from this PR, I found that subgrids got out of their container. If that makes no sense I can throw a bit of code your way.

triblondon commented 8 years ago

OK, updated to use new approach. Also updated the demo to remove margin highlighting because it is misleading now that it goes outside the container and is in some cases negative.

triblondon commented 8 years ago

Still needs documenting, if Origami team are OK with this.

kaelig commented 8 years ago

Ideally there would be a new test for it too! I believe @wheresrhys knows well how they work.

wheresrhys commented 8 years ago

I'm happy to help with explaining the tests, but no time to write one I'm afraid

triblondon commented 8 years ago

OK, the test framework was pretty broken, actually. For example:

a centered column was failing a margin test because the expected margin was NaN. That was because we were trying to multiply a number by a gutter which was coming back as '20px'. I added a parseInt to that.
It passed, but only because the almostEqual function is allowing a 100% variance in the value, so e.g. if expected is '300', an acceptable value was between 0 and 600; I reduced the variance allowed to 1% anywhere that almostEqual is used.
Now that test fails again, but it seems the test is wrongly including the gutter in the expect, since the outer gutter is padding on the container, not on the row. So I removed the gutter from the calculation of the expected value.
Now it passes again, this time I think correctly.

Also added documentation for bleed.

kaelig commented 8 years ago

Good stuff, thanks for doing this, it's good to see the grid evolve with sensible use cases!