Skip to content

Add static grid gutters #8508 #8528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 5, 2016
Merged

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Apr 5, 2016

Add static (unresponsive) grid gutters
See: #8508
Use: .gutter-{size} class on a row

Example:

<div class="row gutter-small"> <!-- Gutters always have a "small" width -->
  <div class="large-6 columns">...</div>
  <div class="large-6 columns">...</div>
  ...
</div>

Standardize grid gutters functions, mixins and arguments.

Adds/Removes:

  • Add @function grid-column-gutter( {breakpoint} ): Get a gutter size for a given breakpoint.
  • Add @mixin grid-column-size( {px or breakpoint} ): Set the gutters on a column.
  • Depreciate @mixin grid-column-uncollapse. Use grid-column-gutter instead.
  • Use these new features in all (flex-)grid mixin & functions. Use @function grid-column-gutter instead of modifying $grid-column-gutter to support its possible single value.

Other changes:

  • Argument naming: use $gutter for a single size, and $gutters for a size map.
  • Fix $gutter (now $gutters) argument descriptions.

Required for the previous feature.
Anyway, all the "groups of properties" of a component (i.e. size, align, color, or specific behavior like collapse, expand...) should have their own @mixin, to allow a component customization in responsive/semantic cases

@kball
Copy link
Contributor

kball commented Apr 5, 2016

@ncoden I like the direction this is going. You should be aware though that $grid-column-gutter currently supports setting it to a single (scalar) value as well as the responsive approach... I think this would break in that case. Probably in that situation these should just degenerate to the same gutter, yes?

@andycochran
Copy link
Contributor

@kball Ah, right! This'll need to account for if you're not using responsive gutters (e.g. $grid-column-gutter is a single value).

@ncoden
Copy link
Contributor Author

ncoden commented Apr 5, 2016

👍 hum. Actually, if $grid-column-gutter is a single value, it is transformed to a (small: $value) map.
So it would generate a useless .gutter-small class.

@ncoden
Copy link
Contributor Author

ncoden commented Apr 5, 2016

It's dirty because we're assuming there is always a smallest breakpoint called "small".
So I purpose to remove the $grid-column-gutter modification, and create:

@function grid-column-gutter( $breakpoint )

that return the gutter if it is a single value, or the gutter for a given breakpoint if it is a map.
What do you think ?

@andycochran
Copy link
Contributor

You can use nth(map-keys($breakpoints),1) to get the smallest (zero) "breakpoint."

@ncoden
Copy link
Contributor Author

ncoden commented Apr 5, 2016

You can use nth(map-keys($breakpoints),1) to get the smallest (zero) "breakpoint."

Can there be no breakpoint ? Does the "zero breakpoint" always exists and be zero ?

I also see a problem with grid-column-uncollapse:

@mixin grid-column-uncollapse($gutter: $grid-column-gutter) { ... }
  • $grid-column-gutter is always a map, and is used as padding. We must remove this bugged default value`
  • So $grid-column-uncollapse is an alias of $grid-column-gutter
  • And $grid-column-collapse is an alias of $grid-column-gutter(0)
  • So it would be better to only have:
@include grid-column-gutter( {size_in_px} );
@include grid-column-gutter( {breakpoint} ); // "small", "medium"...

And maybe

@include grid-column-gutter( collapse ); // alias for 0

CSS classes keep unchanged

@andycochran
Copy link
Contributor

@ncoden Yeah. A zero breakpoint is required as the first key/value of the $breakpoints map.

But the error is misleading.

This:

@error 'Your smallest breakpoint (defined in $breakpoints) must be set to "0".';

Should probably be:

@error 'The first key in the $breakpoints map must have a value of "0".';

And a note should also be added to the docs page.

@DaSchTour
Copy link
Contributor

BTW, since #8318 is merged you can use $-zf-zero-breakpoint to get the smallest breakpoint.

@ncoden
Copy link
Contributor Author

ncoden commented Apr 6, 2016

@andycochran @DaSchTour @kball Ok, I will add theses changes.

@ncoden ncoden force-pushed the grid-gutter-size branch from 6228809 to f0dd400 Compare April 8, 2016 02:37
@ncoden
Copy link
Contributor Author

ncoden commented Apr 8, 2016

I done these changes, and standardize/factorized all the gutter functions/mixins/uses. For all the changes, see this PR description

I have two questions:

  • Should I update the documentation ? (docs/pages/grid.md)
  • Should I keep @mixin grid-column-uncollapse deleted, or would it be better to depreciate it (redirection to @mixin grid-column-size and throw a warning) ?

@kball
Copy link
Contributor

kball commented Apr 12, 2016

@ncoden I prefer depreciations than outright deletions, so that sounds like a good step. Updating the documentation would be great. @andycochran can you try this out and see if the code changes are good to merge?

@kball
Copy link
Contributor

kball commented Apr 26, 2016

@ncoden looks like this branch has gotten stale and needs some conflicts resolved before it can be merged. @andycochran can you give this trial run?

@ncoden
Copy link
Contributor Author

ncoden commented Apr 26, 2016

Ah sorry, i forgot this PR ;)
I rebase it...

ncoden added 3 commits April 27, 2016 12:23
**Add static (unresponsive) grid gutters**
See: foundation#8508
Use: `. row.gutter-{size}`

Example:

```
<div class="row gutter-small"> <!-- Gutters has always "small" width -->
  <div class="large-6 columns">...</div>
  <div class="large-6 columns">...</div>
  ...
</div>
```

**Add grid-column-gutter mixin**
Set the gutters on a column.
Use: `@include grid-column-gutter($width);`
Standardize grid gutters functions, mixins and arguments.

Adds/Removes:
- Add `grid-column-gutter` function: Get a gutter size for a given
breakpoint.
- Allow `grid-column-size` mixin to take breakpoint name as argument
- Remove `grid-column-uncollapse` mixin. Use `grid-column-size` instead.
- Use these new features in all `(flex-)grid` mixin & functions. Use
`grid-column-gutter` function instead of modify `$grid-column-gutter`
variable

Minor changes:
- Use `$gutter` for a single size, and `$gutters` for a size map
Fixs:
- Depreciate `grid-column-uncollapse` and `grid-col-uncollapse` instead
of delete it
- Fix `.row.{breakpoint}-uncollapse` (wrong mixin call)
@ncoden ncoden force-pushed the grid-gutter-size branch from f0dd400 to cc14c08 Compare April 27, 2016 10:32
@ncoden
Copy link
Contributor Author

ncoden commented Apr 27, 2016

@kball @andycochran It's rebased (and fixed). :)

Fixs:
- Fix `grid-column-gutter` misnamed passed parameter
- Add missing `grid-col-gutter` default parameter
@@ -87,12 +75,12 @@

/// Creates a grid column row. This is the equivalent of adding `.row` and `.column` to the same element.
///
/// @param {Number} $gutter [$grid-column-gutter] - Width of the gutters on either side of the column row.
/// @param {Mixed} $gutter [$grid-column-gutter] - Width of the gutters on either side of the column row. Refer to the `grid-column-gutter()` function to see possible values.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment @param name should prob get updated to $gutters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kball Fixed

@kball
Copy link
Contributor

kball commented Apr 28, 2016

@andycochran @brettsmason @JeremyEnglert as our scss expert yetinauts, does this seem ready to you?

@rafibomb rafibomb added this to the 6.3 milestone Jul 12, 2016
@kball kball merged commit de3b357 into foundation:v6.3 Oct 5, 2016
@ncoden ncoden deleted the grid-gutter-size branch November 17, 2016 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants