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

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

Merged
merged 6 commits into from Oct 10, 2016

Conversation

Projects
None yet
4 participants
@ncoden
Member

ncoden commented Apr 28, 2016

Resolve #8259

Changes
Make grid-column-gutter (and also grid-col-gutter) define
responsive gutters when no gutter value ($gutter) is given, instead
of defining gutters from the zero breakpoint.

If only a $gutters map is given, use this map to generate responsive
gutters.

If only a $gutters single value is given, use this value to generate
no-responsive gutters.


⚠️ Require

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)
Fix grid gutters (2)
Fixs:
- Fix `grid-column-gutter` misnamed passed parameter
- Add missing `grid-col-gutter` default parameter

ncoden added some commits Apr 28, 2016

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

Make `grid-column-gutter` (and also `grid-col-gutter`) define
responsive gutters when no gutter value (`$gutter`) is given, instead
of defining gutters from the zero breakpoint.

If only a `$gutters` map is given, use this map to generate responsive
gutters.

If only a `$gutters` single value is given, use this value to generate
no-responsive gutters.

@ncoden ncoden force-pushed the ncoden:grid-gutter-auto-size branch from 49d2516 to 19bae9b Apr 28, 2016

@rafibomb rafibomb added this to the 6.3 milestone Jul 12, 2016

} @else {
// breakpoint name
@if type-of($gutter) == 'string' {
$gutter: grid-column-gutter($gutter, $gutters);

This comment has been minimized.

@kball

kball Oct 5, 2016

Collaborator

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

This comment has been minimized.

@ncoden

ncoden Oct 5, 2016

Member

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

This comment has been minimized.

@kball

kball Oct 5, 2016

Collaborator

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...

This comment has been minimized.

@ncoden

ncoden Oct 5, 2016

Member

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

This comment has been minimized.

@ncoden

ncoden Oct 5, 2016

Member

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. 🤔

This comment has been minimized.

@ncoden

ncoden Oct 5, 2016

Member

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 !

This comment has been minimized.

@kball

kball Oct 5, 2016

Collaborator

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

This comment has been minimized.

@ncoden

ncoden Oct 7, 2016

Member

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.

@brettsmason

This comment has been minimized.

Collaborator

brettsmason commented Oct 6, 2016

I'm all for a documented naming convention and sticking to it. I personally have no experience in using BEM, and I feel it might be a bit too strict on this occasion, but I understand the reasoning behind it.

I prefer SMACSS myself, and its more flexible in its approach.
A simply example would be buttons, currently we have:

.button
.primary
.secondary
.large

You could go with something like:

.button
.button-primary
.button-secondary
.button-large

I'm happy to go with whatever the general consensus is though. Not sure how to apply this to functions/mixins...

@kball

This comment has been minimized.

Collaborator

kball commented Oct 10, 2016

Ok, so I propose we move forward with this as is, and move the naming conversation into a separate location.

@kball kball merged commit a8e8074 into zurb:v6.3 Oct 10, 2016

1 check passed

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

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

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