Skip to content
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

Add static grid gutters #8508 #8528

Merged
merged 5 commits into from Oct 5, 2016

Conversation

5 participants
@ncoden
Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2016

@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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2016

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

@ncoden

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2016

@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

This comment has been minimized.

Copy link
Collaborator

commented Apr 6, 2016

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

@ncoden

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2016

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

@ncoden ncoden force-pushed the ncoden:grid-gutter-size branch from 6228809 to f0dd400 Apr 8, 2016

@ncoden

This comment has been minimized.

Copy link
Member Author

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2016

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

ncoden added some commits Apr 5, 2016

Add static grid gutters #8508
**Add static (unresponsive) grid gutters**
See: #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
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
Fix grid gutters
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 ncoden:grid-gutter-size branch from f0dd400 to cc14c08 Apr 27, 2016

@ncoden

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2016

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

Fix grid gutters (2)
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.

This comment has been minimized.

Copy link
@kball

kball Apr 28, 2016

Collaborator

comment @param name should prob get updated to $gutters

This comment has been minimized.

Copy link
@ncoden

ncoden Apr 28, 2016

Author Member

Thanks

This comment has been minimized.

Copy link
@ncoden

ncoden Apr 28, 2016

Author Member

@kball Fixed

@kball

This comment has been minimized.

Copy link
Collaborator

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 zurb:v6.3 Oct 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ncoden ncoden deleted the ncoden:grid-gutter-size branch Nov 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.