feat(layout-grid): Add fixed column width layout grid modifier. #816

yeelan0319 · 2017-06-13T15:01:38Z

Closes #748.

traviskaufman

✅ 💯 Looks great

codecov-io · 2017-06-14T14:51:19Z

Codecov Report

Merging #816 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #816   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files          68       68           
  Lines        3144     3144           
  Branches      387      387           
=======================================
  Hits         3142     3142           
  Misses          2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 184897b...3022b65. Read the comment docs.

lynnmercier · 2017-06-14T14:48:58Z

demos/layout-grid.html

@@ -342,6 +384,21 @@
          document.documentElement.style.setProperty('--mdc-layout-grid-gutter-phone', gutterSelectPhone.value);
        });

+        var ColumnWidthSelectDesktop = document.querySelector('#desktop-column-width');


is there a reason these variables are capitalized?

lynnmercier · 2017-06-14T14:49:02Z

packages/mdc-layout-grid/README.md

-A grid consists of a group of cells, which get positioned in sequence according to a predefined number of columns.
-Cells specify how many columns to span (the default being 4), and get placed side by side while there is room. When
-there isn't enough room for a cell, it gets moved to the beginning of the next row, and placement continues as usual.
+A grid is a container that consists of a group of cells. The grids can define its own max-width or designate its column to be a certain width. Cells get positioned in sequence according to a predefined number of columns.


"The grids"...do you mean "the cells"? I thought there was only one grid

Yep. The grids I meant for mdc grids in general, but it seems use single will remove some confusion, changing that.

lynnmercier · 2017-06-14T14:49:03Z

packages/mdc-layout-grid/README.md


 The grid has 12 columns in desktop mode (>= 840px), 8 columns in tablet mode (>= 480px), and 4 columns in phone mode
-(< 480px). Column widths are variable; margins and gutters are fixed, with columns taking up the remainder of the space.
+(< 480px). Cells specify how many columns to span (the default being 4), and get placed side by side while there is room. When there isn't enough room for a cell, it gets moved to the beginning of the next row, and placement continues as usual.


Cells specify how many columns to span (the default is 4). Cells are placed side by side until there is no more room, then the next cell is placed at the beginning of the next row.

lynnmercier · 2017-06-14T14:50:19Z

packages/mdc-layout-grid/README.md


-Margins (the space between the edge of the grid and the edge of the first cell) and gutters (the space between edges of adjacent cells) can be customized on different devices respectively based on design needs. Layout grids set default margins and gutters to 24px on desktop, 16px on tablet and phone, according to the Material Design spec.
+ Margins (the space between the edge of the grid and the edge of the first cell) and gutters (the space between edges of adjacent cells) can be customized on different devices respectively based on design needs. The columns are evenly distributed after container minus all margin and gutter width. Layout grids set default margins and gutters to 24px on desktop, 16px on tablet and phone, according to the Material Design spec.


The columns are evenly distributed within the container width, minus the width of all margins and gutters.

lynnmercier · 2017-06-14T14:56:44Z

packages/mdc-layout-grid/_mixins.scss

@@ -122,3 +122,17 @@
    align-self: stretch;
  }
 }
+
+@mixin mdc-layout-grid-fixed-column-width($size, $margin, $gutter, $column-width) {


$size is kinda a vague variable...can it be $platform-type?

Can we also add the same error handling in the sass as we do in mdc-theme? e.g.:

@if not map-has-key($mdc-layout-grid-columns, $size) { @error "Invalid style specified! Choose one of #{map-keys($mdc-layout-grid-columns)}"; }

I feel the original code meant screen-size and I don't think it is vague. Since we both documented that clearly and do the type check, I feel fine to leave just as it is. WDYT?

lynnmercier · 2017-06-14T14:57:01Z

packages/mdc-layout-grid/_mixins.scss

@@ -122,3 +122,17 @@
    align-self: stretch;
  }
 }
+
+@mixin mdc-layout-grid-fixed-column-width($size, $margin, $gutter, $column-width) {
+  $columns: map-get($mdc-layout-grid-columns, $size);


$columnCount?

Sure thing:)

amsheehan

One nit, one improvement requested 😺

amsheehan · 2017-06-14T15:17:27Z

packages/mdc-layout-grid/_mixins.scss

@@ -122,3 +122,17 @@
    align-self: stretch;
  }
 }
+
+@mixin mdc-layout-grid-fixed-column-width($size, $margin, $gutter, $column-width) {


Can we also add the same error handling in the sass as we do in mdc-theme? e.g.:

@if not map-has-key($mdc-layout-grid-columns, $size) { @error "Invalid style specified! Choose one of #{map-keys($mdc-layout-grid-columns)}"; }

amsheehan · 2017-06-14T15:19:30Z

packages/mdc-layout-grid/_variables.scss

@@ -36,6 +36,12 @@ $mdc-layout-grid-default-gutter: (
  phone: 16px
 ) !default;

+$mdc-layout-grid-column-width: (


Maybe $mdc-layout-grid-platform-column-widths? It more closely follows the naming conventions of this pattern in other components.

It would obviously have to be changed in _mixins.scss as well

I tried to look up for this pattern but haven't seen any one have this pattern 😞 The thing I search and found that use sass map to define different property based on screen size only appears in mdc-layout-grid.

Not sure if I misunderstand what you meant here, could you elaborate a bit?

Closes #748.

amsheehan

lgtm

yeelan0319 added this to the Layout Improvement milestone Jun 13, 2017

yeelan0319 force-pushed the feat/fixed-column-width-layout-grid branch 2 times, most recently from abd9153 to 7084558 Compare June 14, 2017 14:33

traviskaufman self-requested a review June 14, 2017 14:44

traviskaufman approved these changes Jun 14, 2017

View reviewed changes

lynnmercier suggested changes Jun 14, 2017

View reviewed changes

amsheehan suggested changes Jun 14, 2017

View reviewed changes

feat(layout-grid): Add fixed column width layout grid modifier.

3022b65

Closes #748.

yeelan0319 force-pushed the feat/fixed-column-width-layout-grid branch from 7084558 to 3022b65 Compare June 15, 2017 18:57

amsheehan approved these changes Jun 22, 2017

View reviewed changes

lynnmercier approved these changes Jun 22, 2017

View reviewed changes

yeelan0319 merged commit 94d62ad into master Jun 22, 2017

yeelan0319 deleted the feat/fixed-column-width-layout-grid branch August 25, 2017 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(layout-grid): Add fixed column width layout grid modifier. #816

feat(layout-grid): Add fixed column width layout grid modifier. #816

yeelan0319 commented Jun 13, 2017

traviskaufman left a comment

codecov-io commented Jun 14, 2017 •

edited

Loading

lynnmercier Jun 14, 2017

lynnmercier Jun 14, 2017

yeelan0319 Jun 15, 2017

lynnmercier Jun 14, 2017

lynnmercier Jun 14, 2017

lynnmercier Jun 14, 2017

amsheehan Jun 14, 2017

yeelan0319 Jun 15, 2017

lynnmercier Jun 14, 2017

yeelan0319 Jun 15, 2017

amsheehan left a comment

amsheehan Jun 14, 2017

amsheehan Jun 14, 2017

yeelan0319 Jun 15, 2017

amsheehan left a comment


		Margins (the space between the edge of the grid and the edge of the first cell) and gutters (the space between edges of adjacent cells) can be customized on different devices respectively based on design needs. Layout grids set default margins and gutters to 24px on desktop, 16px on tablet and phone, according to the Material Design spec.
		Margins (the space between the edge of the grid and the edge of the first cell) and gutters (the space between edges of adjacent cells) can be customized on different devices respectively based on design needs. The columns are evenly distributed after container minus all margin and gutter width. Layout grids set default margins and gutters to 24px on desktop, 16px on tablet and phone, according to the Material Design spec.

feat(layout-grid): Add fixed column width layout grid modifier. #816

feat(layout-grid): Add fixed column width layout grid modifier. #816

Conversation

yeelan0319 commented Jun 13, 2017

traviskaufman left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 14, 2017 • edited Loading

Codecov Report

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

amsheehan left a 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

amsheehan left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 14, 2017 •

edited

Loading