Issue 4912 - Update media coverage links on eyeo.com

Issue 4912 - Update media coverage links on eyeo.com

[ire, 2017-07-27 16:50:15]

Ready for review

[juliandoucette, 2017-07-31 20:00:28]

Thanks Ire!

Please cut me a little vacation slack :p (e.g. for not remembering if we try to
support IE 8 on eyeo.com or have asked about these numbers before.)

Feel free to jump in saroyanm. I may not get back to this quickly.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
File includes/index/media.html (right):

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:1: <h1 class="heading centered">
Should this be H1?

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:2: <span>Media coverage:</span>
Why do we need a span here?

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:5: <p id="media-links">
NIT: I think an unordered list is more appropriate? (But a paragraph is also
fine?)

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:6: <a
href="http://www.mediapost.com/publications/article/289691/adblock-plus-comes-to-new-york.html"
target="_blank">
NIT: I think this is a good candidate for nofollow noreferrer?

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:8: <span class="sr-only">Opens in a new window</span>
NIT: Is there a good reason to put this in an .sr-only span and not at the end
of the image title and alt?

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:11: <a
href="https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blockers-and-the-nuisance-at-the-heart-of-the-modern-web.html"
target="_blank">
NIT: We have new window hints for desktops and screenreaders but not for touch
devices. I suggest we put a label above or below this ~list that displays on
touch devices only.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:16: <a
href="https://www.wsj.com/articles/adblock-plus-chief-till-faida-says-consumers-are-fed-up-with-current-online-ads-1462981668"
target="_blank">
NIT: I would have created a macro for this ~list-item and looped a data
structure instead. But I don't think it makes much of a difference.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/styl...
File includes/index/style.html (right):

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/styl...
includes/index/style.html:11: max-width: 100%;
What does this do?

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/styl...
includes/index/style.html:26: background-size: 20px;
NIT: SVG background-image and background-size do not work in IE 8. Please avoid
if easy (we don't have to support IE8 on eyeo.com, but it may not hurt  - we can
ask Jessica how many IE 8 visitors we get. I don't remember if we have done this
before).

[juliandoucette, 2017-07-31 22:11:46]

Added afterthoughts.

I also forgot to mention that the media coverage images do not fit in one line
in my browser (Firefox ESR, Debian, lowDPI, 1280x800) (even when I zoom out)
(assuming that they are supposed to fit in one line)

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
File includes/index/media.html (right):

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:1: <h1 class="heading centered">
On 2017/07/31 20:00:27, juliandoucette wrote:
> Should this be H1?

(I see that you have converted the existing markdown as-is. As a result, please
consider this a NIT and address it here *or* in another issue.)

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:2: <span>Media coverage:</span>
On 2017/07/31 20:00:27, juliandoucette wrote:
> Why do we need a span here?

(I see that you have converted the existing markdown as-is. As a result, please
consider this a NIT and address it here *or* in another issue.)

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:6: <a
href="http://www.mediapost.com/publications/article/289691/adblock-plus-comes-to-new-york.html"
target="_blank">
On 2017/07/31 20:00:27, juliandoucette wrote:
> NIT: I think this is a good candidate for nofollow noreferrer?

(I forgot that we will probably add nofollow noreferrer automatically via CMS
somehow.)

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:11: <a
href="https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blockers-and-the-nuisance-at-the-heart-of-the-modern-web.html"
target="_blank">
On 2017/07/31 20:00:27, juliandoucette wrote:
> NIT: We have new window hints for desktops and screenreaders but not for touch
> devices. I suggest we put a label above or below this ~list that displays on
> touch devices only.

(Or we could show a tooltip/button on hover state that says ~"open in new
window" on touch and open on second touch or double touch.)

[ire, 2017-08-01 10:04:06]

> Thanks Ire!
> 
> Please cut me a little vacation slack :p (e.g. for not remembering if we try
to
> support IE 8 on eyeo.com or have asked about these numbers before.)

You're welcome! No problem :)

The issue of browser support on the various websites has been recurring, so I
created this ticket https://issues.adblockplus.org/ticket/5462

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
File includes/index/media.html (right):

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:1: <h1 class="heading centered">
On 2017/07/31 22:11:45, juliandoucette wrote:
> On 2017/07/31 20:00:27, juliandoucette wrote:
> > Should this be H1?
> 
> (I see that you have converted the existing markdown as-is. As a result,
please
> consider this a NIT and address it here *or* in another issue.)

Yes I did just convert the existing markdown. It shouldn't be an H1, but the
other headings on the page are also incorrect and would be better addressed as
one.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:2: <span>Media coverage:</span>
On 2017/07/31 22:11:45, juliandoucette wrote:
> On 2017/07/31 20:00:27, juliandoucette wrote:
> > Why do we need a span here?
> 
> (I see that you have converted the existing markdown as-is. As a result,
please
> consider this a NIT and address it here *or* in another issue.)

See answer above

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:5: <p id="media-links">
On 2017/07/31 20:00:28, juliandoucette wrote:
> NIT: I think an unordered list is more appropriate? (But a paragraph is also
> fine?)

I agree with you. Done.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:6: <a
href="http://www.mediapost.com/publications/article/289691/adblock-plus-comes-to-new-york.html"
target="_blank">
On 2017/07/31 22:11:45, juliandoucette wrote:
> On 2017/07/31 20:00:27, juliandoucette wrote:
> > NIT: I think this is a good candidate for nofollow noreferrer?
> 
> (I forgot that we will probably add nofollow noreferrer automatically via CMS
> somehow.)

Acknowledged.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:8: <span class="sr-only">Opens in a new window</span>
On 2017/07/31 20:00:27, juliandoucette wrote:
> NIT: Is there a good reason to put this in an .sr-only span and not at the end
> of the image title and alt?

See my comment in the next patch

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:11: <a
href="https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blockers-and-the-nuisance-at-the-heart-of-the-modern-web.html"
target="_blank">
On 2017/07/31 22:11:45, juliandoucette wrote:
> On 2017/07/31 20:00:27, juliandoucette wrote:
> > NIT: We have new window hints for desktops and screenreaders but not for
touch
> > devices. I suggest we put a label above or below this ~list that displays on
> > touch devices only.
> 
> (Or we could show a tooltip/button on hover state that says ~"open in new
> window" on touch and open on second touch or double touch.)

I’m not sure how to solve this:

We can’t accurately detect touch devices. See this article -
http://www.stucox.com/blog/you-cant-detect-a-touchscreen/

We can, however, change the style for smaller screens using regular media
queries, but I don’t know the best way to handle this either. The “double-tap”
behaviour doesn’t work well on mobile. Double tapping sometimes causes a zoom.
Displaying the icon above images might be a compromise, but I don’t think it
will look that good. 

The only solution I can think of now is adding a sentence below the icons (that
only show on smaller screens) that says “Links open in a new window“. What do
you think?

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:16: <a
href="https://www.wsj.com/articles/adblock-plus-chief-till-faida-says-consumers-are-fed-up-with-current-online-ads-1462981668"
target="_blank">
On 2017/07/31 20:00:27, juliandoucette wrote:
> NIT: I would have created a macro for this ~list-item and looped a data
> structure instead. But I don't think it makes much of a difference.

I agree. I thought of doing that initially but didn't want to change too much.
Will change it.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:16: <a
href="https://www.wsj.com/articles/adblock-plus-chief-till-faida-says-consumers-are-fed-up-with-current-online-ads-1462981668"
target="_blank">
On 2017/07/31 20:00:27, juliandoucette wrote:
> NIT: I would have created a macro for this ~list-item and looped a data
> structure instead. But I don't think it makes much of a difference.

Done.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/styl...
File includes/index/style.html (right):

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/styl...
includes/index/style.html:11: max-width: 100%;
On 2017/07/31 20:00:28, juliandoucette wrote:
> What does this do?

On smaller screens, the image shouldn't expand past 100%

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/styl...
includes/index/style.html:26: background-size: 20px;
On 2017/07/31 20:00:28, juliandoucette wrote:
> NIT: SVG background-image and background-size do not work in IE 8. Please
avoid
> if easy (we don't have to support IE8 on http://eyeo.com, but it may not hurt 
- we can
> ask Jessica how many IE 8 visitors we get. I don't remember if we have done
this
> before).

Okay. I'm going to change this to using an actual image in the HTML.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
File includes/index/media.tmpl (right):

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:36: <img class="external-link-icon"
src="/images/icon-external-link.png" srcset="/images/icon-external-link.svg 2x"
alt="">
I purposefully left a null alternative text here because:

1. I didn't think it made sense for the external link image to have the
alternative text, "Opens in a new window", so I moved it to the logo alternative
text like you suggested
2. Since the "Opens in a new window" text will be read out by the screen reader,
I didn't think it was necessary for a screen reader to also read “External
link”, or whatever alternative text would be placed here

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/styl...
File includes/index/style.html (right):

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/styl...
includes/index/style.html:49: #media-links
This is to force more than just one lone logo to a second line

[juliandoucette, 2017-08-01 23:07:34]

CC Jeen, Thomas

@Ire

Following up.

I'm going to be AFK for most of the day tomorrow.

@Ire, @Jeen

Please reach out to another reviewer (CC saroyanm, greiner) if you want to get
this out sooner.

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
File includes/index/media.html (right):

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:11: <a
href="https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blockers-and-the-nuisance-at-the-heart-of-the-modern-web.html"
target="_blank">
On 2017/08/01 10:04:05, ire wrote:
> The only solution I can think of now is adding a sentence below the icons
(that
> only show on smaller screens) that says “Links open in a new window“. What do
> you think?

Two things come to mind...

1. Could we detect a non-touch device by detecting a "hover" event?
2. I don't think that we should rely on the screen width of touch devices... But
the solution that you proposed above seems fine for now. 

We could address this in the simplest way possible in this review or separately.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
File includes/index/media.tmpl (right):

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:5: {% set media = [
NIT: I think "media" is a little too general / misleading. Can you be more
specific? e.g. media_coverage, media_coverage_links, coverage_links

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:8: "publication": "Media Post",
NIT: Are these "publication"s? Suggest: author, publisher, from

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:34: <a href="{{ item.url }}" target="_blank">
NIT: I think we should describe the action (opening a publisher's article in a
new window) on the element that triggers it (the title of this anchor tag) and
use the image titles and alts to describe the images (see comments below).

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:35: <img class="publication-logo" alt="{{
item.publication }} (Opens in a new window)" src="/images/coverage/{{ item.image
}}-1x.png" srcset="/images/coverage/{{ item.image }}-2x.png 2x">
NIT: I think alt text is supposed to describe the image. In this case, the image
is a publisher's logo e.g. "{{ item.publication }} logo". I think the
publisher's name makes a better title.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:36: <img class="external-link-icon"
src="/images/icon-external-link.png" srcset="/images/icon-external-link.svg 2x"
alt="">
On 2017/08/01 10:04:06, ire wrote:
> I purposefully left a null alternative text here because:
> 
> 1. I didn't think it made sense for the external link image to have the
> alternative text, "Opens in a new window", so I moved it to the logo
alternative
> text like you suggested
> 2. Since the "Opens in a new window" text will be read out by the screen
reader,
> I didn't think it was necessary for a screen reader to also read “External
> link”, or whatever alternative text would be placed here

Acknowledged.

I would suggest something like:

alt: External link icon
title: Open featured {{ item.publication }} article in a new window

[ire, 2017-08-02 10:35:59]

Thanks

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
File includes/index/media.html (right):

https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/medi...
includes/index/media.html:11: <a
href="https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blockers-and-the-nuisance-at-the-heart-of-the-modern-web.html"
target="_blank">
> 1. Could we detect a non-touch device by detecting a "hover" event?

I don't think we can do so reliably, because the "hover" event can be triggered
when a user taps the screen. For example, on a touch device, if you press and
hold the logo you will see the external link icon displayed.

> 2. I don't think that we should rely on the screen width of touch devices...
But
> the solution that you proposed above seems fine for now. 
> 
> We could address this in the simplest way possible in this review or
separately.

I agree it's not reliable. We could actually show the "Links open in a new
window" text no matter what screen size too.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
File includes/index/media.tmpl (right):

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:5: {% set media = [
On 2017/08/01 23:07:33, juliandoucette wrote:
> NIT: I think "media" is a little too general / misleading. Can you be more
> specific? e.g. media_coverage, media_coverage_links, coverage_links

Done.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:8: "publication": "Media Post",
On 2017/08/01 23:07:33, juliandoucette wrote:
> NIT: Are these "publication"s? Suggest: author, publisher, from

I think "publisher" works better. Author implies the individual who wrote the
article, whereas this is the institution publishing the article. 

Done.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:34: <a href="{{ item.url }}" target="_blank">
On 2017/08/01 23:07:33, juliandoucette wrote:
> NIT: I think we should describe the action (opening a publisher's article in a
> new window) on the element that triggers it (the title of this anchor tag) and
> use the image titles and alts to describe the images (see comments below).

Done.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:35: <img class="publication-logo" alt="{{
item.publication }} (Opens in a new window)" src="/images/coverage/{{ item.image
}}-1x.png" srcset="/images/coverage/{{ item.image }}-2x.png 2x">
On 2017/08/01 23:07:33, juliandoucette wrote:
> NIT: I think alt text is supposed to describe the image. In this case, the
image
> is a publisher's logo e.g. "{{ item.publication }} logo". I think the
> publisher's name makes a better title.

Agreed on using "{{ item.publication }} logo".

I'm not sure the benefit of adding the publisher's name as the title as well
though. Seems like duplication of information.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:36: <img class="external-link-icon"
src="/images/icon-external-link.png" srcset="/images/icon-external-link.svg 2x"
alt="">
On 2017/08/01 23:07:33, juliandoucette wrote:
> On 2017/08/01 10:04:06, ire wrote:
> > I purposefully left a null alternative text here because:
> > 
> > 1. I didn't think it made sense for the external link image to have the
> > alternative text, "Opens in a new window", so I moved it to the logo
> alternative
> > text like you suggested
> > 2. Since the "Opens in a new window" text will be read out by the screen
> reader,
> > I didn't think it was necessary for a screen reader to also read “External
> > link”, or whatever alternative text would be placed here
> 
> Acknowledged.
> 
> I would suggest something like:
> 
> alt: External link icon
> title: Open featured {{ item.publication }} article in a new window

Makes sense. I changed the alt, but added the title attribute on the anchor
element instead

[juliandoucette, 2017-08-02 20:31:14]

LGTM + NITs

You can push first and address the NITs in a new patchset or separately.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
File includes/index/media.tmpl (right):

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:35: <img class="publication-logo" alt="{{
item.publication }} (Opens in a new window)" src="/images/coverage/{{ item.image
}}-1x.png" srcset="/images/coverage/{{ item.image }}-2x.png 2x">
On 2017/08/02 10:35:58, ire wrote:
> On 2017/08/01 23:07:33, juliandoucette wrote:
> > NIT: I think alt text is supposed to describe the image. In this case, the
> image
> > is a publisher's logo e.g. "{{ item.publication }} logo". I think the
> > publisher's name makes a better title.
> 
> Agreed on using "{{ item.publication }} logo".
> 
> I'm not sure the benefit of adding the publisher's name as the title as well
> though. Seems like duplication of information.

Acknowledged.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/medi...
includes/index/media.tmpl:36: <img class="external-link-icon"
src="/images/icon-external-link.png" srcset="/images/icon-external-link.svg 2x"
alt="">
On 2017/08/02 10:35:59, ire wrote:
> On 2017/08/01 23:07:33, juliandoucette wrote:
> > On 2017/08/01 10:04:06, ire wrote:
> > > I purposefully left a null alternative text here because:
> > > 
> > > 1. I didn't think it made sense for the external link image to have the
> > > alternative text, "Opens in a new window", so I moved it to the logo
> > alternative
> > > text like you suggested
> > > 2. Since the "Opens in a new window" text will be read out by the screen
> > reader,
> > > I didn't think it was necessary for a screen reader to also read “External
> > > link”, or whatever alternative text would be placed here
> > 
> > Acknowledged.
> > 
> > I would suggest something like:
> > 
> > alt: External link icon
> > title: Open featured {{ item.publication }} article in a new window
> 
> Makes sense. I changed the alt, but added the title attribute on the anchor
> element instead

Acknowledged.

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/styl...
File includes/index/style.html (right):

https://codereview.adblockplus.org/29499714/diff/29502562/includes/index/styl...
includes/index/style.html:49: #media-links
On 2017/08/01 10:04:06, ire wrote:
> This is to force more than just one lone logo to a second line

NIT: Would you mind adding a comment to make this clear?

+ Ack

https://codereview.adblockplus.org/29499714/diff/29503557/includes/index/medi...
File includes/index/media.tmpl (right):

https://codereview.adblockplus.org/29499714/diff/29503557/includes/index/medi...
includes/index/media.tmpl:46: <small>(Links open in a new window)</small>
NIT:
- I think it's better to put labels above (to be upfront + in case someone
doesn't scroll to it)
- *Links* is not descriptive
- "*Links* (plural) open in *a* (singular) new window" is not accurate

Suggest:
- "Click a publisher to open an article in a new window", "Click a publisher to
see what they have to say about us (Opens a new window)", "Click a publisher to
find out more (opens a new window)" or similar (ask Lisa and/or Jeen)

Again, feel free to add this in a separate issue.

https://codereview.adblockplus.org/29499714/diff/29503557/includes/index/styl...
File includes/index/style.html (right):

https://codereview.adblockplus.org/29499714/diff/29503557/includes/index/styl...
includes/index/style.html:28: .external-link-icon
NIT: I think you could center this icon by positioning [top/left: 0,
height/width: 100%, line-height: 25px (100%), text-align: center]. And that
would properly center the alt text without overflowing if the image fails to
load.

https://codereview.adblockplus.org/29499714/diff/29503557/includes/index/styl...
includes/index/style.html:49: opacity: 0.2;
NIT: It may be hard to see the external link icon at this opacity. Here are a
few things that may help:

- Add a white background to the image via CSS (so that the alt text has a white
background too if the image doesn't load)
- Change this opacity to 0.1 (I'm not sure about this. I think we want people
who can't see well to see the effect as well as possible. And 0.1 may may the
fade out too hard to see.)