Fixes #6 - Add media mention banner

Fixes #6 - Add media mention banner
Issue - https://gitlab.com/eyeo/web.adblockplus.org/issues/6

Branch: index_page

[juliandoucette, 2018-02-19 14:15:57]

Thanks Ire!

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:73: <div id="content">
Did you intend to include this change in this patchset? If so, why?

Detail: This causes there to be a large space between the main video and
download button.

(I'm not talking about this div specifically. I'm talking about the changes on
this page besides the media section.)

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:188: <div class="bg-primary section">
The video is currently overlapping this section on small screens :/ (it should
be above the download button, not below it?)

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:193: <small>(Links open in a new window)</small>
NIT: aria-hidden?

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:199: <img src="/img/media/{{ item.image }}.png"
srcset="/img/{{ item.image }}-2x.png 2x" alt="{{ item.image_alt }}">
You forgot "media/" in srcset.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:695: * #media
Detail/TOL: This widget also appears on eyeo.com. And I suspect it will occur
again on other websites in the future. Therefore, I think we should implement it
as consistently as we can across websites.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:711: font-size: 100%;
Why 100% instead of 1em?

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:723: max-width: 5em;
Why max heigh/width instead of *just* hight/width?

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:775: margin: 0 2em;
Is this meant to make not display:flex look a little better? Otherwise, it seems
to just add a little space around the list itself?

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:799: #media-list { direction: rtl; }
Why are these necessary?

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:802: @supports (display: flex)
There seems to be too little space between items without display:flex on
desktop.

[ire, 2018-02-20 10:49:05]

Thanks Julian! 

New patch uploaded

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:73: <div id="content">
On 2018/02/19 14:15:56, juliandoucette wrote:
> Did you intend to include this change in this patchset? If so, why?

Yes. We can't have the #content on the same level as the .content class anymore
because elements on the homepage span full width. So I moved it here instead

> Detail: This causes there to be a large space between the main video and
> download button.
> 
> (I'm not talking about this div specifically. I'm talking about the changes on
> this page besides the media section.)

Yes, this isn't because I moved #content element here. It's because I changed
the overriding of the #content width to be restricted to a smaller size.

Although this change does affect more that the media section, it is a change
that should be made because we no longer want the #content of the homepage to
not span the full width possible.

Since none of these changes will be pushed until the rest of the page is fixed,
I didn't think it made sense to patch that particular issue now. Do you think I
should?

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:188: <div class="bg-primary section">
On 2018/02/19 14:15:56, juliandoucette wrote:
> The video is currently overlapping this section on small screens :/ 

It doesn't appear to be overlapping for me which is strange. It just rests
directly at the edge of the media section. Nothing has actually changed with the
spacing, the reason there was previously space between he video and #key-facts
was because there was padding on the #key-facts element.

I will apply some margin below the video to fix this, but I don't think (as I
mentioned in my comment below) that adding more code to fix it is necessary
since it will very soon change as well and this change is only about the #media
section.

> (it should
> be above the download button, not below it?)

The video should be below the download button.

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:188: <div class="bg-primary section">
On 2018/02/19 14:15:56, juliandoucette wrote:
> The video is currently overlapping this section on small screens :/ (it should
> be above the download button, not below it?)

Done.

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:193: <small>(Links open in a new window)</small>
On 2018/02/19 14:15:56, juliandoucette wrote:
> NIT: aria-hidden?

Why would we use aria-hidden? It is displayed on smaller screens.

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:199: <img src="/img/media/{{ item.image }}.png"
srcset="/img/{{ item.image }}-2x.png 2x" alt="{{ item.image_alt }}">
On 2018/02/19 14:15:56, juliandoucette wrote:
> You forgot "media/" in srcset.

Thanks! Done.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:695: * #media
On 2018/02/19 14:15:56, juliandoucette wrote:
> Detail/TOL: This widget also appears on http://eyeo.com. And I suspect it will
occur
> again on other websites in the future. Therefore, I think we should implement
it
> as consistently as we can across websites.

Noted & Agreed. Is there something in particular you want me to change?

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:711: font-size: 100%;
On 2018/02/19 14:15:57, juliandoucette wrote:
> Why 100% instead of 1em?

I have no idea lol. Changed.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:723: max-width: 5em;
On 2018/02/19 14:15:56, juliandoucette wrote:
> Why max heigh/width instead of *just* hight/width?

Because not all the images will have the same height/width. But we want them to
fit within a certain "box". If I were to set a specific width, the landscape
images will appear so much smaller than the portrait images and vice versa.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:775: margin: 0 2em;
On 2018/02/19 14:15:57, juliandoucette wrote:
> Is this meant to make not display:flex look a little better? 
> Otherwise, it seems to just add a little space around the list itself?

It's for both cases, we need some space around each item/image. Or maybe I
misunderstand what your issue is?

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:799: #media-list { direction: rtl; }
On 2018/02/19 14:15:57, juliandoucette wrote:
> Why are these necessary?

To align the list items to the right if flex is not supported. Otherwise the
images will be aligned left. It's the only way I could come up with to not have
the list items aligned left and have a similar look to the flex version.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:802: @supports (display: flex)
On 2018/02/19 14:15:57, juliandoucette wrote:
> There seems to be too little space between items without display:flex on
> desktop.

How much space should there be? The only reason there is so much space between
items with flex enabled is because they can take up all the available space,
which we can't do in the non-flex version.

[juliandoucette, 2018-02-21 11:22:54]

LGTM + NITs (Nothing blocking AFAICT - provided we investigate NITs and address
the #container issue separately).

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:73: <div id="content">
On 2018/02/20 10:49:04, ire wrote:
> Since none of these changes will be pushed until the rest of the page is
fixed,
> I didn't think it made sense to patch that particular issue now. Do you think
I
> should?

I'd prefer to fix the #content issue separately before pushing this solution.
But that's not a blocking preference.

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:193: <small>(Links open in a new window)</small>
On 2018/02/20 10:49:04, ire wrote:
> On 2018/02/19 14:15:56, juliandoucette wrote:
> > NIT: aria-hidden?
> 
> Why would we use aria-hidden? It is displayed on smaller screens.

It should be hidden aria-hidden on large screens because it's hidden on large
screens (and not on small)?

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:695: * #media
On 2018/02/20 10:49:05, ire wrote:
> Noted & Agreed. Is there something in particular you want me to change?

Not specifically. Ideally, the solution that you implement here should be
portable to eyeo.com. As a result, you may keep that in mind and test it on
eyeo.com while you are implementing if you choose.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:723: max-width: 5em;
On 2018/02/20 10:49:05, ire wrote:
> On 2018/02/19 14:15:56, juliandoucette wrote:
> > Why max heigh/width instead of *just* hight/width?
> 
> Because not all the images will have the same height/width. But we want them
to
> fit within a certain "box". If I were to set a specific width, the landscape
> images will appear so much smaller than the portrait images and vice versa.

Acknowledged.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:775: margin: 0 2em;
On 2018/02/20 10:49:04, ire wrote:
> It's for both cases, we need some space around each item/image. Or maybe I
> misunderstand what your issue is?

I was confused because the space applied to each item does not make a difference
when the space provided by display:flex is greater; except for the first space
before the first item and the last space after the last item.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:799: #media-list { direction: rtl; }
On 2018/02/20 10:49:05, ire wrote:
> To align the list items to the right if flex is not supported. Otherwise the
> images will be aligned left. It's the only way I could come up with to not
have
> the list items aligned left and have a similar look to the flex version.

I think that you should be able to make them identical using a non-flex layout.
I may be mistaking.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:802: @supports (display: flex)
On 2018/02/20 10:49:05, ire wrote:
> How much space should there be? The only reason there is so much space between
> items with flex enabled is because they can take up all the available space,
> which we can't do in the non-flex version. 

See answer above. This may or may not be an issue / blocking issue.

[ire, 2018-02-22 08:43:53]

Thanks Julian, will commit & push now

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:73: <div id="content">
On 2018/02/21 11:22:53, juliandoucette wrote:
> On 2018/02/20 10:49:04, ire wrote:
> > Since none of these changes will be pushed until the rest of the page is
> fixed,
> > I didn't think it made sense to patch that particular issue now. Do you
think
> I
> > should?
> 
> I'd prefer to fix the #content issue separately before pushing this solution.
> But that's not a blocking preference.

Ack. I'll keep it in this issue because 1) I'm being lazy and 2) I think it is
relevant to this issue as the change is required for this implementation to work

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:193: <small>(Links open in a new window)</small>
On 2018/02/21 11:22:53, juliandoucette wrote:
> On 2018/02/20 10:49:04, ire wrote:
> > On 2018/02/19 14:15:56, juliandoucette wrote:
> > > NIT: aria-hidden?
> > 
> > Why would we use aria-hidden? It is displayed on smaller screens.
> 
> It should be hidden aria-hidden on large screens because it's hidden on large
> screens (and not on small)?

I don't think `aria-hidden` is suitable here because this isn't something that
is hidden only for screen-reader users. It is hidden using `display: none` on
larger screens for all users, not just screen reader users. Maybe I'm missing
something here but I don't see how `aria-hidden` is relevant in this case.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:695: * #media
On 2018/02/21 11:22:53, juliandoucette wrote:
> On 2018/02/20 10:49:05, ire wrote:
> > Noted & Agreed. Is there something in particular you want me to change?
> 
> Not specifically. Ideally, the solution that you implement here should be
> portable to http://eyeo.com. As a result, you may keep that in mind and test
it on
> http://eyeo.com while you are implementing if you choose.

Ack. The layout between the two sites is noticeably different though. Since the
"As mentioned on" section is to the left. I see your point here though, and I
did actually use the solution from eyeo.com partly here and changed what was
different.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:775: margin: 0 2em;
On 2018/02/21 11:22:53, juliandoucette wrote:
> On 2018/02/20 10:49:04, ire wrote:
> > It's for both cases, we need some space around each item/image. Or maybe I
> > misunderstand what your issue is?
> 
> I was confused because the space applied to each item does not make a
difference
> when the space provided by display:flex is greater; except for the first space
> before the first item and the last space after the last item.

Acknowledged.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:799: #media-list { direction: rtl; }
On 2018/02/21 11:22:53, juliandoucette wrote:
> On 2018/02/20 10:49:05, ire wrote:
> > To align the list items to the right if flex is not supported. Otherwise the
> > images will be aligned left. It's the only way I could come up with to not
> have
> > the list items aligned left and have a similar look to the flex version.
> 
> I think that you should be able to make them identical using a non-flex
layout.
> I may be mistaking.

I was having trouble achieving the exact same spacing because we need the images
to be vertically centred, and the only non-flex way I could come up with was
table layout. And this forced my hand with the spacing between each image.

It should be noted that none of our "officially" supported devices don't support
flexbox (only the devices we have "partial" support for, i.e. IE 9, IE 8, will
see this version). 

Since this is a NIT, I will opt to keep this solution until I can think of a
better way.

[ire, 2018-02-22 08:55:13]

On 2018/02/22 08:43:53, ire wrote:
> Thanks Julian, will commit & push now

I haven't pushed yet, made a couple changes:

1. Optimised the images (forgot to do this before)
2. Updated the max-width for the image, see the comment below.

Will wait for your comment before I commit&push

https://codereview.adblockplus.org/29695555/diff/29705579/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29695555/diff/29705579/static/css/index.cs...
static/css/index.css:723: max-width: 10em;
I updated this figure because the techcrunch logo was looking to small IMO

[juliandoucette, 2018-02-23 13:46:19]

Thanks Ire!

Still LGTM + NITs.

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:193: <small>(Links open in a new window)</small>
On 2018/02/22 08:43:52, ire wrote:
> I don't think `aria-hidden` is suitable here because this isn't something that
> is hidden only for screen-reader users. It is hidden using `display: none` on
> larger screens for all users, not just screen reader users. Maybe I'm missing
> something here but I don't see how `aria-hidden` is relevant in this case.

On second thought, I think you're right. Thank you for explaining / pushing
back.

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29695555/diff/29695556/static/css/index.cs...
static/css/index.css:799: #media-list { direction: rtl; }
On 2018/02/22 08:43:52, ire wrote:
> Since this is a NIT, I will opt to keep this solution until I can think of a
> better way.

Agreed.

https://codereview.adblockplus.org/29695555/diff/29705579/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29705579/includes/index.tmpl...
includes/index.tmpl:73: <div id="content">
NIT: I'd rather assume that the media mentions section will expand properly and
address this issue separately like we did for the features/benefits section.

https://codereview.adblockplus.org/29695555/diff/29705579/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29695555/diff/29705579/static/css/index.cs...
static/css/index.css:723: max-width: 10em;
On 2018/02/22 08:55:12, ire wrote:
> I updated this figure because the techcrunch logo was looking to small IMO

Acknowledged.

[ire, 2018-02-26 08:32:12]

Thanks

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29695556/includes/index.tmpl...
includes/index.tmpl:193: <small>(Links open in a new window)</small>
On 2018/02/23 13:46:17, juliandoucette wrote:
> On 2018/02/22 08:43:52, ire wrote:
> > I don't think `aria-hidden` is suitable here because this isn't something
that
> > is hidden only for screen-reader users. It is hidden using `display: none`
on
> > larger screens for all users, not just screen reader users. Maybe I'm
missing
> > something here but I don't see how `aria-hidden` is relevant in this case.
> 
> On second thought, I think you're right. Thank you for explaining / pushing
> back.

You're welcome :)

https://codereview.adblockplus.org/29695555/diff/29705579/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29695555/diff/29705579/includes/index.tmpl...
includes/index.tmpl:73: <div id="content">
On 2018/02/23 13:46:17, juliandoucette wrote:
> NIT: I'd rather assume that the media mentions section will expand properly
and
> address this issue separately like we did for the features/benefits section.

Alright, done.

[juliandoucette, 2018-02-26 08:39:11]

One more thing:

It's hard to see the external window icon over top of the faded logos in this
section. I think we should adjust the opacity and/or set a solid black
background behind the icon itself.

This may be done separately.

[ire, 2018-02-26 08:51:11]

On 2018/02/26 08:39:11, juliandoucette wrote:
> One more thing:
> 
> It's hard to see the external window icon over top of the faded logos in this
> section. I think we should adjust the opacity and/or set a solid black
> background behind the icon itself.
> 
> This may be done separately.

Already pushed before I saw this :/

Will handle it separately