Fixes #7 - Updated benefits section on index page

Fixes #7 - Updated benefits section on index page

[juliandoucette, 2018-02-22 09:48:15]

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index.tmpl...
includes/index.tmpl:146: <? include index/features ?>
Detail: This will appear more constricted than it should because it is already
horizontally contained by default.tmpl. I do not think that this will be the
case in the future because most of the sections in the new index page do not fit
into a single container. Instead, each section will probably be it's own
container. That is why I added "container" to the top level inside this include.

[ire, 2018-02-23 09:58:17]

Thanks!

LGTM + NITs

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index.tmpl...
includes/index.tmpl:146: <? include index/features ?>
On 2018/02/22 09:48:15, juliandoucette wrote:
> Detail: This will appear more constricted than it should because it is already
> horizontally contained by default.tmpl. I do not think that this will be the
> case in the future because most of the sections in the new index page do not
fit
> into a single container. Instead, each section will probably be it's own
> container. That is why I added "container" to the top level inside this
include.

Acknowledged.

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
File includes/index/features.html (right):

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
includes/index/features.html:5: height="90px"
NIT: This size seems to big for smaller devices.

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
includes/index/features.html:10: <h2>{{ feature-faster-heading[heading] FASTER,
MORE ENJOYABLE BROWSING }}</h2>
NIT: I think the capitalisation should be applied with CSS

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
includes/index/features.html:11: <p>{{ feature-faster-body Block ads that
interrupt your browsing experience. Say goodbye to video ads, pop-ups, flashing
banners and more. Blocking these annoyances means pages load faster. }}</p>
TOL: It's not clear from the mockups because the headings all happen to take up
2 lines, but it seems like each piece of content (img, heading, paragraph)
should take up the same amount of space. i.e. the paragraphs should all start at
the same horizontal level.

[juliandoucette, 2018-02-23 12:41:59]

Thanks Ire! I added a card concept to better space out these items on mobile.
Please re-review.

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
File includes/index/features.html (right):

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
includes/index/features.html:5: height="90px"
On 2018/02/23 09:58:17, ire wrote:
> NIT: This size seems to big for smaller devices. 

Doesn't look that way to me. I encourage you to bring this up with Jeen if you
think so.

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
includes/index/features.html:10: <h2>{{ feature-faster-heading[heading] FASTER,
MORE ENJOYABLE BROWSING }}</h2>
On 2018/02/23 09:58:17, ire wrote:
> NIT: I think the capitalisation should be applied with CSS

Agreed. Done.

https://codereview.adblockplus.org/29705597/diff/29705598/includes/index/feat...
includes/index/features.html:11: <p>{{ feature-faster-body Block ads that
interrupt your browsing experience. Say goodbye to video ads, pop-ups, flashing
banners and more. Blocking these annoyances means pages load faster. }}</p>
On 2018/02/23 09:58:17, ire wrote:
> TOL: It's not clear from the mockups because the headings all happen to take
up
> 2 lines, but it seems like each piece of content (img, heading, paragraph)
> should take up the same amount of space. i.e. the paragraphs should all start
at
> the same horizontal level.

Good observation. I'd rather not set a fixed height on headings because it's a
headache to account for smaller screens and translations.

[ire, 2018-02-26 19:01:32]

Thanks Julian! Just a couple points below

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:720: /* Cards
There doesn't seem to me to be anything related to "cards" here. Most styles
here are only about spacing. Also, this doesn't appear to be a "card" in the
typical sense of a box with a contrasting background.

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:723: .card-deck
Did you mean for there to be a comma after this?

[juliandoucette, 2018-02-27 13:00:06]

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:720: /* Cards
On 2018/02/26 19:01:31, ire wrote:
> There doesn't seem to me to be anything related to "cards" here. Most styles
> here are only about spacing. Also, this doesn't appear to be a "card" in the
> typical sense of a box with a contrasting background. 

Agreed... Wasn't sure what to call it :/ ... Any suggestions?

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:723: .card-deck
On 2018/02/26 19:01:31, ire wrote:
> Did you mean for there to be a comma after this?

Yes *blush*

[ire, 2018-02-28 09:21:55]

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:720: /* Cards
On 2018/02/27 13:00:06, juliandoucette wrote:
> On 2018/02/26 19:01:31, ire wrote:
> > There doesn't seem to me to be anything related to "cards" here. Most styles
> > here are only about spacing. Also, this doesn't appear to be a "card" in the
> > typical sense of a box with a contrasting background. 
> 
> Agreed... Wasn't sure what to call it :/ ... Any suggestions?

I'm not sure either. Considering that this may be restricted to the benefits
section only, maybe we should just use the ID instead?

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:723: .card-deck
On 2018/02/27 13:00:06, juliandoucette wrote:
> On 2018/02/26 19:01:31, ire wrote:
> > Did you mean for there to be a comma after this?
> 
> Yes *blush*

:P

[juliandoucette, 2018-02-28 19:40:00]

(I moved includes/index/features.html to includes/features.html because
includes/index.tmpl exists and may cause a naming conflict when interpreted by
the cms)

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:720: /* Cards
On 2018/02/28 09:21:54, ire wrote:
> I'm not sure either. Considering that this may be restricted to the benefits
> section only, maybe we should just use the ID instead?

How about "item"?

[ire, 2018-03-01 09:06:05]

On 2018/02/28 19:40:00, juliandoucette wrote:
> (I moved includes/index/features.html to includes/features.html because
> includes/index.tmpl exists and may cause a naming conflict when interpreted by
> the cms)

Ack. I think it was a good idea to separate the includes belonging to the index
page into its own folder though, but we don't have to address that now (if ever)

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:720: /* Cards
On 2018/02/28 19:40:00, juliandoucette wrote:
> On 2018/02/28 09:21:54, ire wrote:
> > I'm not sure either. Considering that this may be restricted to the benefits
> > section only, maybe we should just use the ID instead?
> 
> How about "item"?

I don't think "item" really has much meaning either. Do you think this set of
styles will be repeated elsewhere?

[juliandoucette, 2018-03-01 10:34:09]

> Ack. I think it was a good idea to separate the includes belonging to the
index page into its own folder though, but we don't have to address that now (if
ever)

Agreed. And I have chosen (somewhat arbitrarily) to address that
later/separately.

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:720: /* Cards
On 2018/03/01 09:06:05, ire wrote:
> I don't think "item" really has much meaning either. Do you think this set of
> styles will be repeated elsewhere?

Yes, at least in the almost identical "features" section further down on the
home page. The idea is: "item" is like "card" except it only provides space
instead of border/background and space (acknowledging that I just made that up
retrospectively and I don't yet know for sure if it will be useful elsewhere
beyond these two examples).

[ire, 2018-03-01 14:15:08]

LGTM

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29705597/diff/29706598/static/css/main.css...
static/css/main.css:720: /* Cards
On 2018/03/01 10:34:09, juliandoucette wrote:
> On 2018/03/01 09:06:05, ire wrote:
> > I don't think "item" really has much meaning either. Do you think this set
of
> > styles will be repeated elsewhere?
> 
> Yes, at least in the almost identical "features" section further down on the
> home page. The idea is: "item" is like "card" except it only provides space
> instead of border/background and space (acknowledging that I just made that up
> retrospectively and I don't yet know for sure if it will be useful elsewhere
> beyond these two examples). 

Alright. I'm still not 100% sure about the name "item", but I can't come up with
anything better so I think we should stick with it.

[juliandoucette, 2018-03-05 14:06:25]

Done https://hg.adblockplus.org/web.adblockplus.org/rev/2585fe480462