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.

Sign up for GitHub

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

Jump to bottom

Make grid-column-gutter define responsive gutters by default #8259 #8693

Merged
merged 6 commits into from Oct 10, 2016
Merged

Make grid-column-gutter define responsive gutters by default #8259 #8693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4dd5d99
Add static grid gutters #8508
ncoden Apr 5, 2016
948ddd5
Standardize grid gutters
ncoden Apr 8, 2016
cc14c08
Fix grid gutters
ncoden Apr 27, 2016
085cabb
Fix grid gutters (2)
ncoden Apr 28, 2016
de3b357
Fix grid-column-row parameter in doc
ncoden Apr 28, 2016
19bae9b
Make grid-column-gutter define responsive gutters by default #8259
ncoden Apr 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 11 additions & 3 deletions scss/grid/_classes.scss
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
$row: 'row',
$column: 'column',
$column-row: 'column-row',
$gutter: 'gutter',
$push: 'push',
$pull: 'pull',
$center: 'centered',
Expand Down Expand Up @@ -52,6 +53,15 @@
margin-right: auto;
}
}

// Static (unresponsive) row gutters
@each $breakpoint, $value in $grid-column-gutter {
&.#{$gutter}-#{$breakpoint} {
> .#{$column} {
@include grid-col-gutter($value);
}
}
}
}

// Column
Expand Down Expand Up @@ -124,9 +134,7 @@
}

.#{$-zf-size}-#{$uncollapse} {
$gutter: -zf-get-bp-val($grid-column-gutter, $-zf-size);

> .#{$column} { @include grid-col-uncollapse($gutter); }
> .#{$column} { @include grid-col-gutter($-zf-size); }
}

// Positioning
Expand Down
34 changes: 10 additions & 24 deletions scss/grid/_column.scss
View file
Open in desktop
Expand Up @@ -52,30 +52,16 @@
/// Creates a grid column.
///
/// @param {Mixed} $columns [$grid-column-count] - Width of the column. Refer to the `grid-column()` function to see possible values.
/// @param {Number} $gutter [$grid-column-gutter] - Spacing between columns.
/// @param {Mixed} $gutters [$grid-column-gutter] - Spacing between columns. Refer to the `grid-column-gutter()` function to see possible values.
@mixin grid-column(
$columns: $grid-column-count,
$gutter: $grid-column-gutter
$gutters: $grid-column-gutter
) {
@include grid-column-size($columns);
float: $global-left;

// Gutters
@if type-of($gutter) == 'map' {
@each $breakpoint, $value in $gutter {
$padding: rem-calc($value) / 2;

@include breakpoint($breakpoint) {
padding-left: $padding;
padding-right: $padding;
}
}
}
@else if type-of($gutter) == 'number' and strip-unit($gutter) > 0 {
$padding: rem-calc($gutter) / 2;
padding-left: $padding;
padding-right: $padding;
}
@include grid-column-gutter($gutters: $gutters);

// Last column alignment
@if $grid-column-align-edge {
Expand All @@ -87,12 +73,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} $gutters [$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.
@mixin grid-column-row(
$gutter: $grid-column-gutter
$gutters: $grid-column-gutter
) {
@include grid-row;
@include grid-column($gutter: $gutter);
@include grid-column($gutters: $gutters);

&,
&:last-child {
Expand All @@ -112,15 +98,15 @@
/// @alias grid-column
@mixin grid-col(
$columns: $grid-column-count,
$gutter: $grid-column-gutter
$gutters: $grid-column-gutter
) {
@include grid-column($columns, $gutter);
@include grid-column($columns, $gutters);
}

/// Shorthand for `grid-column-row()`.
/// @alias grid-column-row
@mixin grid-col-row(
$gutter: $grid-column-gutter
$gutters: $grid-column-gutter
) {
@include grid-column-row($gutter);
@include grid-column-row($gutters);
}
30 changes: 7 additions & 23 deletions scss/grid/_flex-grid.scss
View file
Open in desktop
Expand Up @@ -13,18 +13,18 @@
/// @param {Number} $width [$grid-row-width] - Maximum width of the row.
/// @param {Number} $columns [null] - Number of columns to use for this row. If set to `null` (the default), the global column count will be used.
/// @param {Boolean} $base [true] - Set to `false` to prevent basic styles from being output. Useful if you're calling this mixin on the same element twice, as it prevents duplicate CSS output.
/// @param {Number} $gutter [$grid-column-gutter] - Gutter to use when inverting margins, in case the row is nested.
/// @param {Number|Map} $gutters [$grid-column-gutter] - Gutter map or single value to use when inverting margins, in case the row is nested. Responsive gutter settings by default.
@mixin flex-grid-row(
$behavior: null,
$width: $grid-row-width,
$columns: null,
$base: true,
$gutter: $grid-column-gutter
$gutters: $grid-column-gutter
) {
$margin: auto;

@if index($behavior, nest) != null {
@include grid-row-nest($gutter);
@include grid-row-nest($gutters);

@if index($behavior, collapse) != null {
margin-left: 0;
Expand Down Expand Up @@ -71,30 +71,16 @@
/// Creates a column for a flex grid. By default, the column will stretch to the full width of its container, but this can be overridden with sizing classes, or by using the `unstack` class on the parent flex row.
///
/// @param {Mixed} $columns [null] - Width of the column. Refer to the `flex-grid-column()` function to see possible values.
/// @param {Number} $gutter [$grid-column-gutter] - Space between columns, added as a left and right padding.
/// @param {Mixed} $gutters [$grid-column-gutter] - Spacing between columns. Refer to the `grid-column-gutter()` function to see possible values.
@mixin flex-grid-column(
$columns: null,
$gutter: $grid-column-gutter
$gutters: $grid-column-gutter
) {
// Base properties
@include flex-grid-size($columns);

// Gutters
@if type-of($gutter) == 'map' {
@each $breakpoint, $value in $gutter {
$padding: rem-calc($value) / 2;

@include breakpoint($breakpoint) {
padding-left: $padding;
padding-right: $padding;
}
}
}
@else if type-of($gutter) == 'number' and strip-unit($gutter) > 0 {
$padding: rem-calc($gutter) / 2;
padding-left: $padding;
padding-right: $padding;
}
@include grid-column-gutter($gutters: $gutters);

// fixes recent Chrome version not limiting child width
// https://stackoverflow.com/questions/34934586/white-space-nowrap-and-flexbox-did-not-work-in-chrome
Expand Down Expand Up @@ -258,9 +244,7 @@
}

.#{$-zf-size}-uncollapse {
$gutter: -zf-get-bp-val($grid-column-gutter, $-zf-size);

> .column { @include grid-col-uncollapse($gutter); }
> .column { @include grid-col-gutter($-zf-size); }
}
}

Expand Down
7 changes: 0 additions & 7 deletions scss/grid/_grid.scss
View file
Open in desktop
Expand Up @@ -42,13 +42,6 @@ $grid-column-responsive-gutter: null !default;
$grid-column-gutter: $grid-column-responsive-gutter;
}

// If a single value is passed as a gutter, convert it to a map so the code knows what to do with it
@if type-of($grid-column-gutter) == 'number' {
$grid-column-gutter: (
small: $grid-column-gutter,
);
}

@import 'row';
@import 'column';
@import 'size';
Expand Down
68 changes: 63 additions & 5 deletions scss/grid/_gutter.scss
View file
Open in desktop
Expand Up @@ -6,19 +6,76 @@
/// @group grid
////

/// Get a gutter size for a given breakpoint
/// @param {Keyword} $breakpoint [small] - Breakpoint name.
/// @param {Number|Map} $gutters [$grid-column-gutter] - Gutter map or single value to use. Responsive gutter settings by default.
///
/// @returns {Number} Gutter size.
@function grid-column-gutter(
$breakpoint: $-zf-zero-breakpoint,
$gutters: $grid-column-gutter
) {
// If gutter is single value, return it
@if type-of($gutters) == 'number' {
@return $gutters;
}

// Else, return the corresponding responsive value
@return -zf-get-bp-val($gutters, $breakpoint);
}

/// Set the gutters on a column
/// @param {Number|Keyword} $gutter [auto]
/// Spacing between columns, accepts multiple values:
/// - A single value will make the gutter that exact size.
/// - A breakpoint name will make the gutter the corresponding size in the $gutters map.
/// - "auto" will make the gutter responsive, using the $gutters map values.
/// @param {Number|Map} $gutters [$grid-column-gutter] - Gutter map or single value to use. Responsive gutter settings by default.
@mixin grid-column-gutter(
$gutter: auto,
$gutters: $grid-column-gutter
) {
@if $gutter == auto and type-of($gutters) == 'map' {
// "auto"
@each $value, $breakpoint in $gutters {
@include breakpoint($breakpoint) {
@include grid-column-gutter($value);
}
}
} @else {
// breakpoint name
@if type-of($gutter) == 'string' {
$gutter: grid-column-gutter($gutter, $gutters);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... it took me a while to realize this was actually calling a function by the same name, rather than infinitely recursing through the mixin... hmm. I see looking at our codebase that this isn't that uncommon of a pattern, but it struck me as unintuitive at first glance. Is this a common scss practice? (cc @brettsmason @JeremyEnglert @andycochran @ncoden @zurbrandon @rafibomb). Alternatively, my inclination would be to have some sort of consistent scheme for naming functions that are connected to particular mixins, for example by prefixing with a '-' or something similar.

A quick look around at some existing scss styleguides doesn't seem to highlight function naming... interested in your thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the namespace-function-role-bar-baz naming convention, we're limited, we can't explicitly differentiate the namespace from the function role.

So we can make a little, easy and temporary change and rename it to :

@function grid-column-gutter-size

But we can also expand our naming convention to namespace_function-role-... :

@function grid_column_gutter-size

At term, I hope we will switch all the components/functions/mixins names to a better naming convention, full-BEM: namespace__element--modifier

@function grid__column--gutter-size

Copy link
Contributor

Choose a reason for hiding this comment

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

So there's 2 aspects to this... there's having a consistent naming scheme in general, and there's the question of how to handle interconnected functions and mixins.

With regards to the first, I've heard arguments for and against BEM - generally it feels unnecessarily verbose to me, though I'd be open to arguments why that verbosity is helpful - but I think in most cases our naming conventions have at least been reasonably consistent when it comes to component structure and final class name.

On the second though, I'd personally like some sort of immediate visual cue as to what role the thing I'm looking at is... for example, in ruby modules and class names are CamelCase, constants are CAPITALIZED, and variables and method names are snake_case. In scss we have an immediate visual cue of something being a $variable, but no naming distinction between a @mixin (which must be @include'd in) and a @function which is called directly...

I'd like to propose we adopt some sort of convention that gives that immediate visual cue at least within foundation. I don't have a strong opinion on what it is...

Copy link
Contributor Author

@ncoden ncoden Oct 5, 2016 •
edited

Choose a reason for hiding this comment

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

A quick look around at some existing scss styleguides doesn't seem to highlight function naming... interested in your thoughts

I didn't found scss naming convention for functions, variables and mixins when I was working on CaliOpen, so I used an adapted BEM :

Here an exemple : https://gist.github.com/ncoden/171b26636f052c41739aff3c26ab8357

Copy link
Contributor Author

@ncoden ncoden Oct 5, 2016 •
edited

Choose a reason for hiding this comment

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

I'd like to propose we adopt some sort of convention that gives that immediate visual cue at least within foundation. I don't have a strong opinion on what it is...

CSS is case-insensitive, so Sass repeated the bug feature. But sometimes I see code which break the always-snipal-case for a CamelCase alternative version of BEM, because case errors never happen.

Also, everything is declared in the global scope, so we have to almost always deal with a namespace. But maybe we can write something like NameSpace_CONST ?

To distribute the different cases to the differentes Sass things, we only have to deal between @mixin and @function.

  • NameSpace, @mixin (and the .class involved) is more a OO Class, so I would say CamelCase
  • @function (with $variable, unless for const) takes the rest, snipal-case or snake_case.

I made the following exemple : https://gist.github.com/ncoden/a1cef157c7ebe236fe115cd6d2bd4a26
I don't like it, but I don't find better for now. 🤔

Copy link
Contributor Author

@ncoden ncoden Oct 5, 2016 •
edited

Choose a reason for hiding this comment

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

With regards to the first, I've heard arguments for and against BEM - generally it feels unnecessarily verbose to me, though I'd be open to arguments why that verbosity is helpful.

Note: When I say BEM, I talk about the global "Block Element Modifier" separation, not the official (and strict) getbem convention

BEM was not only created to help the developer knowing what he uses, but to prevent unwanted behaviours when using CSS classes : properties overwritten by specificity, naming collisions across components or frameworks... I remember having these problems with Foundation

Also, there is a lot of BEM grammars, some more verbose than others (i.e. block-foo__element-bar--modifier-baz, BlockFoo_ElementBar-ModifierBaz, etc...). Generally, each developer using BEM has its own variante !

You can also take a look at SMACSS (great explanation here). It's more complete than BEM, full of great advices !

Copy link
Contributor

Choose a reason for hiding this comment

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

Very interesting. Curious what our more SCSS-focused yetinauts think @andycochran @brettsmason @JeremyEnglert @zurbrandon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I think that every element declared in the global namespace should be prefixed. Foundation variables, function and mixins should begin with zf-. We should do it also for CSS classes, but it could be a bit disturbing for the static Foundation users. We can purpose an optional prefix option (disabled by default), but it would create multiple standards.

}

// single value
$padding: rem-calc($gutter) / 2;

padding-left: $padding;
padding-right: $padding;
}
}

/// Collapse the gutters on a column by removing the padding. **Note:** only use this mixin within a breakpoint. To collapse a column's gutters on all screen sizes, use the `$gutter` parameter of the `grid-column()` mixin instead.
@mixin grid-column-collapse {
padding-left: 0;
padding-right: 0;
@include grid-column-gutter(0);
}

/// Un-collapse the gutters on a column by re-adding the padding.
///
/// @param {Number} $gutter [$grid-column-gutter] - Spacing between columns.
@mixin grid-column-uncollapse($gutter: $grid-column-gutter) {
$gutter: rem-calc($gutter) / 2;
padding-left: $gutter;
padding-right: $gutter;
@warn 'This mixin is being replaced by grid-column-gutter(). grid-column-uncollapse() will be removed in Foundation 6.4.';
@include grid-column-gutter($gutters: $gutter);
}

/// Shorthand for `grid-column-gutter()`.
/// @alias grid-column-gutter
@mixin grid-col-gutter(
$gutter: auto,
$gutters: $grid-column-gutter
) {
@include grid-column-gutter($gutter, $gutters);
}

/// Shorthand for `grid-column-collapse()`.
Expand All @@ -30,5 +87,6 @@
/// Shorthand for `grid-column-uncollapse()`.
/// @alias grid-column-uncollapse
@mixin grid-col-uncollapse($gutter: $grid-column-gutter) {
@warn 'This mixin is being replaced by grid-col-gutter(). grid-col-uncollapse() will be removed in Foundation 6.4.';
@include grid-column-uncollapse($gutter);
}
23 changes: 9 additions & 14 deletions scss/grid/_row.scss
View file
Open in desktop
Expand Up @@ -40,18 +40,18 @@
/// Modifications to the default grid styles. `nest` indicates the row will be placed inside another row. `collapse` indicates that the columns inside this row will not have padding. `nest collapse` combines both behaviors.
/// @param {Number} $width [$grid-row-width] - Maximum width of the row.
/// @param {Boolean} $cf [true] - Whether or not to include a clearfix.
/// @param {Number} $gutter [$grid-column-gutter] - Gutter to use when inverting margins, in case the row is nested.
/// @param {Number|Map} $gutters [$grid-column-gutter] - Gutter map or single value to use when inverting margins. Responsive gutter settings by default.
@mixin grid-row(
$columns: null,
$behavior: null,
$width: $grid-row-width,
$cf: true,
$gutter: $grid-column-gutter
$gutters: $grid-column-gutter
) {
$margin: auto;

@if index($behavior, nest) != null {
@include grid-row-nest($gutter);
@include grid-row-nest($gutters);

@if index($behavior, collapse) != null {
margin-left: 0;
Expand All @@ -77,19 +77,14 @@

/// Inverts the margins of a row to nest it inside of a column.
///
/// @param {Map|null} $gutter [null] - Gutter value to use when inverting the margins. Set to `null` to refer to the responsive gutter settings.
@mixin grid-row-nest($gutter: $grid-column-gutter) {
@if type-of($gutter) == 'number' {
$gutter: ($-zf-zero-breakpoint: $gutter);
}
/// @param {Number|Map} $gutters [$grid-column-gutter] - Gutter map or single value to use when inverting margins. Responsive gutter settings by default.
@mixin grid-row-nest($gutters: $grid-column-gutter) {
max-width: none;

@each $breakpoint, $value in $gutter {
$margin: rem-calc($value) / 2 * -1;
@include -zf-each-breakpoint {
$margin: rem-calc(grid-column-gutter($-zf-size, $gutters)) / 2 * -1;

@include breakpoint($breakpoint) {
margin-left: $margin;
margin-right: $margin;
}
margin-left: $margin;
margin-right: $margin;
}
}