Add grid-container #408

wardpenney · 2016-02-05T17:55:33Z

Builds on @whmii's Cask grid and previous PR #406. The goal is to do set up a few features:

Provide default grid configuration.
Enable multiple grids with different gutters and max-widths.
Enable collapsed grids (via gutter: 0 grid type)

I still need to write tests and possibly factor the column width calculation into two discrete @functions.

LMKWYT!

houndci-bot · 2016-02-05T17:56:03Z

core/neat/mixins/_neat-column.scss

+
+    width: calc(#{$_column-width-calc});
+  }
+  @else {


@else should be placed on same line as previous curly brace

joshuaogle · 2016-02-05T18:39:17Z

Has there been more discussion around namespacing? I liked the $grid- prefixes better than $neat- because I think it would be easier to remember and be nicer for users, but I understand wanting to avoid any possibility of conflicts.

tysongach · 2016-02-05T19:54:37Z

Nice! This feels like a really great start…

I really enjoy the fact that instead of creating a special syntax, or term, you can create a gutter-less grid with gutter: 0. It’s damn clear.

tysongach · 2016-02-05T19:57:01Z

core/neat/settings/_settings.scss

+///
+/// @type map
+
+$neat-grid-default: () !default;


The name of this feels too similar to the one above, $neat-default-grid, which I think could easily lead to confusion or accidental bugs.

Maybe this one becomes $neat-base-grid? Or something else?

Oh whops, I think I missed a copy-paste. I took that from Bourbon. Perhaps I don't need that +$neat-grid-default: () !default;? Now that I look at it, I don't think it's useful here.

I think I see what you mean now, in relation to the later comment. So I'm going to remove it, and trying to convey to the user that they should define this ahead of time, if they don't want these default values

tysongach · 2016-02-05T20:00:39Z

We’ll need to have a merging function of sorts, so to allow the user to define their overrides in a map, and those get merged in. Currently, I don’t think this works…

wardpenney · 2016-02-06T00:50:45Z

@tysongach Can you explain the need for the merging functionality? I was trying to avoid the need to merge maps by not using the same variable.

This is done in a way that asks the developer to provide a grid context to the method call, as opposed to overriding an assumed global map variable. The methods use the base grid as fallback only. So it's either "use the default" or "provide your custom grid."

Wat do u think?

wardpenney · 2016-02-06T00:55:55Z

@joshuaogle regarding naming prefix, I am down for both! What do y'all think?

We have had some issues, and anecdotal evidence that overly-generic variable names have conflicted with other frameworks and custom variables. I have not seen anyone using grid-, and definately not anyone using neat-. Either way (or other way) I do vote that we just be consistent and everything have the same prefix, 'cause:

Conflict mitigation
Explicitness to user

tysongach · 2016-02-06T21:11:00Z

@wardpenney @joshuaogle Sass 4 is expected to introduce a native module system, and part of that system will (likely) allow us to associate prefixes dynamically, as well as allowing users to change/swap the prefix dynamically to their pleasing. It sounds incredible and you can read more about that here: https://github.com/sass/proposal.module-system#using-modules

whmii · 2016-02-11T09:29:35Z

So happy to see this. Pumped for some new functionality. I agree with @joshuaogle that personally I prefer grid prefixing to neat especially since we have everything so well scoped. I could see having neat-settings be a thing but then still using things like grid-column. Not that we always should take cues from others, but in poking around, I was unable to find another system that used a taxonimy like my-thing-column but the majority were using grid-column.

But it's not a huge issue for me. ¯_(ツ)_/¯

I would like to take a deeper look at this PR over all. lets hold off merging till at least Friday.
ty

joshuaogle · 2016-02-12T17:41:26Z

I've done my best to completely tear this apart and find any use-case that I've come across that has made Neat frustrating to see how these changes hold up. I love it. Not only is the code so so much simpler than the current mixins, but the way you've set it up to handle multiple grids is really clever. I love the idea of setting up multiple grids (in grid-settings.scss, let's say) rather than overriding variables and some of the messier solutions I've seen out there.

Code looks great and this is nice solid foundation in my opinion 👍 Good work @wardpenney and @whmii!

@tysongach I hadn't seen the module system in Sass4, and it looks like the perfect solution to the naming problem (and I think a good reason to use grid- prefixes for now)

whmii · 2016-02-12T19:34:14Z

@wardpenney Hey I have a weird request. Would you mind rebasing your neat pr off of https://github.com/thoughtbot/neat/tree/neat-2.0.0-origin

It is just the addition of the grid columns mixin. I put a lot of work in to that system, and wanted to have my signature on it. Not because of the code itself but I feel like that file represents the idea as a whole. I thought it wouldn't bother me, but annoyingly it does. This is very petty. I apologize. 🙇

Also, based on some of the comments, I swapped out the neat prefix for grid on the mixins.

whmii · 2016-02-15T19:39:46Z

core/neat/mixins/_grid-container.scss

+  &::after {
+    clear: both;
+    content: "";
+    display: table;


This can be display block, Display table is no longer needed → thoughtbot/bourbon@7633f70

+1…don’t forget to also update Line 19 above, for the docs showing output.

whmii · 2016-02-15T19:42:09Z

BTW, I like the inclusion of grid-container, instead of grid row. I think grid-row is a leftover from when people were wrapping each row of content individually, which is very rare now a days.

whmii · 2016-02-15T23:29:01Z

core/neat/mixins/_grid-container.scss

@@ -0,0 +1,35 @@
+/// Creates Neat grid container with clearfix.
+///
+/// @argument {list} $grid


I'm not 100% on sass doc but I think this line should be:

/// @argument {map} $grid [$neat-default-grid]

tysongach · 2016-02-19T02:21:10Z

@whmii @wardpenney I’m confused why we rebased off of another branch and are now merging changes from that branch into neat-2.0.0 via this PR? I think we lost the focus of this PR by doing that.

If we go back to the original commit this PR was opened with, get it reviewed and merged, we can then put up a new PR with other additions (a2c5842). This way it gets a solid review, discussion and clear path in. Will you’re commit authorship will remain the same.

tysongach · 2016-02-19T02:25:25Z

core/neat/settings/_settings.scss

+///
+/// @type map
+///
+/// @property {number (unitless)} columns 12


The property’s default value should be in brackets:

- /// @property {number (unitless)} columns 12 + /// @property {number (unitless)} columns [12]

tysongach · 2016-03-01T00:26:50Z

I’m going to rebase and remove Will’s commit, which is the same commit in his new PR: #415

tysongach · 2016-03-01T01:30:41Z

I removed 65980d3 and this PR is now solely Ward’s addition of grid-container.

tysongach · 2016-03-01T01:53:00Z

core/neat/mixins/_grid-container.scss

+
+@mixin grid-container($grid: $neat-default-grid) {
+  margin-left: auto;
+  margin-right: auto;


Should “centering” be an option? Do we need it at all? Maybe this mixin is literally just a wrapper to a clearfix mixin, so to have a nice grid API?

Yes, we had chatted a bit about that earlier and defaulted to this. But maybe the best thing to do is not try to do anything, and let the developer do it.

Other option is to handle it in the $grid map definition, center, left, or right but that seems like a lot of logic for little benefit. Also, on more than one project we wanted it left aligned at large breakpoints, and centered at lower ones. The $grid setting route would prove painful for that.

This looks good to me. let's remove the option and just have this be a simple mixin with no options (since we aren't using $grid anyways).

So this grid-container will literally just be a wrapper for a clearfix?

Yep 💩. I would imagine anyone using a floated grid would want this, though.

I agree that the margin styles may do more harm than good. I'm okay with it just being a clearfix wrapper. It's really all we need.

@wardpenney Yeah. To clarify, I think it's totally fine for this mixin to just be a clearfix. It makes for a good API for the grid system and prevents us from having to say “Oh, don't forget when you use grid columns, that you need to use a clearfix!”.

👍

whmii · 2016-03-01T16:48:02Z

#415 has been merged

tysongach · 2016-03-01T18:05:06Z

contrib/index.html

+        <div class="box column--thirds">
+        </div>
+        <div class="box column--thirds">
+        </div>
      </main>
    </body>
  </html>


This whole file is oddly indented?

The pre tag will add indentation as whitespace in the code block that you can see. It looks weird here, but you have to.

@joshuaogle That pre tag is deleted. I’m referencing the fact that the entire HTML document is indented with two spaces: https://github.com/thoughtbot/neat/blob/neat-2.0.0-basic-grid/contrib/index.html

Ah okay. Your comment was showing up on the deleted side so I thought you were just curious. Yep, good catch on that spacing.

Ahh weird, good catch.

tysongach · 2016-03-01T18:27:26Z

contrib/index.html

+    <link rel="stylesheet" href="styles.css">
+  </head>
+  <body>
+    <header class="welcome-message" role="banner">


Can we remove all these unused classes?

✂️ welcome-message

houndci-bot · 2016-03-01T18:42:20Z

contrib/patterns/_grid.scss

+  @include grid-column(1, $nested-grid);
+}
+
+.grid__column--full {


Selector grid__column--full should be written in lowercase with hyphens

tysongach · 2016-03-01T18:45:52Z

contrib/base/_grids.scss

@@ -0,0 +1,4 @@
+$nested-grid: (


Any reason to not put this in _variables.scss, since $nested-grid is just a variable?

houndci-bot · 2016-03-01T18:50:27Z

contrib/patterns/_grid.scss

+  @include grid-column(4);
+}
+
+.grid__column--full {


Selector grid__column--full should be written in lowercase with hyphens

tysongach · 2016-03-01T18:50:47Z

👍 Do it.

* Add to contrib

houndci-bot reviewed Feb 5, 2016
View reviewed changes

tysongach reviewed Feb 5, 2016
View reviewed changes

wardpenney force-pushed the neat-2.0.0-basic-grid branch 3 times, most recently from 045ec39 to 49650f1 Compare February 12, 2016 21:31

whmii reviewed Feb 15, 2016
View reviewed changes

tysongach reviewed Feb 19, 2016
View reviewed changes

tysongach force-pushed the neat-2.0.0-basic-grid branch from df602fd to 4e258aa Compare March 1, 2016 01:29

tysongach reviewed Mar 1, 2016
View reviewed changes

wardpenney force-pushed the neat-2.0.0-basic-grid branch from 4e258aa to 13d4e29 Compare March 1, 2016 18:00

tysongach reviewed Mar 1, 2016
View reviewed changes

wardpenney force-pushed the neat-2.0.0-basic-grid branch 2 times, most recently from b16573b to e7005de Compare March 1, 2016 18:13

tysongach reviewed Mar 1, 2016
View reviewed changes

wardpenney force-pushed the neat-2.0.0-basic-grid branch from e7005de to 98d681a Compare March 1, 2016 18:41

houndci-bot reviewed Mar 1, 2016
View reviewed changes

wardpenney force-pushed the neat-2.0.0-basic-grid branch from 98d681a to 12d8246 Compare March 1, 2016 18:42

tysongach reviewed Mar 1, 2016
View reviewed changes

wardpenney force-pushed the neat-2.0.0-basic-grid branch from 12d8246 to 730c16b Compare March 1, 2016 18:49

houndci-bot reviewed Mar 1, 2016
View reviewed changes

wardpenney force-pushed the neat-2.0.0-basic-grid branch from 730c16b to 624ea49 Compare March 1, 2016 18:51

Create container for floated grid

f589a7f

* Add to contrib

wardpenney force-pushed the neat-2.0.0-basic-grid branch from 624ea49 to f589a7f Compare March 1, 2016 18:53

wardpenney merged commit f589a7f into neat-2.0.0 Mar 1, 2016

wardpenney deleted the neat-2.0.0-basic-grid branch March 1, 2016 18:55

whmii mentioned this pull request Mar 1, 2016

Rename outer-container ~> grid-container #374

Closed

Add grid-container #408

Add grid-container #408

Conversation

wardpenney commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuaogle commented Feb 5, 2016

tysongach commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tysongach commented Feb 5, 2016

wardpenney commented Feb 6, 2016

wardpenney commented Feb 6, 2016

tysongach commented Feb 6, 2016

whmii commented Feb 11, 2016

joshuaogle commented Feb 12, 2016

whmii commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whmii commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tysongach commented Feb 19, 2016

Choose a reason for hiding this comment

tysongach commented Mar 1, 2016

tysongach commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whmii commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tysongach commented Mar 1, 2016