Issue 5135 - Reduce font size and padding on smaller screens on acceptableads.com

Issue 5135 - Reduce font size and padding on smaller screens on acceptableads.com

[juliandoucette, 2017-05-16 15:51:33]

(This is going to take me a little while.)

[juliandoucette, 2017-05-16 19:07:35]

https://codereview.adblockplus.org/29438582/diff/29438583/pages/about/best-pr...
File pages/about/best-practices.md (right):

https://codereview.adblockplus.org/29438582/diff/29438583/pages/about/best-pr...
pages/about/best-practices.md:18: <div class="bg-accent section" markdown="1">
Is there a reason why you put section here instead of below? It seems to be
placed inconsistently across pages.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:41: margin: $lg - $sm 0 $md 0;
Awesome!

(NIT: No extra line breaks please.)

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:50: @media screen and (max-width:
$tablet-breakpoint)
We have been restricting each file to one media query per size and including
selectors within.

I like your approach. Would you like to make a case for it at a frontend
developer meeting and potentially change our code style?

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:50: @media screen and (max-width:
$tablet-breakpoint)
Why screen? This would be useful for print too?

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
File static/scss/content/_reset.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
static/scss/content/_reset.scss:95: // margin-top: -$sm;
Better to delete it and add comment on this line in the code review.

[ire, 2017-05-16 21:32:28]

https://codereview.adblockplus.org/29438582/diff/29438583/pages/about/best-pr...
File pages/about/best-practices.md (right):

https://codereview.adblockplus.org/29438582/diff/29438583/pages/about/best-pr...
pages/about/best-practices.md:18: <div class="bg-accent section" markdown="1">
On 2017/05/16 19:07:34, juliandoucette wrote:
> Is there a reason why you put section here instead of below? It seems to be
> placed inconsistently across pages.

Yes. So this seemed to fit the description/styling of what a ".section" is.
Plus, by making it a .section I was able to change the margin responsively, as
opposed to the fixed xl margin previously applied.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:41: margin: $lg - $sm 0 $md 0;
On 2017/05/16 19:07:34, juliandoucette wrote:
> Awesome!
> 
> (NIT: No extra line breaks please.)

ACK

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:50: @media screen and (max-width:
$tablet-breakpoint)
On 2017/05/16 19:07:34, juliandoucette wrote:
> We have been restricting each file to one media query per size and including
> selectors within.
> 
> I like your approach. Would you like to make a case for it at a frontend
> developer meeting and potentially change our code style?

Thanks! Yes I will.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:50: @media screen and (max-width:
$tablet-breakpoint)
On 2017/05/16 19:07:34, juliandoucette wrote:
> Why screen? This would be useful for print too?

Kind of a habit to write screen for most cases. You're right, will make this
more agnostic

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
File static/scss/content/_reset.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
static/scss/content/_reset.scss:95: // margin-top: -$sm;
On 2017/05/16 19:07:34, juliandoucette wrote:
> Better to delete it and add comment on this line in the code review.

ACK

[juliandoucette, 2017-05-17 13:09:11]

Thanks Ire!

Detail: I see you have been replying "ACK". This is not a problem. But there is
an "Ack" button beside "Reply" in Rietveld.

https://codereview.adblockplus.org/29438582/diff/29438583/pages/about/best-pr...
File pages/about/best-practices.md (right):

https://codereview.adblockplus.org/29438582/diff/29438583/pages/about/best-pr...
pages/about/best-practices.md:18: <div class="bg-accent section" markdown="1">
On 2017/05/16 21:32:27, ire wrote:
> On 2017/05/16 19:07:34, juliandoucette wrote:
> > Is there a reason why you put section here instead of below? It seems to be
> > placed inconsistently across pages.
> 
> Yes. So this seemed to fit the description/styling of what a ".section" is.
> Plus, by making it a .section I was able to change the margin responsively, as
> opposed to the fixed xl margin previously applied.

Acknowledged.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:50: @media screen and (max-width:
$tablet-breakpoint)
> Thanks! Yes I will.

Cool :)

Can you [explain in markdown, upload explanation to codereview, and set Manvel,
Thomas, and I as reviewers] please?

That way, we may or may not have to fix the @media style in this change.

[ire, 2017-05-18 00:25:28]

On 2017/05/17 13:09:11, juliandoucette wrote:
> Detail: I see you have been replying "ACK". This is not a problem. But there
is
> an "Ack" button beside "Reply" in Rietveld.

Just noticed that!

[ire, 2017-05-18 00:27:14]

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:50: @media screen and (max-width:
$tablet-breakpoint)
On 2017/05/17 13:09:11, juliandoucette wrote:
> 
> Cool :)
> 
> Can you [explain in markdown, upload explanation to codereview, and set
Manvel,
> Thomas, and I as reviewers] please?
> 
> That way, we may or may not have to fix the @media style in this change.

Sure. Do you mean as a separate codereview (i.e. a different repo with the one
file)?

[juliandoucette, 2017-05-18 08:45:12]

> Sure. Do you mean as a separate codereview (i.e. a different repo with the one
> file)?

Yup :)

[juliandoucette, 2017-05-30 14:22:57]

Thanks Ire! :)

A few high level points:

- Don't forget about removing @media screen
- I think we need to address more heading font sizes
  - e.g. http://localhost:5000/en/committee
    - It would be nice if "Acceptable Ads Committe" fit on one line
    - We could fit blog titles better if their font size was smaller
  - e.g. http://localhost:5000/en/about/criteria
    - h2 seems too large IMO

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:45: img.card-icon,
I don't think that these icons are too big on mobile :/

If you do, can you please run this by product and address it in a separate
issue?

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_groups.scss:26: @media screen and (max-width:
$desktop-breakpoint)
Note: There was an inconsistent border-left on card group items that was made
more apparent to me when reviewing this change. I pushed an non-issue fix for
this https://hg.adblockplus.org/web.acceptableads.com/rev/86d35c4f2fb5.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_groups.scss:40: .group-container > p
Looks good on mobile, but not on desktop. I think we should shrink this space on
desktop too.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
static/scss/content/_typography.scss:37: margin: $md 0 $lg 0;
NIT: It would be more straightforward if you just changed margin-top

[ire, 2017-05-30 17:44:48]

On 2017/05/30 14:22:57, juliandoucette wrote:
> - Don't forget about removing @media screen

Noted.

> - I think we need to address more heading font sizes
>   - e.g. http://localhost:5000/en/committee
>     - It would be nice if "Acceptable Ads Committe" fit on one line
>     - We could fit blog titles better if their font size was smaller
>   - e.g. http://localhost:5000/en/about/criteria
>     - h2 seems too large IMO

Okay, will work on this too.

[ire, 2017-05-30 17:44:59]

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_cards.scss:45: img.card-icon,
On 2017/05/30 14:22:56, juliandoucette wrote:
> I don't think that these icons are too big on mobile :/
> 
> If you do, can you please run this by product and address it in a separate
> issue?

Acknowledged.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_groups.scss:26: @media screen and (max-width:
$desktop-breakpoint)
On 2017/05/30 14:22:57, juliandoucette wrote:
> Note: There was an inconsistent border-left on card group items that was made
> more apparent to me when reviewing this change. I pushed an non-issue fix for
> this https://hg.adblockplus.org/web.acceptableads.com/rev/86d35c4f2fb5.

Okay. Do I need to apply those changes here, or will the difference be resolved
when this issue is pushed?

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_groups.scss:40: .group-container > p
On 2017/05/30 14:22:57, juliandoucette wrote:
> Looks good on mobile, but not on desktop. I think we should shrink this space
on
> desktop too. 

Acknowledged.

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/content...
static/scss/content/_typography.scss:37: margin: $md 0 $lg 0;
On 2017/05/30 14:22:57, juliandoucette wrote:
> NIT: It would be more straightforward if you just changed margin-top

Acknowledged.

[juliandoucette, 2017-06-07 18:05:29]

I answered your question below :)

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29438582/diff/29438583/static/scss/compone...
static/scss/components/_groups.scss:26: @media screen and (max-width:
$desktop-breakpoint)
On 2017/05/30 17:44:59, ire wrote:
> On 2017/05/30 14:22:57, juliandoucette wrote:
> > Note: There was an inconsistent border-left on card group items that was
made
> > more apparent to me when reviewing this change. I pushed an non-issue fix
for
> > this https://hg.adblockplus.org/web.acceptableads.com/rev/86d35c4f2fb5.
> 
> Okay. Do I need to apply those changes here, or will the difference be
resolved
> when this issue is pushed?

You can just pull and rebase if there is a conflict (I don't think there will
be).

[ire, 2017-06-08 12:53:25]

On 2017/06/07 18:05:29, juliandoucette wrote:
> You can just pull and rebase if there is a conflict (I don't think there will
> be).

Got it, I will do that when I am pushing. Are there any other notes on this?

[juliandoucette, 2017-06-09 14:08:27]

I still don't think we are going far enough...

- Look at the homepage on an iphone 4:
  - It seems like "Support better advertising." should fit on one line
  - It seems like we can't fit much on one line at 18pm (body text)
  - Despite .item h3 being only 22px the font and style make it look huge
  - It seems like there is way too much vertical space in the cards
- Look at the criteria page on an iphone 4:
  - List items seem really squashed
  - Headings and subheadings seem too big and bold and barely different from
each other
  - It seems like there is too much space between headings and images
- Look at the solutions page on an iphone 4
  - There should be space between the text in cards and the buttons
- Look at any solutions page on an iphone 4
  - There should be less space after the horizontal rule under the h1
  - There should be less space around the Inquire button
  - There should be less space at the bottom after the "Acceptable Ads" provider
table

The list goes on...

What you have so far is great. But I don't think it resolves this issue.

We can break this up into several issues/reviews if it makes things easier.

https://codereview.adblockplus.org/29438582/diff/29452577/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29438582/diff/29452577/static/scss/compone...
static/scss/components/_groups.scss:62: .group-container > %headings
NIT: I don't like `margin-top: 0` here. It seems like we should drop the
vertical padding on `.group-container` and rely on the margin applied to it's
children for vertical spacing. What do you think?

[ire, 2017-06-14 15:41:13]

> I still don't think we are going far enough...

Noted. For some reason I had made the (incorrect) assumption this was primarily
for specific headings, not the font size overall. I will look into this more
thoroughly.


> We can break this up into several issues/reviews if it makes things easier.

Not necessary.

https://codereview.adblockplus.org/29438582/diff/29452577/static/scss/compone...
File static/scss/components/_groups.scss (right):

https://codereview.adblockplus.org/29438582/diff/29452577/static/scss/compone...
static/scss/components/_groups.scss:62: .group-container > %headings
On 2017/06/09 14:08:26, juliandoucette wrote:
> NIT: I don't like `margin-top: 0` here. It seems like we should drop the
> vertical padding on `.group-container` and rely on the margin applied to it's
> children for vertical spacing. What do you think?

I'm not sure about this. The way I see it, all `.group-container`s should have
the same amount of padding. Relying on the children will make the padding
inconsistent, and also depends on the children having a top margin to begin
with. 

Note: For most things like headings, I prefer to apply margins in one direction
only (usually bottom). This way there is less conflict. I prefer margin-bottom
as I've found that margin-top is more likely to interfere with other things on
the page.

In this case, I think the overriding of the top margin is fine. Given the way
margins are applied, I think we are bound to have cases were overriding is
necessary.

[ire, 2017-06-15 11:16:23]

I've gone through the site more thoroughly and have resized the font and spacing
in most cases. However, in cases where the utility classes are used (e.g.
`.m-y-lg` on the Publishers page) I haven't handled this yet. I would like to
get your opinion on how you think we should handle this. There are two routes we
can take:

1. We create an extra set of utility classes that are responsive, e.g.
`.m-y-lg-r`. These classes will resize shrink down with smaller screen sizes.
The major issue with this of course is that it adds even more bloat to the site.
However, we don't need to create a responsive size for every type of class. For
example, we may only want to have responsive classes for the `lg` or `xl` sizes,
as the classes for smaller sizes work on smaller screens anyway. 

2. We remove the uses of the utility classes in the instances they are causing a
problem and style each element individually. Although this will result in less
unused CSS, there will be a lot of repetition of styles. 

I don't have a strong preference either way, so let me know what you think.

https://codereview.adblockplus.org/29438582/diff/29466580/pages/committee/ind...
File pages/committee/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29466580/pages/committee/ind...
pages/committee/index.md:29: ---
This page was missing an <hr> so I added it.

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
static/scss/content/_typography.scss:162: margin-top: 0;
I had to override this because it was adding extra padding where there didn't
need to be, and making the space between the page heading and the start of the
content seem larger

[ire, 2017-06-15 17:26:55]

On 2017/06/15 11:16:23, ire wrote: 
>
https://codereview.adblockplus.org/29438582/diff/29466580/pages/committee/ind...
> pages/committee/index.md:29: ---
> This page was missing an <hr> so I added it.

Note: I realise that Issue #5102 exists which will remove the use of <hr>, but I
thought it made sense to add it anyway here for consistency.

[juliandoucette, 2017-06-16 18:47:53]

Thanks Ire!

This one is really close.

https://codereview.adblockplus.org/29438582/diff/29466580/pages/committee/ind...
File pages/committee/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29466580/pages/committee/ind...
pages/committee/index.md:29: ---
On 2017/06/15 11:16:23, ire wrote:
> This page was missing an <hr> so I added it. 

Acknowledged.

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
static/scss/components/_cards.scss:33: @media (max-width: $mobile-breakpoint)
NIT: I'm guessing you are using max-width to [be consistent, make the smallest
change possible]. I would actually prefer mobile-first min-width here and
elsewhere.

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
static/scss/content/_typography.scss:34: @media (max-width: $tablet-breakpoint)
NIT: Regarding all $font-size-N - N calculations: It seems these are all close
to or exactly subtracting one size unit. Why not change font-size-h1 to h2 on
tablets and h3 on mobile, and follow the same pattern for h2 and non-header
sizes when appropriate?

(I'm not sure if you tried this already and chose these exact sizes based on
that or if you considered this and thought that consistently subtracting
specific numbers was a better idea.)

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
static/scss/content/_typography.scss:162: margin-top: 0;
On 2017/06/15 11:16:23, ire wrote:
> I had to override this because it was adding extra padding where there didn't
> need to be, and making the space between the page heading and the start of the
> content seem larger

You should change components/_columns.scss instead.

[ire, 2017-06-19 11:51:04]

Thanks! I think you missed this comment/question I made, so copying it here
again:

> I've gone through the site more thoroughly and have resized the font and
spacing
> in most cases. However, in cases where the utility classes are used (e.g.
> `.m-y-lg` on the Publishers page) I haven't handled this yet. I would like to
> get your opinion on how you think we should handle this. There are two routes
we
> can take:
> 
> 1. We create an extra set of utility classes that are responsive, e.g.
> `.m-y-lg-r`. These classes will resize shrink down with smaller screen sizes.
> The major issue with this of course is that it adds even more bloat to the
site.
> However, we don't need to create a responsive size for every type of class.
For
> example, we may only want to have responsive classes for the `lg` or `xl`
sizes,
> as the classes for smaller sizes work on smaller screens anyway. 
> 
> 2. We remove the uses of the utility classes in the instances they are causing
a
> problem and style each element individually. Although this will result in less
> unused CSS, there will be a lot of repetition of styles. 
> 
> I don't have a strong preference either way, so let me know what you think.

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
static/scss/components/_cards.scss:33: @media (max-width: $mobile-breakpoint)
On 2017/06/16 18:47:53, juliandoucette wrote:
> NIT: I'm guessing you are using max-width to [be consistent, make the smallest
> change possible]. I would actually prefer mobile-first min-width here and
> elsewhere.

I would generally opt for min-width everywhere as well. However, I prefer
consistency within the project over writing it in a way I would have liked in
only some cases. Although min-width is preferable, I don't know if it's worth
causing the inconsistency here (unless you are suggesting we refactor
everything?)

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
static/scss/content/_typography.scss:34: @media (max-width: $tablet-breakpoint)
On 2017/06/16 18:47:53, juliandoucette wrote:
> NIT: Regarding all $font-size-N - N calculations: It seems these are all close
> to or exactly subtracting one size unit. Why not change font-size-h1 to h2 on
> tablets and h3 on mobile, and follow the same pattern for h2 and non-header
> sizes when appropriate?
> 
> (I'm not sure if you tried this already and chose these exact sizes based on
> that or if you considered this and thought that consistently subtracting
> specific numbers was a better idea.)

In some cases, it would work to use a smaller font size variable like you
suggest. However, for some cases, it doesn't work or is too small. For example,
a $font-size-h2 would work for the $tablet-breakpoint here, but a $font-size-h3
would be way too small for the mobile size. So I would have to do the
subtraction method anyway. 

I guess it boils down to if we would prefer to use a combination of the two
methods, or just stick to one. I think I may prefer the former; so I would use
the $font-size-h2 for the $tablet-breakpoint here and do a manual subtraction
for the mobile styles. I think it makes sense to make use of our variables
wherever we can. If you have no objection, I'll go ahead and make this change?

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
static/scss/content/_typography.scss:162: margin-top: 0;
On 2017/06/16 18:47:53, juliandoucette wrote:
> On 2017/06/15 11:16:23, ire wrote:
> > I had to override this because it was adding extra padding where there
didn't
> > need to be, and making the space between the page heading and the start of
the
> > content seem larger
> 
> You should change components/_columns.scss instead.

Acknowledged.

[juliandoucette, 2017-06-20 16:38:36]

> Thanks! I think you missed this comment/question I made, so copying it here
again:

Sorry about that - this is a tough question.

See our previous discussion about this issue here:
https://codereview.adblockplus.org/29350476/diff/29351446/static/scss/_utilit...

Ultimately, I currently disagree with myself in the past because I think this
gives the content developer too much control. I expect content developers to get
semantics right (wishful thinking? :D), but I don't currently expect them to get
responsive spacing right. As a result, my current opinion is that we should
style individual elements that differ from the site wide style individually on
the pages where they appear.

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
static/scss/components/_cards.scss:33: @media (max-width: $mobile-breakpoint)
On 2017/06/19 11:51:04, ire wrote:
> I would generally opt for min-width everywhere as well. However, I prefer
> consistency within the project over writing it in a way I would have liked in
> only some cases. Although min-width is preferable, I don't know if it's worth
> causing the inconsistency here (unless you are suggesting we refactor
> everything?)

You're right.

We should assess changing this separately (if it's worth it).

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
static/scss/content/_typography.scss:34: @media (max-width: $tablet-breakpoint)
On 2017/06/19 11:51:04, ire wrote:
> I guess it boils down to if we would prefer to use a combination of the two
> methods, or just stick to one. I think I may prefer the former; so I would use
> the $font-size-h2 for the $tablet-breakpoint here and do a manual subtraction
> for the mobile styles. I think it makes sense to make use of our variables
> wherever we can. If you have no objection, I'll go ahead and make this change?

I prefer the later actually. I think it's more consistent / easier to
understand.

[ire, 2017-06-22 15:41:44]

> Sorry about that - this is a tough question.

No worries. Yes it is!

> Ultimately, I currently disagree with myself in the past because I think this
> gives the content developer too much control. I expect content developers to
get
> semantics right (wishful thinking? :D), but I don't currently expect them to
get
> responsive spacing right. As a result, my current opinion is that we should
> style individual elements that differ from the site wide style individually on
> the pages where they appear.

Okay I've implemented it that way. 

As a side thought, I'm not sure about the idea of content developers building
pages themselves. It seems like they will need to have an understanding of
layout and CSS to not do things incorrectly.

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
File static/scss/components/_cards.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/compone...
static/scss/components/_cards.scss:33: @media (max-width: $mobile-breakpoint)
On 2017/06/20 16:38:35, juliandoucette wrote:
> We should assess changing this separately (if it's worth it).

My initial thought is that it's not worth it. Because although max-width isn't
preferable, it's also not problematic.

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29466580/static/scss/content...
static/scss/content/_typography.scss:34: @media (max-width: $tablet-breakpoint)
On 2017/06/20 16:38:36, juliandoucette wrote:
> I prefer the later actually. I think it's more consistent / easier to
> understand.

Okay, I'll leave it as it is then.

[juliandoucette, 2017-06-26 10:19:08]

The latest patch does not apply. Please update/rebase.

[juliandoucette, 2017-06-26 10:31:22]

I have just skimmed the latest patchset. Here are my first impressions.

https://codereview.adblockplus.org/29438582/diff/29471582/includes/about/styl...
File includes/about/style.html (right):

https://codereview.adblockplus.org/29438582/diff/29471582/includes/about/styl...
includes/about/style.html:1: <style>
Looks like this applies to a few pages?

- about
- solutions/*
- committee/apply
- tool-certification

If so, please move this to SCSS.

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
File includes/tool-certification/style.html (right):

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
includes/tool-certification/style.html:4: .bg-accent blockquote
.bg-accent was intended to be about background colour *only*. I suggest using
another class for this.

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
includes/tool-certification/style.html:9: .btn-primary
See comment in styles/about (it may be unnecessary to style this button's margin
individually)

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
File includes/tool-certification/summary.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
includes/tool-certification/summary.md:19: [{{sign-up-label[Sign Up button
label] Sign Up}}](https://platform.acceptableads.me/signup){: .btn-primary
.m-t-sm target="blank" }
Did you mean to remove .m-t-sm too?

https://codereview.adblockplus.org/29438582/diff/29471582/pages/blog/index.md
File pages/blog/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/blog/index.md...
pages/blog/index.md:13: .container p:last-child
Seems like there may / should be a container class for this.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/app...
File pages/committee/apply.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/app...
pages/committee/apply.md:16: .join-committee-paragraph
Does this apply to more than one element (use ID instead?) / it seems like there
may/should be a container class for this?

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/app...
pages/committee/apply.md:21: .application-form-btns
See comment in about/style.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/ind...
File pages/committee/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/ind...
pages/committee/index.md:25: .container p:last-child
Again, seems like there may / should be a container class for this.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/ind...
pages/committee/index.md:50: ---
Good catch.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/mem...
File pages/committee/members/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/mem...
pages/committee/members/index.md:13: .card .summary
Is there supposed to be a difference between .card .summary and .card-summary?

Perhaps height: auto; should be the default?

https://codereview.adblockplus.org/29438582/diff/29471582/pages/tool-certific...
File pages/tool-certification/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/tool-certific...
pages/tool-certification/index.md:26: <p class="logos m-b-na">
Looks like we could use an id for this.

https://codereview.adblockplus.org/29438582/diff/29471582/static/scss/compone...
File static/scss/components/_columns.scss (right):

https://codereview.adblockplus.org/29438582/diff/29471582/static/scss/compone...
static/scss/components/_columns.scss:45: .row p:first-child
Where/Why does this apply? (Perhaps we should comment because it's not obvious)

[ire, 2017-06-27 08:36:33]

Done :)

https://codereview.adblockplus.org/29438582/diff/29471582/includes/about/styl...
File includes/about/style.html (right):

https://codereview.adblockplus.org/29438582/diff/29471582/includes/about/styl...
includes/about/style.html:1: <style>
On 2017/06/26 10:31:20, juliandoucette wrote:
> Looks like this applies to a few pages?
> 
> - about
> - solutions/*
> - committee/apply
> - tool-certification
> 
> If so, please move this to SCSS.

Done.

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
File includes/tool-certification/style.html (right):

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
includes/tool-certification/style.html:4: .bg-accent blockquote
On 2017/06/26 10:31:20, juliandoucette wrote:
> .bg-accent was intended to be about background colour *only*. I suggest using
> another class for this.

Done.

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
includes/tool-certification/style.html:9: .btn-primary
On 2017/06/26 10:31:20, juliandoucette wrote:
> See comment in styles/about (it may be unnecessary to style this button's
margin
> individually) 

Done.

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
File includes/tool-certification/summary.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/includes/tool-certi...
includes/tool-certification/summary.md:19: [{{sign-up-label[Sign Up button
label] Sign Up}}](https://platform.acceptableads.me/signup){: .btn-primary
.m-t-sm target="blank" }
On 2017/06/26 10:31:20, juliandoucette wrote:
> Did you mean to remove .m-t-sm too?

Yes I did. Done.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/blog/index.md
File pages/blog/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/blog/index.md...
pages/blog/index.md:13: .container p:last-child
On 2017/06/26 10:31:20, juliandoucette wrote:
> Seems like there may / should be a container class for this.

Good spot. Will create one.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/app...
File pages/committee/apply.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/app...
pages/committee/apply.md:16: .join-committee-paragraph
On 2017/06/26 10:31:21, juliandoucette wrote:
> Does this apply to more than one element (use ID instead?) / it seems like
there
> may/should be a container class for this?

I went with a container class

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/app...
pages/committee/apply.md:21: .application-form-btns
On 2017/06/26 10:31:21, juliandoucette wrote:
> See comment in about/style.

Done.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/ind...
File pages/committee/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/ind...
pages/committee/index.md:25: .container p:last-child
On 2017/06/26 10:31:21, juliandoucette wrote:
> Again, seems like there may / should be a container class for this.

Done.

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/ind...
pages/committee/index.md:50: ---
On 2017/06/26 10:31:21, juliandoucette wrote:
> Good catch.

Thanks :)

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/mem...
File pages/committee/members/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/committee/mem...
pages/committee/members/index.md:13: .card .summary
On 2017/06/26 10:31:21, juliandoucette wrote:
> Is there supposed to be a difference between .card .summary and .card-summary?
> 
> Perhaps height: auto; should be the default?

You're right it should have been .card-summary, but this no longer applies since
your change yesterday. `height:auto` is the default (for smaller screens) on
.card-summary

https://codereview.adblockplus.org/29438582/diff/29471582/pages/tool-certific...
File pages/tool-certification/index.md (right):

https://codereview.adblockplus.org/29438582/diff/29471582/pages/tool-certific...
pages/tool-certification/index.md:26: <p class="logos m-b-na">
On 2017/06/26 10:31:21, juliandoucette wrote:
> Looks like we could use an id for this.

Done.

https://codereview.adblockplus.org/29438582/diff/29471582/static/scss/compone...
File static/scss/components/_columns.scss (right):

https://codereview.adblockplus.org/29438582/diff/29471582/static/scss/compone...
static/scss/components/_columns.scss:45: .row p:first-child
On 2017/06/26 10:31:22, juliandoucette wrote:
> Where/Why does this apply? (Perhaps we should comment because it's not
obvious)

Actually found a way to refactor this to work with the new paragraph container
class I created.

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
static/scss/content/_typography.scss:160: .p-container
Note: I couldn't really figure out where else to put this class. If you have any
better suggestions, let me know.

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
static/scss/content/_typography.scss:175: .p-container p:first-child
This is a change from the `.row p:first-child` selector. Let me know if a
comment is still needed.

[juliandoucette, 2017-07-03 23:12:41]

Thanks Ire :)

I'm a little tired. Please forgive me if I missed something obvious here.

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
static/scss/content/_typography.scss:160: .p-container
On 2017/06/27 08:36:31, ire wrote:
> Note: I couldn't really figure out where else to put this class. If you have
any
> better suggestions, let me know. 

I don't like the name of this class because it's too specific (it contains more
than *just* paragraphs).

I'm also confused about it's usage. It seems to do the same thing as .section
except only margin-bottom. But, at least in the case of h1, hr, [p]N, why not
just wrap the group in a .section (the header margin should collapse)?

That this class is necessary so often suggest to me that something is wrong with
our structure or defaults.

[ire, 2017-07-04 11:03:13]

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
static/scss/content/_typography.scss:160: .p-container
On 2017/07/03 23:12:38, juliandoucette wrote:
> I don't like the name of this class because it's too specific (it contains
more
> than *just* paragraphs).
> 
> I'm also confused about it's usage. It seems to do the same thing as .section
> except only margin-bottom. But, at least in the case of h1, hr, [p]N, why not
> just wrap the group in a .section (the header margin should collapse)?
> 
> That this class is necessary so often suggest to me that something is wrong
with
> our structure or defaults.

Yeah you'e right it could have a better name. 

I tried to use the .section but it wasn't quite right. What I was going for with
this class was more a container for:

1. The main pargraphs on the page. e.g. in pages/committee/apply.md, the class
is applied only around that part of the page
2. The main section of the page. e.g. in pages/solutions/publishers.md, the
class is applied around basically the whole content area of the page. 

In these cases the .section class didn't really work. But perhaps there is
another class, similar to .section we can extend? Or rename this .p-container
class with something that is more general like .section is?

[juliandoucette, 2017-07-04 15:33:08]

(Unspoken: This looks good to me; otherwise.)

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
static/scss/content/_typography.scss:160: .p-container
On 2017/07/04 11:03:11, ire wrote:
> In these cases the .section class didn't really work. But perhaps there is
> another class, similar to .section we can extend? Or rename this .p-container
> class with something that is more general like .section is?

This is what I had in mind.

But it's not clear:
- Why .section didn't work
- Whether or not .section could/should work (if modified)

I don't have a strong opinion on using .section or not. Rather, I just want
*whatever-it-is* to be straightforward. So that when I create another page I
know whether or not to use this class and why.

If you see https://acceptableads.com/en/styleguide (a hidden and depricated
page) you can get the idea about what .section was meant for at the bottom.
There were 3 different colour sections [primary, secondary, accent] and they all
had xl margin and padding and their margins would collapse if they appeared
after each other.

The class that you seem to be proposing seems something like a .content-section
or .content-container - a container (that provides spacing) meant for content
within .section-s and/or columns. But it's not entirely clear me to yet (what it
is or should be or why).

I hope that this comment is clear and I'm not repeating myself :D - thank you
for your patience.

[ire, 2017-07-05 10:33:10]

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
static/scss/content/_typography.scss:160: .p-container
On 2017/07/04 15:33:08, juliandoucette wrote:
> On 2017/07/04 11:03:11, ire wrote:
> > In these cases the .section class didn't really work. But perhaps there is
> > another class, similar to .section we can extend? Or rename this
.p-container
> > class with something that is more general like .section is?
> 
> This is what I had in mind.
> 
> But it's not clear:
> - Why .section didn't work
> - Whether or not .section could/should work (if modified)
> 
> I don't have a strong opinion on using .section or not. Rather, I just want
> *whatever-it-is* to be straightforward. So that when I create another page I
> know whether or not to use this class and why.
> 
> If you see https://acceptableads.com/en/styleguide (a hidden and depricated
> page) you can get the idea about what .section was meant for at the bottom.
> There were 3 different colour sections [primary, secondary, accent] and they
all
> had xl margin and padding and their margins would collapse if they appeared
> after each other.
> 
> The class that you seem to be proposing seems something like a
.content-section
> or .content-container - a container (that provides spacing) meant for content
> within .section-s and/or columns. But it's not entirely clear me to yet (what
it
> is or should be or why).
> 
> I hope that this comment is clear and I'm not repeating myself :D - thank you
> for your patience.

Ah I see what you mean! The .section class *should* work in this case, and I
think with some refactoring it could. In fact, the margin-top on the .section
class is almost almost collapsed, so we may be able to refactor it to only
applying a margin on the bottom without having to change too much. I'll make the
change in the next patch.

[ire, 2017-07-05 10:53:50]

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29475555/static/scss/content...
static/scss/content/_typography.scss:160: .p-container
On 2017/07/05 10:33:09, ire wrote:
> On 2017/07/04 15:33:08, juliandoucette wrote:
> > On 2017/07/04 11:03:11, ire wrote:
> > > In these cases the .section class didn't really work. But perhaps there is
> > > another class, similar to .section we can extend? Or rename this
> .p-container
> > > class with something that is more general like .section is?
> > 
> > This is what I had in mind.
> > 
> > But it's not clear:
> > - Why .section didn't work
> > - Whether or not .section could/should work (if modified)
> > 
> > I don't have a strong opinion on using .section or not. Rather, I just want
> > *whatever-it-is* to be straightforward. So that when I create another page I
> > know whether or not to use this class and why.
> > 
> > If you see https://acceptableads.com/en/styleguide (a hidden and depricated
> > page) you can get the idea about what .section was meant for at the bottom.
> > There were 3 different colour sections [primary, secondary, accent] and they
> all
> > had xl margin and padding and their margins would collapse if they appeared
> > after each other.
> > 
> > The class that you seem to be proposing seems something like a
> .content-section
> > or .content-container - a container (that provides spacing) meant for
content
> > within .section-s and/or columns. But it's not entirely clear me to yet
(what
> it
> > is or should be or why).
> > 
> > I hope that this comment is clear and I'm not repeating myself :D - thank
you
> > for your patience.
> 
> Ah I see what you mean! The .section class *should* work in this case, and I
> think with some refactoring it could. In fact, the margin-top on the .section
> class is almost almost collapsed, so we may be able to refactor it to only
> applying a margin on the bottom without having to change too much. I'll make
the
> change in the next patch. 

Done.

[juliandoucette, 2017-07-05 13:28:34]

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
File static/scss/content/_reset.scss (left):

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
static/scss/content/_reset.scss:89: h2 + .row,
I was wondering why there was less spacing after hr in production... I see now
(margins don't collapse above/below rows of columns).

e.g.

h1
hr
p

Looks different than

h1
hr
row
col
p
col
p

I can't think of a perfect way around this off of the top of my head because I
think that I can't predict what will be above/below and inside of rows/columns
(the margins inside and out could be different depending on that) therefore
killing the margin-top on the :first-child and margin-bottom on the :last-child
would not be the same as collapsing above/below (unless the above/below margins
were always larger than those inside).

Leaving this as-is in the latest patchset will cause the heading margin to look
different across pages depending on whether there are rows/columns below (e.g.
compare http://localhost:5000/en/about to
http://localhost:5000/en/about/criteria). But I am willing to allow this for now
and consider this a worthy website-defaults issue.

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
File static/scss/content/_typography.scss (left):

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
static/scss/content/_typography.scss:26: h1,
I think the change to this header margin is miss-aligning the hr on mobile.

https://drive.google.com/open?id=0B9dIRGeSpmZ_SkdXUUt4b2VIYmM

I think the hr should be centered in the space below.

There is room for interpretation here (in the design).

[ire, 2017-07-06 10:31:39]

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
File static/scss/content/_reset.scss (left):

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
static/scss/content/_reset.scss:89: h2 + .row,
On 2017/07/05 13:28:33, juliandoucette wrote:
> I was wondering why there was less spacing after hr in production... I see now
> (margins don't collapse above/below rows of columns).
> 
> e.g.
> 
> h1
> hr
> p
> 
> Looks different than
> 
> h1
> hr
> row
> col
> p
> col
> p
> 
> I can't think of a perfect way around this off of the top of my head because I
> think that I can't predict what will be above/below and inside of rows/columns
> (the margins inside and out could be different depending on that) therefore
> killing the margin-top on the :first-child and margin-bottom on the
:last-child
> would not be the same as collapsing above/below (unless the above/below
margins
> were always larger than those inside).
> 
> Leaving this as-is in the latest patchset will cause the heading margin to
look
> different across pages depending on whether there are rows/columns below (e.g.
> compare http://localhost:5000/en/about to
> http://localhost:5000/en/about/criteria). But I am willing to allow this for
now
> and consider this a worthy website-defaults issue.

I've added this style back.

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
File static/scss/content/_typography.scss (left):

https://codereview.adblockplus.org/29438582/diff/29480584/static/scss/content...
static/scss/content/_typography.scss:26: h1,
On 2017/07/05 13:28:33, juliandoucette wrote:
> I think the change to this header margin is miss-aligning the hr on mobile.
> 
> https://drive.google.com/open?id=0B9dIRGeSpmZ_SkdXUUt4b2VIYmM
> 
> I think the hr should be centered in the space below.
> 
> There is room for interpretation here (in the design).

Done. I had initially thought of it as an underline, but on second look at it on
desktop you're right it's more of a separator between the heading and content.  

Note: The alignment is very slightly different across the pages due to the
differing spacing we discussed in the other comment (e.g.
http://localhost:5000/en/about vs http://localhost:5000/en/about/criteria). But
I think the difference is slight enough to not be an issue.

https://codereview.adblockplus.org/29438582/diff/29481594/static/scss/content...
File static/scss/content/_reset.scss (right):

https://codereview.adblockplus.org/29438582/diff/29481594/static/scss/content...
static/scss/content/_reset.scss:89: // Simulate collapsable top-margin on first
paragraph after page heading
I added this comment here until we decide to revisit this issue so it's clear
why the style is here.

https://codereview.adblockplus.org/29438582/diff/29481594/static/scss/content...
File static/scss/content/_typography.scss (right):

https://codereview.adblockplus.org/29438582/diff/29481594/static/scss/content...
static/scss/content/_typography.scss:438: margin-top: -$sm - 5px;
The $xs size was every so slightly off

[juliandoucette, 2017-07-06 13:15:49]

LGTM

https://codereview.adblockplus.org/29438582/diff/29481594/static/scss/content...
File static/scss/content/_reset.scss (right):

https://codereview.adblockplus.org/29438582/diff/29481594/static/scss/content...
static/scss/content/_reset.scss:89: // Simulate collapsable top-margin on first
paragraph after page heading
On 2017/07/06 10:31:38, ire wrote:
> I added this comment here until we decide to revisit this issue so it's clear
> why the style is here. 

Acknowledged.