Issue 4609 - Default grid component

This review depends on https://codereview.adblockplus.org/29361647/

=== Background ===

Building upon #4606, we would like to implement a grid component.

=== Requirements ===

- Desktop
    - 4 columns
- Tablet
    - 2 columns
- Mobile
    - 1 column
- Nesting
- Reverse order
- Right-to-left direction
- Support [http://browserl.ist/?q=last+2+versions%2C+Safari+6%2C+IE+8 last 2 versions, Safari 6, IE 8]

=== What to change ===

- Implement grid CSS
- Implement grid demo

[juliandoucette, 2016-11-03 23:22:26]

I contradicted my documentation by mistake by confusing one-fourth and one-half
in _grid.scss. I will update this tomorrow.

https://codereview.adblockplus.org/29361708/diff/29361717/scss/_grid.scss
File scss/_grid.scss (right):

https://codereview.adblockplus.org/29361708/diff/29361717/scss/_grid.scss#new...
scss/_grid.scss:74: .one-half
This should be one-fourth not one-half.

https://codereview.adblockplus.org/29361708/diff/29361717/scss/_grid.scss#new...
scss/_grid.scss:100: @media(min-width: $desktop-breakpoint)
I have to add .two-fourths and .one-half here.

[juliandoucette, 2016-11-08 16:09:52]

- Fixed one-half one-fourth mix up I comment on
- Added units to 0s
- Adjusted HTML content hirarchy

https://codereview.adblockplus.org/29361708/diff/29361717/scss/_grid.scss
File scss/_grid.scss (right):

https://codereview.adblockplus.org/29361708/diff/29361717/scss/_grid.scss#new...
scss/_grid.scss:74: .one-half
On 2016/11/03 23:22:26, juliandoucette wrote:
> This should be one-fourth not one-half.

Done.

https://codereview.adblockplus.org/29361708/diff/29361717/scss/_grid.scss#new...
scss/_grid.scss:100: @media(min-width: $desktop-breakpoint)
On 2016/11/03 23:22:26, juliandoucette wrote:
> I have to add .two-fourths and .one-half here. 

Done.

[saroyanm, 2016-11-08 16:19:40]

Sorry that it take longer than expected.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss
File scss/_grid.scss (right):

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:19: .container,
What suppose to be the difference between the container and .row ?

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:27: .container:after,
Note: we might use double colon as soon we will drop IE8 support.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:32: display: table;
I'll suggest to use display: block;
I can't see reason of using display: table; if you are not planing to use table
like structure. While clearing with the block element sounds more reasonable IMO
and provide less questions even if it's used with the display:table in other
examples.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:38: margin: 0px -$small-space;
Why do we need to use negative margins here ?

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:49: [dir="ltr"] .column
detail: .column selector should be enough, same applies to similar "ltr" styles
below.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:51: float: left;
I'm not opposed of using floats, because seems like having the solution of
clearing it should cover most of the issues that appears when people forget to
clear, but I'd like to ask @Thomas opinion regarding this as well.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:63: width: 540px;
Why is the width specification for the container is necessary here and below ?
Maybe I do not know the difference between container and row, that why it's
questionable for me.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:100: @media(min-width: $desktop-breakpoint)
I can't see a reason why we need to specify desktop-breakpoint, I think this
should be the default style, BTW the mobile-breakpoint is missing.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:137: [dir="ltr"] .pull-one-fourth
A suggestion regarding pulling and pushing. 
I'm not really sure if we will use the part of the functionality from it, so
maybe we can make it simpler, what about current implementation ? It will make
things more simpler IMO:

.one-fourth
{
  width: 25%;
}

.one-fourth.move-to-end
{
  left: 75%;
}

.one-fourth.move-to-start
{
  left: 0px;
}

Let me know if you think that in our designs we are going to use pull and push
functionality in the way you have implemented.

[juliandoucette, 2016-11-09 14:51:17]

Julian + Manvel reviewed in meeting.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss
File scss/_grid.scss (right):

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:19: .container,
On 2016/11/08 16:19:40, saroyanm wrote:
> What suppose to be the difference between the container and .row ?

Acknowledged.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:19: .container,
On 2016/11/08 16:19:40, saroyanm wrote:
> What suppose to be the difference between the container and .row ?

We decided to move .container to _content.scss.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:27: .container:after,
On 2016/11/08 16:19:40, saroyanm wrote:
> Note: we might use double colon as soon we will drop IE8 support.

Acknowledged.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:32: display: table;
On 2016/11/08 16:19:40, saroyanm wrote:
> I'll suggest to use display: block;
> I can't see reason of using display: table; if you are not planing to use
table
> like structure. While clearing with the block element sounds more reasonable
IMO
> and provide less questions even if it's used with the display:table in other
> examples.

Acknowledged.

I will change this to display:block.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:38: margin: 0px -$small-space;
On 2016/11/08 16:19:39, saroyanm wrote:
> Why do we need to use negative margins here ?

It should be (-$small-space) to avoid incorrect calculation.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:38: margin: 0px -$small-space;
On 2016/11/08 16:19:39, saroyanm wrote:
> Why do we need to use negative margins here ?

Negative margin reasoning:

To add consistent padding around all columns except for the first left padding
and last right padding.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:49: [dir="ltr"] .column
On 2016/11/08 16:19:39, saroyanm wrote:
> detail: .column selector should be enough, same applies to similar "ltr"
styles
> below.

Acknowledged.

This only applies to properties which are overridden by rtl.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:51: float: left;
On 2016/11/08 16:19:40, saroyanm wrote:
> I'm not opposed of using floats, because seems like having the solution of
> clearing it should cover most of the issues that appears when people forget to
> clear, but I'd like to ask @Thomas opinion regarding this as well.

Julian - Acknowledged.

@Thomas's opinion is still requested.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:63: width: 540px;
On 2016/11/08 16:19:39, saroyanm wrote:
> Why is the width specification for the container is necessary here and below ?
> Maybe I do not know the difference between container and row, that why it's
> questionable for me.

Acknowledged.

- We are moving container outside
- We set the width of the regular container to avoid scaling edge cases

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:100: @media(min-width: $desktop-breakpoint)
On 2016/11/08 16:19:39, saroyanm wrote:
> I can't see a reason why we need to specify desktop-breakpoint, I think this
> should be the default style, BTW the mobile-breakpoint is missing.

Acknowledged.

- Since we are styling mobile first, $mobile-breakpoint is not necessary

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:137: [dir="ltr"] .pull-one-fourth
On 2016/11/08 16:19:40, saroyanm wrote:
> A suggestion regarding pulling and pushing. 
> I'm not really sure if we will use the part of the functionality from it, so
> maybe we can make it simpler, what about current implementation ? It will make
> things more simpler IMO:
> 
> .one-fourth
> {
>   width: 25%;
> }
> 
> .one-fourth.move-to-end
> {
>   left: 75%;
> }
> 
> .one-fourth.move-to-start
> {
>   left: 0px;
> }
> 
> Let me know if you think that in our designs we are going to use pull and push
> functionality in the way you have implemented.

Acknowledged.

- naming TBD
- we'll cover 1/2, 1/3 + 2/3, 1/4 + 3/4 (not individual 1/4s and 1/3s)

[juliandoucette, 2016-11-09 19:47:19]

I've addressed the comments from our meeting today.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss
File scss/_grid.scss (right):

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:19: .container,
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:40, saroyanm wrote:
> > What suppose to be the difference between the container and .row ?
> 
> We decided to move .container to _content.scss.

Done.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:27: .container:after,
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:40, saroyanm wrote:
> > Note: we might use double colon as soon we will drop IE8 support.
> 
> Acknowledged.

Done.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:32: display: table;
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:40, saroyanm wrote:
> > I'll suggest to use display: block;
> > I can't see reason of using display: table; if you are not planing to use
> table
> > like structure. While clearing with the block element sounds more reasonable
> IMO
> > and provide less questions even if it's used with the display:table in other
> > examples.
> 
> Acknowledged.
> 
> I will change this to display:block.

Done.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:38: margin: 0px -$small-space;
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:39, saroyanm wrote:
> > Why do we need to use negative margins here ?
> 
> Negative margin reasoning:
> 
> To add consistent padding around all columns except for the first left padding
> and last right padding.

Done.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:49: [dir="ltr"] .column
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:39, saroyanm wrote:
> > detail: .column selector should be enough, same applies to similar "ltr"
> styles
> > below.
> 
> Acknowledged.
> 
> This only applies to properties which are overridden by rtl. 

Done.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:63: width: 540px;
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:39, saroyanm wrote:
> > Why is the width specification for the container is necessary here and below
?
> > Maybe I do not know the difference between container and row, that why it's
> > questionable for me.
> 
> Acknowledged.
> 
> - We are moving container outside
> - We set the width of the regular container to avoid scaling edge cases

Done.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:100: @media(min-width: $desktop-breakpoint)
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:39, saroyanm wrote:
> > I can't see a reason why we need to specify desktop-breakpoint, I think this
> > should be the default style, BTW the mobile-breakpoint is missing.
> 
> Acknowledged.
> 
> - Since we are styling mobile first, $mobile-breakpoint is not necessary 

Done.

https://codereview.adblockplus.org/29361708/diff/29362045/scss/_grid.scss#new...
scss/_grid.scss:137: [dir="ltr"] .pull-one-fourth
On 2016/11/09 14:51:16, juliandoucette wrote:
> On 2016/11/08 16:19:40, saroyanm wrote:
> > A suggestion regarding pulling and pushing. 
> > I'm not really sure if we will use the part of the functionality from it, so
> > maybe we can make it simpler, what about current implementation ? It will
make
> > things more simpler IMO:
> > 
> > .one-fourth
> > {
> >   width: 25%;
> > }
> > 
> > .one-fourth.move-to-end
> > {
> >   left: 75%;
> > }
> > 
> > .one-fourth.move-to-start
> > {
> >   left: 0px;
> > }
> > 
> > Let me know if you think that in our designs we are going to use pull and
push
> > functionality in the way you have implemented.
> 
> Acknowledged.
> 
> - naming TBD
> - we'll cover 1/2, 1/3 + 2/3, 1/4 + 3/4 (not individual 1/4s and 1/3s)

Done.

[juliandoucette, 2016-11-10 16:08:15]

Quick note below.

https://codereview.adblockplus.org/29361708/diff/29362199/html/grid.html
File html/grid.html (right):

https://codereview.adblockplus.org/29361708/diff/29362199/html/grid.html#newc...
html/grid.html:28: /* NOTE: The following inline styles are meant for this demo
**only**. */
I should probably explain what I'm doing with the classes below in comments.

https://codereview.adblockplus.org/29361708/diff/29362199/html/grid.html#newc...
html/grid.html:59: .clearfix:after
This is redundant. _content.scss will define .clearfix.

[saroyanm, 2016-11-15 16:09:44]

https://codereview.adblockplus.org/29361708/diff/29362505/html/grid.html
File html/grid.html (right):

https://codereview.adblockplus.org/29361708/diff/29362505/html/grid.html#newc...
html/grid.html:30: .example
I think people still may refer to this document as an example of the
implementation, so I'll suggest to rename the .example to something more
meaningful, ex.: content-wrapper ?

https://codereview.adblockplus.org/29361708/diff/29362505/html/grid.html#newc...
html/grid.html:30: .example
Note: considering the comment above I think it make sense to setup height and
width to the 100%, we are relying to the browser to set the height and width,
while in current example it works people might think it's a way to go for the
wrappers.

https://codereview.adblockplus.org/29361708/diff/29362505/html/grid.html#newc...
html/grid.html:35: margin: 0em 0em 1em 0em;
Suggestion: What about setting margin-bottom on the column element ?
In this example seems like that the content of the clumn is pushing the column
that is placed below, but not the column itself, which is more intuitive IMO.

https://codereview.adblockplus.org/29361708/diff/29362505/scss/_grid.scss
File scss/_grid.scss (right):

https://codereview.adblockplus.org/29361708/diff/29362505/scss/_grid.scss#new...
scss/_grid.scss:23: * .row clears .column(s)
Detail: this belong to the selector below I guess.

https://codereview.adblockplus.org/29361708/diff/29362505/scss/_grid.scss#new...
scss/_grid.scss:48: min-height: 1px;
Deatail: can't see a reason of overwriting the initial value, is there one ?

[juliandoucette, 2016-11-16 17:17:38]

Comments from review meeting.

https://codereview.adblockplus.org/29361708/diff/29362505/html/grid.html
File html/grid.html (right):

https://codereview.adblockplus.org/29361708/diff/29362505/html/grid.html#newc...
html/grid.html:30: .example
On 2016/11/15 16:09:41, saroyanm wrote:
> I think people still may refer to this document as an example of the
> implementation, so I'll suggest to rename the .example to something more
> meaningful, ex.: content-wrapper ?

Acknowledged.

We will clarify this class name and function.

https://codereview.adblockplus.org/29361708/diff/29362505/html/grid.html#newc...
html/grid.html:35: margin: 0em 0em 1em 0em;
On 2016/11/15 16:09:44, saroyanm wrote:
> Suggestion: What about setting margin-bottom on the column element ?
> In this example seems like that the content of the clumn is pushing the column
> that is placed below, but not the column itself, which is more intuitive IMO.

Acknowledged.

See comment above.

https://codereview.adblockplus.org/29361708/diff/29362505/scss/_grid.scss
File scss/_grid.scss (right):

https://codereview.adblockplus.org/29361708/diff/29362505/scss/_grid.scss#new...
scss/_grid.scss:19: /** Grid component */
Note: We will test a margin-left + :first-child grid solution.

https://codereview.adblockplus.org/29361708/diff/29362505/scss/_grid.scss#new...
scss/_grid.scss:23: * .row clears .column(s)
On 2016/11/15 16:09:44, saroyanm wrote:
> Detail: this belong to the selector below I guess.

Acknowledged.

https://codereview.adblockplus.org/29361708/diff/29362505/scss/_grid.scss#new...
scss/_grid.scss:48: min-height: 1px;
On 2016/11/15 16:09:44, saroyanm wrote:
> Deatail: can't see a reason of overwriting the initial value, is there one ?

Acknowledged.

A column must have at least 1px height to stack horizontally.

[saroyanm, 2016-11-21 19:14:47]

LGTM

https://codereview.adblockplus.org/29361708/diff/29363517/html/grid.html
File html/grid.html (right):

https://codereview.adblockplus.org/29361708/diff/29363517/html/grid.html#newc...
html/grid.html:30: .bg-inverse
Deatail: I think this is used as a container, but not just as an inverse color
class. I'll suggest to rename it to "content-wrapper" or "column-container".

[juliandoucette, 2016-11-30 18:27:43]

Notes from review meeting.

https://codereview.adblockplus.org/29361708/diff/29363517/html/grid.html
File html/grid.html (right):

https://codereview.adblockplus.org/29361708/diff/29363517/html/grid.html#newc...
html/grid.html:30: .bg-inverse
On 2016/11/21 19:14:46, saroyanm wrote:
> Deatail: I think this is used as a container, but not just as an inverse color
> class. I'll suggest to rename it to "content-wrapper" or "column-container".

Acknowledged.

[saroyanm, 2016-12-01 19:11:06]

Please don't push this yet, I would like to have a look, noticed some more
changes just would like to know the reason.

https://codereview.adblockplus.org/29361708/diff/29366710/demo/grid.html
File demo/grid.html (right):

https://codereview.adblockplus.org/29361708/diff/29366710/demo/grid.html#newc...
demo/grid.html:45: <div class="one-half column"><p class="inverse-text">One
half</p></div>
* What was the reason of renaming the classes from bg-inverse to inverse-text ? 
* And why we are currently using Paragraphs instead of DIVs

[saroyanm, 2016-12-02 14:31:30]

LGTM

[saroyanm, 2016-12-13 15:14:55]

Note: I think this review can be closed