Fixes #9 - Update cross-promotional section

Fixes #9 - Update cross-promotional section

https://gitlab.com/eyeo/web.adblockplus.org/issues/9

Branch: index_page https://hg.adblockplus.org/web.adblockplus.org/shortlog/index_page

[ire, 2018-02-06 16:31:14]

@Manvel please can you have a look at this.

Note that I'm working on the new index_page branch so you'll need to switch to
that branch before applying this patch

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:718: text-transform: uppercase;
All of the headings should be uppercase, but I think I should change that in a
separate issue so I can properly test all the cases.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/main.css
File static/css/main.css (right):

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/main.css...
static/css/main.css:3: font-family: Arial, sans;
The correct font-family isn't being applied yet so the implementation may not
look exactly correct. I'm not sure why the font wasn't changed with the start of
the updated design, I'll double check about that and handle it in a separate
issue.

[juliandoucette, 2018-02-20 09:23:34]

Thanks Ire!

Mostly NIT below :)

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (left):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <div id="adblock-browser-notification"
class="notice">
NIT/Suggest: "notification" has new meaning now that there is a web notification
API. As a result, I think this name is confusing. I think that this could be
more accurately described as a "section" or an "ad" (honestly :D).

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification"
class="section">
NIT/Suggest: This include only appears on one page and does not use any template
functionality AFAICT. For that reason, I think that it should be implemented in
a page instead of an include, and/or in HTML format instead of Jinja2.

Detail: The homepage is currently one big include IIRC. By "in a page" I mean
"in that include" in this context.

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:9: <a
href="https://eyeo.to/adblockbrowser/abp-website">
NIT: I suspect Jeen would like this to open in a new window?

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:9: <a
href="https://eyeo.to/adblockbrowser/abp-website">
NIT/Detail: This link doesn't make any sense to me :/. It seems like more than
this heading should be clickable if this heading is clickable.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:711: background-image:
url(../img/abb-notification-bg-desktop.png);
NIT: I think that we should request that this background slowly fades into the
background color behind on large desktops to avoid a hard edge.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:718: text-transform: uppercase;
On 2018/02/06 16:31:14, ire wrote:
> All of the headings should be uppercase, but I think I should change that in a
> separate issue so I can properly test all the cases. 

Acknowledged.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:726: .abb-logo
NIT: Why classname? I don't think that this applies to anything else? / I don't
think that we want multiple of these on a page?

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:734: max-width: 200px;
Why max width instead of width here?

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:737: #abb-notification figure,
I think that 2em is too much on small phones. Perhaps this should be >= tablet
only?

[ire, 2018-02-20 11:16:48]

Thanks Julian! 

New patch uploaded

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (left):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <div id="adblock-browser-notification"
class="notice">
On 2018/02/20 09:23:33, juliandoucette wrote:
> NIT/Suggest: "notification" has new meaning now that there is a web
notification
> API. As a result, I think this name is confusing. I think that this could be
> more accurately described as a "section" or an "ad" (honestly :D).

I agree that notification is not ideal and I would like to change it.

One question though, if I change the string name "abb_notification" and the
corresponding file name, will that have an impact with translations?

If so, I would probably rather stick with the unideal name.

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification"
class="section">
On 2018/02/20 09:23:33, juliandoucette wrote:
> NIT/Suggest: This include only appears on one page and does not use any
template
> functionality AFAICT. For that reason, I think that it should be implemented
in
> a page instead of an include, and/or in HTML format instead of Jinja2.
> 
> Detail: The homepage is currently one big include IIRC. By "in a page" I mean
> "in that include" in this context.

I'm a bit unsure about this. Since this only appears on one page, I am inclined
to agree that it shouldn't be a separate include. However, the index file is
currently so long that I think it's more readable to have things separated into
different includes. I would prefer to keep it in this include for now, at least
until the rest of the index page is cleaned up. 

Also, doesn't `get_string()` need to be inside a Jinja2 template?

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:9: <a
href="https://eyeo.to/adblockbrowser/abp-website">
On 2018/02/20 09:23:33, juliandoucette wrote:
> NIT: I suspect Jeen would like this to open in a new window?

If we do this, wouldn't we have to have all the usual things we do for links
that open in a new window?

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:9: <a
href="https://eyeo.to/adblockbrowser/abp-website">
On 2018/02/20 09:23:33, juliandoucette wrote:
> NIT/Detail: This link doesn't make any sense to me :/. It seems like more than
> this heading should be clickable if this heading is clickable.

Would you like to bring this up with Jeen? As this is how it is in the spec. 

For the record I do sort of get where you're coming from, but I don't think
*more* should be clickable. I think either the heading alone or nothing at all.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:711: background-image:
url(../img/abb-notification-bg-desktop.png);
On 2018/02/20 09:23:33, juliandoucette wrote:
> NIT: I think that we should request that this background slowly fades into the
> background color behind on large desktops to avoid a hard edge.

I like this idea but according to the prototype, the section immediately above
this has it's own background image -
https://eyeogmbh.invisionapp.com/share/KPFB1POXB#/screens/273054427_ABP_Home_...
. Not sure how we could blend those together as they are light versus dark.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:726: .abb-logo
On 2018/02/20 09:23:34, juliandoucette wrote:
> NIT: Why classname? I don't think that this applies to anything else? / I
don't
> think that we want multiple of these on a page?

Agreed. Done.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:734: max-width: 200px;
On 2018/02/20 09:23:33, juliandoucette wrote:
> Why max width instead of width here?

My intention was to allow more fluidity, but in practice the images stick to
200px so I will change to just using width.

Done.

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:737: #abb-notification figure,
On 2018/02/20 09:23:34, juliandoucette wrote:
> I think that 2em is too much on small phones. Perhaps this should be >= tablet
> only?

Done.

[juliandoucette, 2018-02-20 14:06:18]

Thanks Ire!

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification"
class="section">
On 2018/02/20 11:16:47, ire wrote:
> I would prefer to keep it in this include for now, at least
> until the rest of the index page is cleaned up. 

Acknowledged.

> Also, doesn't `get_string()` need to be inside a Jinja2 template?

Yes and no.

{{ get_string(id, page) }}

is the same as

{{ id }}

in a markdown or html file, provided the page passed to get_string is the
current page.

This implementation seems to be using a separate locale file, which makes sense
when you consider the way that the existing homepage is implemented (each
browser has it's own page and includes it's contents). However, we are going to
change the way that this page is implemented (browser detection on the frontend
instead of the backend), and our new implementation will not have this
~limitation (situation in which it makes sense to use a separate locale file).

Therefore, we could keep this as-is and change it later. Or, we could change it
now and not publish said change until the rest of the page is completed. Either
way, we should avoid publishing translations until both of these issues are
resolved to avoid duplication.

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:9: <a
href="https://eyeo.to/adblockbrowser/abp-website">
On 2018/02/20 11:16:47, ire wrote:
> Would you like to bring this up with Jeen? As this is how it is in the spec. 
> 
> For the record I do sort of get where you're coming from, but I don't think
> *more* should be clickable. I think either the heading alone or nothing at
all. 

Acknowledged. I'll ask her.

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:9: <a
href="https://eyeo.to/adblockbrowser/abp-website">
On 2018/02/20 11:16:47, ire wrote:
> If we do this, wouldn't we have to have all the usual things we do for links
> that open in a new window?

Yes, an external link icon; I think :/

(Obviously it would be strange to put an external link icon here. Again, I'll
ask Jeen.)

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29690912/diff/29690913/static/css/index.cs...
static/css/index.css:711: background-image:
url(../img/abb-notification-bg-desktop.png);
On 2018/02/20 11:16:47, ire wrote:
> I like this idea but according to the prototype, the section immediately above
> this has it's own background image -
>
https://eyeogmbh.invisionapp.com/share/KPFB1POXB#/screens/273054427_ABP_Home_...
> . Not sure how we could blend those together as they are light versus dark. 

Acknowledged. I meant left/right not top/bottom.

[ire, 2018-02-21 08:53:17]

Thanks Julian. Currently waiting for Jeen to update the spec/issue and provide
me with updated assets.

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification"
class="section">
On 2018/02/20 14:06:17, juliandoucette wrote:
> On 2018/02/20 11:16:47, ire wrote:
> > I would prefer to keep it in this include for now, at least
> > until the rest of the index page is cleaned up. 
> 
> Acknowledged.
> 
> > Also, doesn't `get_string()` need to be inside a Jinja2 template?
> 
> Yes and no.
> 
> {{ get_string(id, page) }}
> 
> is the same as
> 
> {{ id }}
> 
> in a markdown or html file, provided the page passed to get_string is the
> current page.
> 
> This implementation seems to be using a separate locale file, which makes
sense
> when you consider the way that the existing homepage is implemented (each
> browser has it's own page and includes it's contents). However, we are going
to
> change the way that this page is implemented (browser detection on the
frontend
> instead of the backend), and our new implementation will not have this
> ~limitation (situation in which it makes sense to use a separate locale file).
> 
> Therefore, we could keep this as-is and change it later. Or, we could change
it
> now and not publish said change until the rest of the page is completed.
Either
> way, we should avoid publishing translations until both of these issues are
> resolved to avoid duplication.

Ack. If it is the case that this will change across the whole homepage, then I
think we should do it altogether and keep this review to just the layout/design
change.

[juliandoucette, 2018-02-26 13:11:27]

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification"
class="section">
On 2018/02/21 08:53:16, ire wrote:
> Ack. If it is the case that this will change across the whole homepage, then I
> think we should do it altogether and keep this review to just the
layout/design
> change.

Agreed

[ire, 2018-02-28 08:40:53]

New patch set

[juliandoucette, 2018-02-28 19:48:22]

LGTM + NITs

https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification" class="section
bg-secondary">
NIT: This isn't really a "notification" anymore IMO (if it ever was anything
resembling that). Please consider renaming it to something else e.g.
"promotion".

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
File locales/en/abb-notification.json (right):

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the
App Store"
Suggest: *in* or *from* the App Store or (Apple | iOS) App Store.

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:24: "message": "Get Adblock Browser on Google
Play"
Suggest: *in* or *from* the Google Play *Store*.

[ire, 2018-03-01 09:11:11]

https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification" class="section
bg-secondary">
On 2018/02/28 19:48:21, juliandoucette wrote:
> NIT: This isn't really a "notification" anymore IMO (if it ever was anything
> resembling that). Please consider renaming it to something else e.g.
> "promotion".

You already brought this up here -
https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...

I think you may have missed my response?

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
File locales/en/abb-notification.json (right):

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the
App Store"
On 2018/02/28 19:48:21, juliandoucette wrote:
> Suggest: *in* or *from* the App Store or (Apple | iOS) App Store.

This is how it was written in the spec
(https://gitlab.com/eyeo/specs/spec/blob/c7dba1c91acc420051ec0065eede200ac0370...).
Could you please raise this issue/suggestion there?

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:24: "message": "Get Adblock Browser on Google
Play"
On 2018/02/28 19:48:21, juliandoucette wrote:
> Suggest: *in* or *from* the Google Play *Store*.

(See answer above)

[juliandoucette, 2018-03-01 09:53:33]

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (left):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <div id="adblock-browser-notification"
class="notice">
On 2018/02/20 11:16:47, ire wrote:
> On 2018/02/20 09:23:33, juliandoucette wrote:
> > NIT/Suggest: "notification" has new meaning now that there is a web
> notification
> > API. As a result, I think this name is confusing. I think that this could be
> > more accurately described as a "section" or an "ad" (honestly :D).
> 
> I agree that notification is not ideal and I would like to change it.
> 
> One question though, if I change the string name "abb_notification" and the
> corresponding file name, will that have an impact with translations?
> 
> If so, I would probably rather stick with the unideal name.

You can change the include file name, ids, and classes. But you can't change the
string ids or files without losing translations. get_string accepts a string id
and locale file name (below). If you changed that, then you would have to change
it in all locales and on crowdin.

https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notifi...
File includes/abb-notification.tmpl (right):

https://codereview.adblockplus.org/29690912/diff/29711555/includes/abb-notifi...
includes/abb-notification.tmpl:1: <section id="abb-notification" class="section
bg-secondary">
On 2018/03/01 09:11:10, ire wrote:
> You already brought this up here -
>
https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
> 
> I think you may have missed my response?

Yes, thank you. I'll respond there.

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
File locales/en/abb-notification.json (right):

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the
App Store"
On 2018/03/01 09:11:11, ire wrote:
> This is how it was written in the spec
>
(https://gitlab.com/eyeo/specs/spec/blob/c7dba1c91acc420051ec0065eede200ac0370...).
> Could you please raise this issue/suggestion there?

Yeah. We can change this separately.

[juliandoucette, 2018-03-01 09:58:15]

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
File locales/en/abb-notification.json (right):

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the
App Store"
On 2018/03/01 09:11:11, ire wrote:
> 
> This is how it was written in the spec
>
(https://gitlab.com/eyeo/specs/spec/blob/c7dba1c91acc420051ec0065eede200ac0370...).
> Could you please raise this issue/suggestion there?

Detail: There seems to be two separate alt texts provided in the spec. One under
"App store links" and another under "Assets".

[juliandoucette, 2018-03-01 10:26:08]

Sorry about the triple posting :D

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
File locales/en/abb-notification.json (right):

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the
App Store"
On 2018/03/01 09:58:14, juliandoucette wrote:
> Detail: There seems to be two separate alt texts provided in the spec. One
under
> "App store links" and another under "Assets".

I created https://gitlab.com/eyeo/specs/spec/issues/153

(Feel free to push and change this later if necessary.)

[ire, 2018-03-01 14:04:37]

Thanks Julian! Pushing now

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
File includes/abb-notification.tmpl (left):

https://codereview.adblockplus.org/29690912/diff/29690913/includes/abb-notifi...
includes/abb-notification.tmpl:1: <div id="adblock-browser-notification"
class="notice">
On 2018/03/01 09:53:33, juliandoucette wrote:
> On 2018/02/20 11:16:47, ire wrote:
> > On 2018/02/20 09:23:33, juliandoucette wrote:
> > > NIT/Suggest: "notification" has new meaning now that there is a web
> > notification
> > > API. As a result, I think this name is confusing. I think that this could
be
> > > more accurately described as a "section" or an "ad" (honestly :D).
> > 
> > I agree that notification is not ideal and I would like to change it.
> > 
> > One question though, if I change the string name "abb_notification" and the
> > corresponding file name, will that have an impact with translations?
> > 
> > If so, I would probably rather stick with the unideal name.
> 
> You can change the include file name, ids, and classes. But you can't change
the
> string ids or files without losing translations. get_string accepts a string
id
> and locale file name (below). If you changed that, then you would have to
change
> it in all locales and on crowdin.

In that case I would prefer to not have inconsistent naming within this
component, so I will keep it called "notification"

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
File locales/en/abb-notification.json (right):

https://codereview.adblockplus.org/29690912/diff/29711555/locales/en/abb-noti...
locales/en/abb-notification.json:18: "message": "Download Adblock Browser on the
App Store"
On 2018/03/01 10:26:07, juliandoucette wrote:
> On 2018/03/01 09:58:14, juliandoucette wrote:
> > Detail: There seems to be two separate alt texts provided in the spec. One
> under
> > "App store links" and another under "Assets".
> 
> I created https://gitlab.com/eyeo/specs/spec/issues/153
> 
> (Feel free to push and change this later if necessary.)

Ack. I will push now and change this later when the spec has been updated