Auto-detect table layout #43

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@treppo
treppo commented Dec 9, 2012

Implement feature discussed in #33

@kaishin
kaishin commented Dec 27, 2012

@treppo Thanks for submitting this PR.

After having played with it for a while, I found out that if we go this route we will break compatibility, and, more importantly, force users to always use outer-container() or row() after using outer-container(table) or row(table) in order to reset $container-display-table to false.

For instance, using your fork with the examples page causes the layout to break (even after adding default values for $container-display-table and $display-table) because the value remains true after the table layout example and carries on to all the remaining ones given the call order of the mixins (outer-container is called only once for all the sections, before row(table) is called).

As it stands now, I think we should either come up with a mixin that sets $container-display-table to false (something like @include table-end;) or keep things the way they are now.

Additionally, I think unless we want to output display: table on outer-container, we'd better stick to implementing this only in row().

@kaishin
kaishin commented Dec 28, 2012

So, I tried a couple of approaches to solve the problem mentioned above.
The first is to create a reset-display mixin:

@mixin reset-display {
  $container-display-table: false;
}

This mixin should be called in the last cell of the table to reset the display to block when the user is not calling outer-container() or row() in the code that comes after the table container.

  div.last-cell {
    @include span-columns(7);
    @include reset-display;
  }

The second approach consists in using a last-cell argument in the $display property of the span-columns() mixin.

  div.last-cell {
    @include span-columns(7, last-cell);
  }

And on the implementation side of things:

@if $display-table  {
    display: table-cell;
    padding-right: flex-gutter($container-columns);
    width: flex-grid($columns, $container-columns) + flex-gutter($container-columns);

    &:last-child {
      padding-right: 0;
    }

    @if $display == last-cell {
      $container-display-table: false;
    }
  }

@treppo Thoughts?

@treppo
treppo commented Jan 5, 2013

@kaishin I took a look at the examples page, added the default values and I see the problem now.

What puzzles me is why the $container-display-table variable is not reset by the use of @include outer-container in the following <section> in the first place.

I like your approach but I got the same effect by just resetting the $container-display-table inside the &:last-child directive.

@if $display-table  {
  display: table-cell;
  padding-right: flex-gutter($container-columns);
  width: flex-grid($columns, $container-columns) + flex-gutter($container-columns);

  &:last-child {
    padding-right: 0;
    $container-display-table: false;
  }
}

Do you think that this is a simpler solution to the problem—and demands less effort from the user—or do we then run into other problems?

@kaishin
kaishin commented Jan 7, 2013

@treppo That wouldn't work. Consider this example:

section {
  @include row(table); // sets $container-display-table to true

  div {
    @include span-columns(8); // :last-child sets $container-display-table to false
  }
  aside {
    @include span-columns(2); // Will use block-type display instead
  }
}
@treppo
treppo commented Jan 7, 2013

Of course, why didn't I think of that!?
I can't come up with anything better than your reset-display mixin.
There I like the first approach better

div.last-cell {
  @include span-columns(7);
  @include reset-display;
}

as it is more explicit and more similar to

div.last-cell {
  @include span-columns(7);
  @include omega;
}
@dukex
Contributor
dukex commented Jan 8, 2013

I like that approach using reset-display mixin!

@kaishin
kaishin commented Jan 22, 2013

@treppo What's the status on this?

@treppo
treppo commented Jan 22, 2013

I should be able to come up with some results this weekend

@kaishin
kaishin commented Jan 22, 2013

Cool! Thanks.

@treppo treppo referenced this pull request Jan 27, 2013
Closed

Auto-detect table layout #56

@treppo
treppo commented Jan 27, 2013

There it is. I also updated the gh-pages branch accordingly: #56

@kaishin
kaishin commented Feb 1, 2013

After giving this some thought, I think we'd better limit the auto-detection to row() given that outer-container() doesn't / can't output display: table. Afaik, display: table-cell elements won't display properly unless they are contained inside a display: table element, which would make adding a row div necessary anyway.

@treppo
treppo commented Feb 2, 2013

You are right, makes sense. I will drop the auto–detection in outer-container().

@kaishin kaishin pushed a commit that closed this pull request Feb 13, 2013
Christian Treppo + Reda Lemeden Auto-detect table layout
* Closes #43
c5290ef
@kaishin kaishin closed this in c5290ef Feb 13, 2013
@dukex
Contributor
dukex commented Feb 15, 2013

Nice @treppo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment