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

Grids #271

Merged
merged 139 commits into from
Jan 16, 2019
Merged

Grids #271

merged 139 commits into from
Jan 16, 2019

Conversation

sherakama
Copy link
Member

@sherakama sherakama commented Nov 9, 2018

READY FOR REVIEW

Summary

  • Slight re-factoring to our grid systems & an addition of many mixins
  • A couple dozen layout examples using the css grid based grid system
  • Started to create clearer separation of what is a css grid and what is a flex grid with new naming conventions
  • Removed previous page layout examples
  • Upgraded grunt sass version to the latest
  • Added new and improved mixins for the flex grid

I went through an excercise in order to test out the Grids that are in our framework and how it is to use them. My exercise was to re-create a bunch of simple to complex layouts that I had previously created using Decanter V1 and neat's grid system. At the start I had all the tools I needed to create a few simple layouts but I was annoyed by how much code I had to write and remember to account for all of the grid breakpoints and scaling margins and gutters. This fustration lead to the creation of many mixins to help expidite and simplify the creation of responsive and flexible grid systems. I cherry picked a few mixins from internet searches as well as some from neat and reverse engineered them back in to our framework. At the end of my exercise I had a huge collection of mixins and some really clean and stripped down semantic templates.

There is still a significant amount of clean up work to be done to refactor some of our components and to ensure cross browser compatibility but this is should be a significant start.

Needed By (Date)

  • ?

Urgency

  • None

Steps to Test

  1. Will schedule a first pass meeting to go over the changes in detail with you.
  2. In the mean time please check out this branch
  3. Run npm install
  4. Compile the styleguide
  5. Open a browser and navigate to the layouts section to see the new layouts
  6. Start reading some of the code in the layout examples to see the general approach.

Affected Projects or Products

  • Pretty much everything

See Also

//
// Styleguide: Variables.Core.Gutters.gutter-xs
//
$gutter-xs: 32px !default;
Copy link
Member

Choose a reason for hiding this comment

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

Are these gutter values (for all breakpoints) correct? I'm looking at Figma Stanford Design Library under "Visual Guidelines" -> "Responsive Notes", it seems the gutter values should be:
XS = 20px
SM = 20px
MD = 20px
LG = 36px
XL = 40px
XXL = 48px

Choose a reason for hiding this comment

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

We should use the Figma specs for these. I believe the Figma document was updated more recently than when we spun up the Grids branch with this values. We should update these on this branch.

Copy link
Member

Choose a reason for hiding this comment

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

Great. I suspected that they just haven't been updated. Good to know.

Choose a reason for hiding this comment

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

Do you wanna throw this update up? I'm happy to do so if you're in the middle of other things today.

Copy link
Member

Choose a reason for hiding this comment

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

Sure! Looks like this one would be easy to update on GitHub directly. I'll do that now.

Choose a reason for hiding this comment

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

Yep - should just be value changes. Thanks YT! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

You two rock. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Thank YOU - both of you. I feel bad for taking this long to review this PR.

$gutter: map-get($gutters, $code);

@media #{$breakpoint} {
max-width: $max-width - $gutter;
Copy link
Member

@yvonnetangsu yvonnetangsu Jan 15, 2019

Choose a reason for hiding this comment

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

It seems like the way the centered column max-width is calculated here is different from what's proposed in Figma. Currently in this branch, for each breakpoint, the max-width of the centered column is a fixed number, eg, for the XL breakpoint, it's the max-width of the breakpoint (1200px) - width of the XL gutter, and the grid layouts behave this way.

In the Figma file, the centered column max-width uses what the designers called "margin" (under Visual Guidelines -> Responsive Notes):
XS = 20px
SM = 30px
MD = 50px
LG = 80px
XL = 100px
XXL = max 1500px (fixed max container width for XXL)
So basically the proposed centered column width is (browser_width - 2*margin) and is fluid. Just want to check in and see if we have decided to use a different system.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see what you are saying here. This mixin could use a refactor. Yes, this is too "jumpy" and "rigid". I would prefer the more elastic and fluid feel to this.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Maybe we could talk more about this in the Decanter meeting tomorrow?

core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
core/scss/utilities/variables/core/_gutter.scss Outdated Show resolved Hide resolved
@josephgknox
Copy link

npm*; updating to the latest and pushing up (testing with Brian Young).

&--row-gap {
> * {
@include grid-media('2xl') {
@include margin(0 0 map-get($modular-spacing, '2xl') 0);
Copy link
Member

@yvonnetangsu yvonnetangsu Jan 16, 2019

Choose a reason for hiding this comment

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

Looks like we either need to update the variable file for modular-spacing, or change $modular-spacing to $grid-gutters.

@sherakama sherakama merged commit 5111eaa into master Jan 16, 2019
@sherakama sherakama deleted the grids branch January 16, 2019 21:20
yvonnetangsu added a commit that referenced this pull request Jan 16, 2019
* master:
  Grids (#271)
  minor fixes to some of the comments in SCSS files. (#278)

# Conflicts:
#	core/css/decanter.css
#	core/scss/core/index.scss
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants