Issue 5511 - Create Product Help Home Template for help.eyeo.com

Issue 5511 - Create Product Help Home Template for help.eyeo.com

[ire, 2017-08-15 15:09:22]

First go. I left out the Contact Section that would go at the bottom of the
page. I'll create a separate Issue for it.

https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_sl...
File globals/get_page_slug.py (right):

https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_sl...
globals/get_page_slug.py:17: def get_page_slug(url):
Here I was trying to get just the unique slug part of a url. Wasn't able to find
a method that already exists to do this. Is there one?

https://codereview.adblockplus.org/29516622/diff/29516623/pages/adblockplus/a...
File pages/adblockplus/adblock-plus-breaks-the-websites-i-visit.md (right):

https://codereview.adblockplus.org/29516622/diff/29516623/pages/adblockplus/a...
pages/adblockplus/adblock-plus-breaks-the-websites-i-visit.md:1: title=Adblock
Plus breaks the websites I visit
I needed some articles for the homepage to work so I added a few blank pages.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
File static/scss/layout/_grid.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_grid.scss:19: @media (min-width: $tablet-breakpoint)
in website defaults, this class doesn't become half-width until
$tablet-breakpoint. I think this is too large

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
File templates/product-home.tmpl (right):

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
templates/product-home.tmpl:13: <section class="card {{product_id}}-card
section">
I could make this a macro if it's easier to read/understand. Same with the help
categories below.

[juliandoucette, 2017-08-24 11:14:01]

Here are my first impressions.

https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_sl...
File globals/get_page_slug.py (right):

https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_sl...
globals/get_page_slug.py:17: def get_page_slug(url):
On 2017/08/15 15:09:22, ire wrote:
> Here I was trying to get just the unique slug part of a url. Wasn't able to
find
> a method that already exists to do this. Is there one?

Yes it's the page's "page" property.

https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py...
globals/products.py:21: "help_categories": [
I think that we want to avoid hard coding things like this if possible.

e.g. Instead of looping products[product]['help_categories'] we could loop
get_pages_metadata({product: product}) sorted by category (by sorting it
ourselves or requesting that Sitescripts adds a sort feature to
get_pages_metadata).

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
File static/scss/components/_lists.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:24: .underlined-list li a
NIT: I think that the line should be under the <li> because the <a> may have
text-decoration (now or later) and it seems more modular to me. This is a little
subjective; obviously :/ . You may disagree.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:27: width: 100%;
NIT: Is this necessary because display:block?

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:39: .underlined-list .one-half:nth-child(1) a
Why is this necessary?

The same applies below.

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

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/content...
static/scss/content/_typography.scss:26: .h1
Note: Maybe we should make these configurable via variables in website-defaults
*thinking out loud*

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/content...
static/scss/content/_typography.scss:51: height: 0.9em;
NIT: My spidy-senses are telling me that 90% may be clearer. But this may be
misguided because ems are relative.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_body.scss:20: background-image: url("/img/png/help-bg.png");
NIT: Apparently our code style guide suggests no quotes here (this is not my
personal preference)

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_body.scss:24: .main
NIT: Isn't there usually only one <main> or #main? In that case, perhaps we
should considering using an element or ID selector.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_body.scss:35: .wrapper
NIT: If you agree with my comment about .nav-wrapper in the header than it may
make more sense to use a _base.css .container as a parent and then set the width
of #main or <main> instead.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
File static/scss/layout/_grid.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_grid.scss:19: @media (min-width: $tablet-breakpoint)
On 2017/08/15 15:09:22, ire wrote:
> in website defaults, this class doesn't become half-width until
> $tablet-breakpoint. I think this is too large

Doesn't this code do the same?

I considered this when I created website-default grid (because I agree with
you). Certainly, in some cases, we may want half-width on phablets ( 544 < 768
). I would suggest using a modifier e.g. `.force-half` or a special component
e.g. A custom group or card deck may scale differently then vanilla grid columns
-> instead of overwriting website-defaults _grid.scss. If we change the way a
standard component works on a specific website then we will have to document
that and keep it in mind when maintaining that website.

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
File templates/product-home.tmpl (right):

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
templates/product-home.tmpl:5: <h1 class="h1 section ta-center">
NIT: It seems confusing that an <h1> is a .section?

[juliandoucette, 2017-08-24 11:15:09]

(I forgot to mention that this Patchset does not apply in it's current form.)

[ire, 2017-08-28 14:54:50]

> (I forgot to mention that this Patchset does not apply in it's current
> form.)

Sorry I don't think I mentioned that this was applied on top of the Site Header
Component review [this
one](https://codereview.adblockplus.org/29485575/#ps29511640).

When that review is done I will rebase on top of it.

https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_sl...
File globals/get_page_slug.py (right):

https://codereview.adblockplus.org/29516622/diff/29516623/globals/get_page_sl...
globals/get_page_slug.py:17: def get_page_slug(url):
On 2017/08/24 11:13:59, juliandoucette wrote:
> On 2017/08/15 15:09:22, ire wrote:
> > Here I was trying to get just the unique slug part of a url. Wasn't able to
> find
> > a method that already exists to do this. Is there one?
> 
> Yes it's the page's "page" property.

That includes the whole path (not including the hostname), what I'm looking for
is just the part for the particular page, not parent pages as well

https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py...
globals/products.py:21: "help_categories": [
On 2017/08/24 11:13:59, juliandoucette wrote:
> I think that we want to avoid hard coding things like this if possible.
> 
> e.g. Instead of looping products[product]['help_categories'] we could loop
> get_pages_metadata({product: product}) sorted by category (by sorting it
> ourselves or requesting that Sitescripts adds a sort feature to
> get_pages_metadata).

I agree with you, I think this should be included in get_pages_metadata

I'm unsure how to deal with the icons though. If we don't have a single
reference for them being tied to categories here, we wouldn't be able to just
loop through the categories like we are doing now. Any ideas?

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
File static/scss/components/_lists.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:24: .underlined-list li a
On 2017/08/24 11:14:00, juliandoucette wrote:
> NIT: I think that the line should be under the <li> because the <a> may have
> text-decoration (now or later) and it seems more modular to me. This is a
little
> subjective; obviously :/ . You may disagree.

I agree, I initially did it this way actually, but it didn't really work out
because the <li> has the `.one-half` class, which applies padding to both sides
to create the space. If the <li> was underlined, there would be no spacing
between each of the items.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:27: width: 100%;
On 2017/08/24 11:14:00, juliandoucette wrote:
> NIT: Is this necessary because display:block?

Done.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:39: .underlined-list .one-half:nth-child(1) a
On 2017/08/24 11:13:59, juliandoucette wrote:
> Why is this necessary?
> 
> The same applies below.

All of the list items typically have a border-bottom. With <li> items like the
popular topics one that are displayed in two columns, the first item has a
border-top as well. The media queries are because, the columns collapse into one
on smaller screens, so only the first list item needs the border-top until a
certain viewport width

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

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/content...
static/scss/content/_typography.scss:26: .h1
On 2017/08/24 11:14:00, juliandoucette wrote:
> Note: Maybe we should make these configurable via variables in
website-defaults
> *thinking out loud*

I like the idea

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/content...
static/scss/content/_typography.scss:51: height: 0.9em;
On 2017/08/24 11:14:00, juliandoucette wrote:
> NIT: My spidy-senses are telling me that 90% may be clearer. But this may be
> misguided because ems are relative.

90% makes the icon really small. I can't figure out what it is 90% of though :/

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_body.scss:20: background-image: url("/img/png/help-bg.png");
On 2017/08/24 11:14:01, juliandoucette wrote:
> NIT: Apparently our code style guide suggests no quotes here (this is not my
> personal preference)

Oh okay. I thought it was the other way around, I even mentioned it in a review
of Manvel's code haha

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_body.scss:24: .main
On 2017/08/24 11:14:00, juliandoucette wrote:
> NIT: Isn't there usually only one <main> or #main? In that case, perhaps we
> should considering using an element or ID selector.

Changed to #main

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_body.scss:35: .wrapper
On 2017/08/24 11:14:01, juliandoucette wrote:
> NIT: If you agree with my comment about .nav-wrapper in the header than it may
> make more sense to use a _base.css .container as a parent and then set the
width
> of #main or <main> instead.

Okay, I think it makes sense to update this when the review for the header is
set

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
File static/scss/layout/_grid.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_grid.scss:19: @media (min-width: $tablet-breakpoint)
On 2017/08/24 11:14:01, juliandoucette wrote:
> On 2017/08/15 15:09:22, ire wrote:
> > in website defaults, this class doesn't become half-width until
> > $tablet-breakpoint. I think this is too large
> 
> Doesn't this code do the same?

Oops I meant to say it doesn't become half-width until $desktop-breakpoint

> I considered this when I created website-default grid (because I agree with
> you). Certainly, in some cases, we may want half-width on phablets ( 544 < 768
> ). I would suggest using a modifier e.g. `.force-half` or a special component
> e.g. A custom group or card deck may scale differently then vanilla grid
columns
> -> instead of overwriting website-defaults _grid.scss. If we change the way a
> standard component works on a specific website then we will have to document
> that and keep it in mind when maintaining that website.

I agree with you about not overriding the standard component. 

I'm struggling with the modifier class name `.force-half`, because I don't think
it really describes what we are trying to achieve here. This is basically the
same grid, but having the breakpoints happen at a smaller viewport. The way I
see it, if I'm using a `.col-half`class, then that means I would want the column
to take up half the space for the majority of viewport sizes possible (same with
the other classes). But the way it is currently set up, it's only on larger
desktops that this would happen. For a significant amount of viewport sizes, the
column would be full width. I actually think the case in which a col-half should
only become half at desktop is more the exception, than the rule.

That said, perhaps we could have an additional version of the grid which breaks
at $tablet-breakpoint instead of $desktop-breakpoint. The modifier class could
be applied to the `.row`, something like `.row.tablet-break` (Not an actual
suggestion, I can't think of a good classname right now). What do you think?

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
File templates/product-home.tmpl (right):

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
templates/product-home.tmpl:5: <h1 class="h1 section ta-center">
On 2017/08/24 11:14:01, juliandoucette wrote:
> NIT: It seems confusing that an <h1> is a .section?

I see what you mean, but if I move the class to a container that may seem to be
unnecessary markup

[ire, 2017-09-11 14:47:29]

Rebased

[juliandoucette, 2017-09-12 13:22:00]

Looking good. Mostly NITs left. Thanks Ire!

https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29516623/globals/products.py...
globals/products.py:21: "help_categories": [
On 2017/08/28 14:54:47, ire wrote:
> On 2017/08/24 11:13:59, juliandoucette wrote:
> > I think that we want to avoid hard coding things like this if possible.
> > 
> > e.g. Instead of looping products[product]['help_categories'] we could loop
> > get_pages_metadata({product: product}) sorted by category (by sorting it
> > ourselves or requesting that Sitescripts adds a sort feature to
> > get_pages_metadata).
> 
> I agree with you, I think this should be included in get_pages_metadata
> 
> I'm unsure how to deal with the icons though. If we don't have a single
> reference for them being tied to categories here, we wouldn't be able to just
> loop through the categories like we are doing now. Any ideas?

See my comment in the latest patchset.

https://codereview.adblockplus.org/29516622/diff/29516623/pages/adblockplus/a...
File pages/adblockplus/adblock-plus-breaks-the-websites-i-visit.md (right):

https://codereview.adblockplus.org/29516622/diff/29516623/pages/adblockplus/a...
pages/adblockplus/adblock-plus-breaks-the-websites-i-visit.md:1: title=Adblock
Plus breaks the websites I visit
On 2017/08/15 15:09:22, ire wrote:
> I needed some articles for the homepage to work so I added a few blank pages.

Acknowledged.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
File static/scss/components/_lists.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:24: .underlined-list li a
On 2017/08/28 14:54:48, ire wrote:
> On 2017/08/24 11:14:00, juliandoucette wrote:
> > NIT: I think that the line should be under the <li> because the <a> may have
> > text-decoration (now or later) and it seems more modular to me. This is a
> little
> > subjective; obviously :/ . You may disagree.
> 
> I agree, I initially did it this way actually, but it didn't really work out
> because the <li> has the `.one-half` class, which applies padding to both
sides
> to create the space. If the <li> was underlined, there would be no spacing
> between each of the items. 

Acknowledged.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:39: .underlined-list .one-half:nth-child(1) a
On 2017/08/28 14:54:48, ire wrote:
> On 2017/08/24 11:13:59, juliandoucette wrote:
> > Why is this necessary?
> > 
> > The same applies below.
> 
> All of the list items typically have a border-bottom. With <li> items like the
> popular topics one that are displayed in two columns, the first item has a
> border-top as well. The media queries are because, the columns collapse into
one
> on smaller screens, so only the first list item needs the border-top until a
> certain viewport width

Acknowledged.

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

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/content...
static/scss/content/_typography.scss:51: height: 0.9em;
On 2017/08/28 14:54:48, ire wrote:
> On 2017/08/24 11:14:00, juliandoucette wrote:
> > NIT: My spidy-senses are telling me that 90% may be clearer. But this may be
> > misguided because ems are relative.
> 
> 90% makes the icon really small. I can't figure out what it is 90% of though
:/

Acknowledged. % height didn't work when I tested just now. I think we can ignore
this comment for now anyway.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_body.scss:20: background-image: url("/img/png/help-bg.png");
On 2017/08/28 14:54:49, ire wrote:
> Oh okay. I thought it was the other way around, I even mentioned it in a
review
> of Manvel's code haha

That's probably my fault :D (or, at least, you can blame me if you want.)

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
File static/scss/layout/_grid.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/layout/...
static/scss/layout/_grid.scss:19: @media (min-width: $tablet-breakpoint)
On 2017/08/28 14:54:49, ire wrote:
> Oops I meant to say it doesn't become half-width until $desktop-breakpoint
> 
> > I considered this when I created website-default grid (because I agree with
> > you). Certainly, in some cases, we may want half-width on phablets ( 544 <
768
> > ). I would suggest using a modifier e.g. `.force-half` or a special
component
> > e.g. A custom group or card deck may scale differently then vanilla grid
> columns
> > -> instead of overwriting website-defaults _grid.scss. If we change the way
a
> > standard component works on a specific website then we will have to document
> > that and keep it in mind when maintaining that website.
> 
> I agree with you about not overriding the standard component. 
> 
> I'm struggling with the modifier class name `.force-half`, because I don't
think
> it really describes what we are trying to achieve here. This is basically the
> same grid, but having the breakpoints happen at a smaller viewport. The way I
> see it, if I'm using a `.col-half`class, then that means I would want the
column
> to take up half the space for the majority of viewport sizes possible (same
with
> the other classes). But the way it is currently set up, it's only on larger
> desktops that this would happen. For a significant amount of viewport sizes,
the
> column would be full width. I actually think the case in which a col-half
should
> only become half at desktop is more the exception, than the rule.
> 
> That said, perhaps we could have an additional version of the grid which
breaks
> at $tablet-breakpoint instead of $desktop-breakpoint. The modifier class could
> be applied to the `.row`, something like `.row.tablet-break` (Not an actual
> suggestion, I can't think of a good classname right now). What do you think?

See my comment in the latest Patchset. (Note: Maybe I changed this by accident
in website-defaults? Or one or both of us are or were confused. :D)

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
File templates/product-home.tmpl (right):

https://codereview.adblockplus.org/29516622/diff/29516623/templates/product-h...
templates/product-home.tmpl:5: <h1 class="h1 section ta-center">
On 2017/08/28 14:54:49, ire wrote:
> I see what you mean, but if I move the class to a container that may seem to
be
> unnecessary markup

See my comment in the latest Patchset.

https://codereview.adblockplus.org/29516622/diff/29541770/globals/get_page_sl...
File globals/get_page_slug.py (right):

https://codereview.adblockplus.org/29516622/diff/29541770/globals/get_page_sl...
globals/get_page_slug.py:18: 
NIT: I think that the name get_page_slug implies that there is some
"sluggifying" going on in this function. May I suggest get_page_name?

(While Acknowledging that it's kindof confusing that the "page" property is a
path. Perhaps we should create an issue about this.)

https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py...
globals/products.py:21: "help_categories": [
Note: These could be auto-generate via category property in pages provided that
the name/slug/icon format do not differ in contents/format. Unfortunately this
couldn't be done in a global because globals don't have access to other globals.
Therefore, your implementation, although (arguably) not optimal, is the best we
can currently do; I think.

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/base/_u...
static/scss/base/_utilities.scss:90: // Typography
///////////////////////////////////////////////////////////////
Note: Please add .text-align-center (or equivalent) to website-defaults instead
(Sorry if I said this before) (this is non-blocking)

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
File static/scss/components/_card.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
static/scss/components/_card.scss:24: border-top-style: solid;
NIT: The border-style is already set to solid above?

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
static/scss/components/_card.scss:27: background-color: $white;
Suggest: $primary-light (This may be a bigger issue best dropped or addressed
separately)

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

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/content...
static/scss/content/_typography.scss:19: font-size: $font-size-h3;
Note: It's a little confusing that we are setting h-size classes to different
h-sizes at different widths. This may suggest that we should use less specific
font-size variable names. (Thinking out loud.)

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
static/scss/layout/_body.scss:34: .wrapper
You seem to be re-writing the .container class in website-defaults _base.scss?

The existing differences are:

- You chose $md padding instead of $small-space (which are roughly equivalent,
except $small-space is arguably more accessible)
- You didn't resize this wrapper to device widths (I think that this resizing is
a good idea because it creates fewer test (and by-extension) edge cases. If the
design insists otherwise then I would suggest that we simply make this device
width resizing configurable in website-defaults.)

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
File static/scss/layout/_grid.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
static/scss/layout/_grid.scss:17: .one-half
NIT: This is configurable now and the value below is the default behavior.

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
File templates/product-home.tmpl (right):

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:5: <h1 class="h1 section ta-center">
NIT: I think it's confusing to add a class named "section" to a header because a
header logically belongs to a "section". I think that this indicates that this
class should be named less specifically or this  heading should belong to a
component that styles headings differently. I'm leaning towards the latter e.g.
.product-heading

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:6: <img class="heading-icon"
src="/img/png/logo-{{product_id}}.png" srcset="/img/svg/logo-{{product_id}}.svg
2x" alt="{{ products[product_id].full_name+" Logo" | translate(
product_id+"-logo-alt", "Image alt text") }}">
NIT: I prefer string interpolation in python e.g. '%s-logo-alt' % product_id

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:8: {{ products[product_id].full_name + " Help
Center" | translate(product_id+"-help-home-title", "Page title") }}
NIT: I suggest you {% set product = products[product_id] %} so that you can {{
product.full_name }} (or equivalent)

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:14: <h2 class="h4 card-heading">
NIT: If all cards of the same type have the same font-size then it doesn't make
sense to set their font-size using a separate class?

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:20: <ul class="underlined-list row">
Note: Clever use of row and columns here :)

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:23: <a href="{{article["page"]}}">
NIT: Inconsistent use of space in {{ }}

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:34: {% set articles = get_pages_metadata({
"product_id": product_id, "template": "article", "category": category.name }) %}
NIT: I think that it's unnecessary to test against the template key/value here?

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:36: <section class="section column one-half">
Note: If you agree with my comment above about manually setting the font-size of
a card then it may logically follow to componentize this section. This section
is basically the inside of a card like the one above. Which suggests that we
could use the same single class for both.

[juliandoucette, 2017-09-12 22:53:56]

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

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/content...
static/scss/content/_typography.scss:19: font-size: $font-size-h3;
On 2017/09/12 13:21:57, juliandoucette wrote:
> Note: It's a little confusing that we are setting h-size classes to different
> h-sizes at different widths. This may suggest that we should use less specific
> font-size variable names. (Thinking out loud.)

* I'm having doubts about this comment. Feel free to ignore it if you like. It's
not blocking anyway. *

[ire, 2017-09-19 10:27:24]

> Looking good. Mostly NITs left. Thanks Ire!

Yay :)

https://codereview.adblockplus.org/29516622/diff/29541770/globals/get_page_sl...
File globals/get_page_slug.py (right):

https://codereview.adblockplus.org/29516622/diff/29541770/globals/get_page_sl...
globals/get_page_slug.py:18: 
On 2017/09/12 13:21:55, juliandoucette wrote:
> NIT: I think that the name get_page_slug implies that there is some
> "sluggifying" going on in this function. May I suggest get_page_name?

Done.

> (While Acknowledging that it's kindof confusing that the "page" property is a
> path. Perhaps we should create an issue about this.)

Agreed.

https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py...
globals/products.py:21: "help_categories": [
On 2017/09/12 13:21:56, juliandoucette wrote:
> Note: These could be auto-generate via category property in pages provided
that
> the name/slug/icon format do not differ in contents/format. Unfortunately this
> couldn't be done in a global because globals don't have access to other
globals.
> Therefore, your implementation, although (arguably) not optimal, is the best
we
> can currently do; I think.

Yes I thought about standardizing the names of the icons as well, then got a bit
stuck when it came to the ones that have "&", which would require us to probably
write a custom function to "slugify" the name. I think this is something we can
still look into, but perhaps in a separate issue.

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/base/_u...
static/scss/base/_utilities.scss:90: // Typography
///////////////////////////////////////////////////////////////
On 2017/09/12 13:21:56, juliandoucette wrote:
> Note: Please add .text-align-center (or equivalent) to website-defaults
instead
> (Sorry if I said this before) (this is non-blocking)

I will create an issue to transfer some of these utility classes to
website-defaults

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
File static/scss/components/_card.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
static/scss/components/_card.scss:24: border-top-style: solid;
On 2017/09/12 13:21:57, juliandoucette wrote:
> NIT: The border-style is already set to solid above?

Done. Not sure why I added this :/

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
static/scss/components/_card.scss:27: background-color: $white;
On 2017/09/12 13:21:57, juliandoucette wrote:
> Suggest: $primary-light (This may be a bigger issue best dropped or addressed
> separately)

Yeah I think this is something we can address later. Because I do actually mean
for this to be white.

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

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/content...
static/scss/content/_typography.scss:19: font-size: $font-size-h3;
On 2017/09/12 22:53:55, juliandoucette wrote:
> On 2017/09/12 13:21:57, juliandoucette wrote:
> > Note: It's a little confusing that we are setting h-size classes to
different
> > h-sizes at different widths. This may suggest that we should use less
specific
> > font-size variable names. (Thinking out loud.)
> 
> * I'm having doubts about this comment. Feel free to ignore it if you like.
It's
> not blocking anyway. *

I think it's something we can think about for a separate issue.

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
static/scss/layout/_body.scss:34: .wrapper
On 2017/09/12 13:21:57, juliandoucette wrote:
> You seem to be re-writing the .container class in website-defaults _base.scss?
> 
> The existing differences are:
> 
> - You chose $md padding instead of $small-space (which are roughly equivalent,
> except $small-space is arguably more accessible)

Noted. No argument there.

> - You didn't resize this wrapper to device widths (I think that this resizing
is
> a good idea because it creates fewer test (and by-extension) edge cases. If
the
> design insists otherwise then I would suggest that we simply make this device
> width resizing configurable in website-defaults.)

I believe we discussed this before but I can't seem to find the comments
anywhere :/. My argument was basically that I prefer the continually responsive
look, than restricting the content to certain widths. You are correct that the
current .container method creates fewer test/edge cases, but I think (which is a
totally personal opinion) that my method just *looks* better.

That said, I think the best way forward is for the .container class to be used
here, and if I can come up with a convincing enough argument, we can discuss
changing the way the class works in website-defaults (or configuring it like you
mentioned). 

Also, I don't think the design specifies one way or the other (unless we ask
Jeen).

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
File static/scss/layout/_grid.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
static/scss/layout/_grid.scss:17: .one-half
On 2017/09/12 13:21:58, juliandoucette wrote:
> NIT: This is configurable now and the value below is the default behavior.

Done.

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
File templates/product-home.tmpl (right):

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:5: <h1 class="h1 section ta-center">
On 2017/09/12 13:21:58, juliandoucette wrote:
> NIT: I think it's confusing to add a class named "section" to a header because
a
> header logically belongs to a "section". I think that this indicates that this
> class should be named less specifically or this  heading should belong to a
> component that styles headings differently. I'm leaning towards the latter
e.g.
> .product-heading

Okay, I like the .product-heading way. Done.

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:6: <img class="heading-icon"
src="/img/png/logo-{{product_id}}.png" srcset="/img/svg/logo-{{product_id}}.svg
2x" alt="{{ products[product_id].full_name+" Logo" | translate(
product_id+"-logo-alt", "Image alt text") }}">
On 2017/09/12 13:21:59, juliandoucette wrote:
> NIT: I prefer string interpolation in python e.g. '%s-logo-alt' % product_id

I actually find it more confusing, particularly because we're not using it
everywhere (e.h. the image urls)

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:8: {{ products[product_id].full_name + " Help
Center" | translate(product_id+"-help-home-title", "Page title") }}
On 2017/09/12 13:21:58, juliandoucette wrote:
> NIT: I suggest you {% set product = products[product_id] %} so that you can {{
> product.full_name }} (or equivalent) 

Good idea. Done.

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:14: <h2 class="h4 card-heading">
On 2017/09/12 13:21:59, juliandoucette wrote:
> NIT: If all cards of the same type have the same font-size then it doesn't
make
> sense to set their font-size using a separate class?

Done.

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:20: <ul class="underlined-list row">
On 2017/09/12 13:21:59, juliandoucette wrote:
> Note: Clever use of row and columns here :)

Thanks :)

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:34: {% set articles = get_pages_metadata({
"product_id": product_id, "template": "article", "category": category.name }) %}
On 2017/09/12 13:21:58, juliandoucette wrote:
> NIT: I think that it's unnecessary to test against the template key/value
here?

Done.

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:36: <section class="section column one-half">
On 2017/09/12 13:21:58, juliandoucette wrote:
> Note: If you agree with my comment above about manually setting the font-size
of
> a card then it may logically follow to componentize this section. This section
> is basically the inside of a card like the one above. Which suggests that we
> could use the same single class for both.

I agree about the font-size, but in the case it is a bit unclear where the
"component" should go. Because of where I am applying the columns, I can't use
the same structure for both sections.

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

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/content...
static/scss/content/_typography.scss:56: @extend .ta-center;
Do we have anything against using @extend rules?

[juliandoucette, 2017-09-22 13:29:37]

Responses before review.

https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29541770/globals/products.py...
globals/products.py:21: "help_categories": [
On 2017/09/19 10:27:18, ire wrote:
> On 2017/09/12 13:21:56, juliandoucette wrote:
> > Note: These could be auto-generate via category property in pages provided
> that
> > the name/slug/icon format do not differ in contents/format. Unfortunately
this
> > couldn't be done in a global because globals don't have access to other
> globals.
> > Therefore, your implementation, although (arguably) not optimal, is the best
> we
> > can currently do; I think.
> 
> Yes I thought about standardizing the names of the icons as well, then got a
bit
> stuck when it came to the ones that have "&", which would require us to
probably
> write a custom function to "slugify" the name. I think this is something we
can
> still look into, but perhaps in a separate issue.

Acknowledged.

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/base/_u...
File static/scss/base/_utilities.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/base/_u...
static/scss/base/_utilities.scss:90: // Typography
///////////////////////////////////////////////////////////////
On 2017/09/19 10:27:18, ire wrote:
> On 2017/09/12 13:21:56, juliandoucette wrote:
> > Note: Please add .text-align-center (or equivalent) to website-defaults
> instead
> > (Sorry if I said this before) (this is non-blocking)
> 
> I will create an issue to transfer some of these utility classes to
> website-defaults

Acknowledged.

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
File static/scss/components/_card.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/compone...
static/scss/components/_card.scss:27: background-color: $white;
On 2017/09/19 10:27:19, ire wrote:
> On 2017/09/12 13:21:57, juliandoucette wrote:
> > Suggest: $primary-light (This may be a bigger issue best dropped or
addressed
> > separately)
> 
> Yeah I think this is something we can address later. Because I do actually
mean
> for this to be white.

Acknowledged.

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

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/content...
static/scss/content/_typography.scss:19: font-size: $font-size-h3;
On 2017/09/19 10:27:19, ire wrote:
> On 2017/09/12 22:53:55, juliandoucette wrote:
> > On 2017/09/12 13:21:57, juliandoucette wrote:
> > > Note: It's a little confusing that we are setting h-size classes to
> different
> > > h-sizes at different widths. This may suggest that we should use less
> specific
> > > font-size variable names. (Thinking out loud.)
> > 
> > * I'm having doubts about this comment. Feel free to ignore it if you like.
> It's
> > not blocking anyway. *
> 
> I think it's something we can think about for a separate issue.

Acknowledged.

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29541770/static/scss/layout/...
static/scss/layout/_body.scss:34: .wrapper
On 2017/09/19 10:27:19, ire wrote:
> On 2017/09/12 13:21:57, juliandoucette wrote:
> > You seem to be re-writing the .container class in website-defaults
_base.scss?
> > 
> > The existing differences are:
> > 
> > - You chose $md padding instead of $small-space (which are roughly
equivalent,
> > except $small-space is arguably more accessible)
> 
> Noted. No argument there.
> 
> > - You didn't resize this wrapper to device widths (I think that this
resizing
> is
> > a good idea because it creates fewer test (and by-extension) edge cases. If
> the
> > design insists otherwise then I would suggest that we simply make this
device
> > width resizing configurable in website-defaults.)
> 
> I believe we discussed this before but I can't seem to find the comments
> anywhere :/. My argument was basically that I prefer the continually
responsive
> look, than restricting the content to certain widths. You are correct that the
> current .container method creates fewer test/edge cases, but I think (which is
a
> totally personal opinion) that my method just *looks* better.
> 
> That said, I think the best way forward is for the .container class to be used
> here, and if I can come up with a convincing enough argument, we can discuss
> changing the way the class works in website-defaults (or configuring it like
you
> mentioned). 
> 
> Also, I don't think the design specifies one way or the other (unless we ask
> Jeen).

Acknowledged. And I agree that we should address this separately.

Background:

I tend to agree with your taste regarding fluid vs fixed width responsive
layouts. Besides the obvious (and huge, if you are through) QA benefits (testing
every px of resize): The hard problem (for me) has been trying to limit the size
of internationalized text in 3+ column fluid layouts between mobile, phablet,
and tablet sizes (Acknowledging that not all layouts have this problem and that
fixed widths are not a cure to this illness).

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
File templates/product-home.tmpl (right):

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:6: <img class="heading-icon"
src="/img/png/logo-{{product_id}}.png" srcset="/img/svg/logo-{{product_id}}.svg
2x" alt="{{ products[product_id].full_name+" Logo" | translate(
product_id+"-logo-alt", "Image alt text") }}">
On 2017/09/19 10:27:22, ire wrote:
> On 2017/09/12 13:21:59, juliandoucette wrote:
> > NIT: I prefer string interpolation in python e.g. '%s-logo-alt' % product_id
> 
> I actually find it more confusing, particularly because we're not using it
> everywhere (e.h. the image urls)

Acknowledged.

Please fix the spacing then.

https://codereview.adblockplus.org/29516622/diff/29541770/templates/product-h...
templates/product-home.tmpl:36: <section class="section column one-half">
On 2017/09/19 10:27:20, ire wrote:
> On 2017/09/12 13:21:58, juliandoucette wrote:
> > Note: If you agree with my comment above about manually setting the
font-size
> of
> > a card then it may logically follow to componentize this section. This
section
> > is basically the inside of a card like the one above. Which suggests that we
> > could use the same single class for both.
> 
> I agree about the font-size, but in the case it is a bit unclear where the
> "component" should go. Because of where I am applying the columns, I can't use
> the same structure for both sections.

Acknowledged.

[juliandoucette, 2017-09-22 13:49:15]

LGTM + NITs. I think that we can address the non-NITs below in separate issues.
Push after addressing NITs if you agree.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
File static/scss/components/_lists.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:39: .underlined-list .one-half:nth-child(1) a
On 2017/08/28 14:54:48, ire wrote:
> All of the list items typically have a border-bottom. With <li> items like the
> popular topics one that are displayed in two columns, the first item has a
> border-top as well. The media queries are because, the columns collapse into
one
> on smaller screens, so only the first list item needs the border-top until a
> certain viewport width

Can you add a comment about this?

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
File globals/get_page_name.py (right):

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
globals/get_page_name.py:17: def get_page_name(url):
NIT: I don't think we newline under definition declarations

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
globals/get_page_name.py:20: return split_url[ len(split_url) - 1 ]
NIT: I don't think we space inside brackets

https://codereview.adblockplus.org/29516622/diff/29549725/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29549725/globals/products.py...
globals/products.py:18: "abp": {
NIT: I think we prefer single quotes in python

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

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/content...
static/scss/content/_typography.scss:56: @extend .ta-center;
On 2017/09/19 10:27:23, ire wrote:
> Do we have anything against using @extend rules?

Yes; unfortunately.

But I think we aught not to. Lets keep em for now.

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
static/scss/layout/_body.scss:19: background-image: url(/img/png/help-bg.png);
This should be smaller/texturized/repeated and transparent.

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
static/scss/layout/_body.scss:25: padding-top: $lg;
Can we get rid of these custom spaces entirely in a separate issue? We already
have space variables in website-defaults that we can set.

[ire, 2017-09-25 07:25:30]

> I think that we can address the non-NITs below in separate issues.

Thanks! Done.

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
File static/scss/components/_lists.scss (right):

https://codereview.adblockplus.org/29516622/diff/29516623/static/scss/compone...
static/scss/components/_lists.scss:39: .underlined-list .one-half:nth-child(1) a
On 2017/09/22 13:49:13, juliandoucette wrote:
> On 2017/08/28 14:54:48, ire wrote:
> > All of the list items typically have a border-bottom. With <li> items like
the
> > popular topics one that are displayed in two columns, the first item has a
> > border-top as well. The media queries are because, the columns collapse into
> one
> > on smaller screens, so only the first list item needs the border-top until a
> > certain viewport width
> 
> Can you add a comment about this?

Done.

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
File globals/get_page_name.py (right):

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
globals/get_page_name.py:17: def get_page_name(url):
On 2017/09/22 13:49:13, juliandoucette wrote:
> NIT: I don't think we newline under definition declarations

Done.

https://codereview.adblockplus.org/29516622/diff/29549725/globals/get_page_na...
globals/get_page_name.py:20: return split_url[ len(split_url) - 1 ]
On 2017/09/22 13:49:13, juliandoucette wrote:
> NIT: I don't think we space inside brackets

Done.

https://codereview.adblockplus.org/29516622/diff/29549725/globals/products.py
File globals/products.py (right):

https://codereview.adblockplus.org/29516622/diff/29549725/globals/products.py...
globals/products.py:18: "abp": {
On 2017/09/22 13:49:14, juliandoucette wrote:
> NIT: I think we prefer single quotes in python

Done.

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

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/content...
static/scss/content/_typography.scss:56: @extend .ta-center;
On 2017/09/22 13:49:14, juliandoucette wrote:
> On 2017/09/19 10:27:23, ire wrote:
> > Do we have anything against using @extend rules?
> 
> Yes; unfortunately.
> 
> But I think we aught not to. Lets keep em for now.

Acknowledged.

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
File static/scss/layout/_body.scss (right):

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
static/scss/layout/_body.scss:19: background-image: url(/img/png/help-bg.png);
On 2017/09/22 13:49:14, juliandoucette wrote:
> This should be smaller/texturized/repeated and transparent.

I don't see the difference between the implementation and the design here.

- Why smaller? The image is the same size as in the mockup/spec
- The background image isn't supposed to be repeated. It's supposed to fade out
as it does here. 
- It is already transparent?

(Since I'm about to close this issue, we can continue this discuss in IRC or you
can create an issue with more info)

https://codereview.adblockplus.org/29516622/diff/29549725/static/scss/layout/...
static/scss/layout/_body.scss:25: padding-top: $lg;
On 2017/09/22 13:49:15, juliandoucette wrote:
> Can we get rid of these custom spaces entirely in a separate issue? We already
> have space variables in website-defaults that we can set.

https://issues.adblockplus.org/ticket/5765