Financial-Times / o-grid

Responsive grid system
http://registry.origami.ft.com/components/o-grid
94 stars 17 forks source link

Gutterless doesn't appear to work #134

Closed triblondon closed 8 years ago

triblondon commented 8 years ago

http://jsbin.com/limifi/edit?html,output#

The first mmmbop should be wider than the second. Currently, it seems that gutters work like this:

  1. Container has padding on both sides
  2. Row has negative left margin that nullifies the container padding
  3. Cells have positive left padding

For compact, the left-margin on the row and left-padding on the cell are removed, which leaves just the padding on the container, and effectively makes no difference to a row containing a single cell (it does remove the gutters between cells)

Would it be simpler to remove the container padding, so that we have:

  1. Row has padding on the right
  2. Cell has padding on the left

And for compact, both of those are removed.

I can't see any benefit to padding the container.

wheresrhys commented 8 years ago

A question for @kaelig as to why it's structured like this. Probably a good reason.

kaelig commented 8 years ago

Compact rows's main use case is for subgrids, but you can remove the side gutters by removing the container div altogether.

The reason why negative margins were used was to make the code as simple as possible (thanks to @i-like-robots for suggesting it), and to allow for good separation of concerns. Style overrides for each layout / media query are kept to a minimum.

If needed, I can walk you through the code in detail to explain the reasoning.

triblondon commented 8 years ago

Subgrids is a good point. But I don't understand how negative margins make it 'simple'. Comparing the two solutions:

Current Proposed fix
Padding or margin Both Padding only
Element types with whitespace 3 2
Negative lengths? Yes No
Overrides for gutterless Some All

The solution involving negative margins has a combination of margins and padding, applies them to three separate element types, and overrides 2 out of three of them for gutterless. A solution that has only padding (no margin), no negative lengths, applies padding only to two element types, and overrides both of them for gutterless, well that seems simpler to me!

Any objections to that change? If not I'll do a PR. Any concerns about this being a breaking change?

i-like-robots commented 8 years ago

I believe that would break things. The negative margin was introduced so that we did not need to wrap everything in a grid row and column I guess the simplicity is that pages require less markup.

triblondon commented 8 years ago

So you want to use grid columns without rows or containers? Maybe I'm not understanding the use case - can you post some markup?

i-like-robots commented 8 years ago

http://codepen.io/anon/pen/XdvoGB

The issue with the padded columns being that the width of the wrapping container is not equal to the content width of the columns.

i-like-robots commented 8 years ago

So you want to use grid columns without rows or containers?

I think we wanted to use containers without rows or columns.

triblondon commented 8 years ago

The issue with the padded columns being that the width of the wrapping container is not equal to the content width of the columns.

o-grid currently contains left-padding on columns, so this solution doesn't change that. I would have thought the situation was the exact opposite - padding counts towards content width, but margin doesn't, so if you avoid using margin and don't pad your container, the container should be the exact width of the sum of the column widths plus the row padding...

I think we wanted to use containers without rows or columns.

That sounds like just a padded div. Do you really need the grid to do that?

Do you have a proposal to fix the bug that doesn't remove the padding on the container? I need to fix this bug for Nikkei.

wheresrhys commented 8 years ago

container sets max-width according to o-grid's breakpoints

On 24 May 2016 at 11:36, Andrew Betts notifications@github.com wrote:

The issue with the padded columns being that the width of the wrapping container is not equal to the content width of the columns.

o-grid currently contains left-padding on columns, so this solution doesn't change that. I would have thought the situation was the exact opposite - padding counts towards content width, but margin doesn't, so if you avoid using margin and don't pad your container, the container should be the exact width of the sum of the column widths plus the row padding...

I think we wanted to use containers without rows or columns.

That sounds like just a padded div. Do you really need the grid to do that?

Do you have a proposal to fix the bug that doesn't remove the padding on the container? I need to fix this bug for Nikkei.

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/Financial-Times/o-grid/issues/134#issuecomment-221230824


This email was sent by a company owned by Financial Times Group Limited ("FT Group http://aboutus.ft.com/corporate-information/#axzz3rajCSIAt"), registered office at Number One Southwark Bridge, London SE1 9HL.
Registered in England and Wales with company number 879531. This e-mail may contain confidential information. If you are not the intended recipient, please notify the sender immediately, delete all copies and do not distribute it further. It could also contain personal views which are not necessarily those of the FT Group. We may monitor outgoing or incoming emails as permitted by law.

i-like-robots commented 8 years ago

The .o-grid-container element is used widely without child rows or columns to centre content and set the width of the content area according to the breakpoints defined by o-grid. Prior to V4 the container and row were not decoupled so many extra nested elements were needed to convey the most basic layouts and grouping content together became tricky, E.G.:

<!-- V3 -->
<div class="o-grid-row">
  <div data-o-grid-colspan="12"><h1>Title</h1></div>
</div>
<div class="o-grid-row">
  <div data-o-grid-colspan="12 M6">Content</div>
  <div data-o-grid-colspan="12 M6">Content</div>
</div>
<div class="o-grid-row">
  <div data-o-grid-colspan="12">Content</div>
</div>

<!-- V4 -->
<div class="o-grid-container">
  <h1>Title</h1>
  <div class="o-grid-row">
    <div data-o-grid-colspan="12 M6">Content</div>
    <div data-o-grid-colspan="12 M6">Content</div>
  </div>
  Content
</div>

<!-- V3 -->
<div class="o-grid-row">
  <div data-o-grid-colspan="12">
    <div class="arbitrary-background-padded-block">
      <div class="o-grid-row">
        <div data-o-grid-colspan="12"><h1>Title</h1></div>
      </div>
      <div class="o-grid-row">
        <div data-o-grid-colspan="12 M6">Content</div>
        <div data-o-grid-colspan="12 M6">Content</div>
      </div>
    </div>
  </div>
</div>

<!-- V4 -->
<div class="o-grid-container">
  <div class="arbitrary-background-padded-block">
    <h1>Title</h1>
    <div class="o-grid-row">
      <div data-o-grid-colspan="12 M6">Content</div>
      <div data-o-grid-colspan="12 M6">Content</div>
    </div>
  </div>
</div>
i-like-robots commented 8 years ago

The example are constructed from memory so may not be perfect... I guess the point is that the container and row elements are now separate and may be used exclusively.

If you're not any closer to solving your layout conundrum I'd be very happy to help =]

triblondon commented 8 years ago

OK, I understand the use case for the container alone (though I'm pretty sure we used to support something like <div class='o-grid-row' data-o-grid-colspan='12'> to do a row and a column in one element, no?)

My layout conundrum is simply that I want a completely gutterless grid, and the component is supposed to support that, so I think it's just a bug we need to fix. Any suggestions for fixing it without changing behaviour that you rely on are very much appreciated!

I suppose we could add a new compact class on container, which removes the container padding and changes the behaviour to what I'm proposing. It would have no visual effect unless combined with compact on a row.

wheresrhys commented 8 years ago

<div class='o-grid-row' data-o-grid-colspan='12'> hasn't worked since v1 (or possibly v2)

It would have no visual effect unless combined with compact on a row.

That feels weird. I would expect .o-grid-conrtainer--compact to have a visual effect on all its contents, especially given that .o-grid-row--compact does. Can this not be fixed by just making .o-grid-row--compact do more?

triblondon commented 8 years ago

OK, I'll give that a go.

kaelig commented 8 years ago

Can you simply not use a container at all? Or does it break? (I haven't tested)

triblondon commented 8 years ago

Container is required for the max-width.

OK, I think I've fixed it in https://github.com/Financial-Times/o-grid/pull/135, but I would love this architecture to be tidied up - it doesn't really make sense to me for there to be padding on the container. Next's use case could be accommodated by creating a Next-specific class that mixes in both container and column behaviour into the same element, couldn't it?