Issue 4912 - Update media coverage links on eyeo.com

includes/index/media.tmpl

-<h1 class="heading centered">
-  <span>Media coverage:</span>
-</h1>
-
-{% set media = [

[juliandoucette, 2017/08/01 23:07:33]

NIT: I think "media" is a little too general / misleading. Can you be more
specific? e.g. media_coverage, media_coverage_links, coverage_links

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

Done.

+{% set media_coverage = [
   {
     "url": "http://www.mediapost.com/publications/article/289691/adblock-plus-comes-to-new-york.html",
-    "publication": "Media Post",

[juliandoucette, 2017/08/01 23:07:33]

NIT: Are these "publication"s? Suggest: author, publisher, from

[ire, 2017/08/02 10:35:58]

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

Done.

+    "publisher": "Media Post",
     "image": "media-post"
   },
   {
     "url": "https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blockers-and-the-nuisance-at-the-heart-of-the-modern-web.html",
-    "publication": "The New York Times",
+    "publisher": "The New York Times",
     "image": "new-york-times"
   },
   {
     "url": "https://www.wsj.com/articles/adblock-plus-chief-till-faida-says-consumers-are-fed-up-with-current-online-ads-1462981668",
-    "publication": "The Wall Street Journal",
+    "publisher": "The Wall Street Journal",
     "image": "wall-street-journal"
   },
   {
     "url": "https://techcrunch.com/2016/05/09/adblock-plus-closes-in-on-a-billion-downloads/",
-    "publication": "TechCrunch",
+    "publisher": "TechCrunch",
     "image": "techcrunch"
   },
   {
     "url": "http://www.businessinsider.com/theres-nothing-wrong-about-the-way-adblock-plus-makes-money-2015-9",
-    "publication": "Business Insider",
+    "publisher": "Business Insider",
     "image": "business-insider"
   }
 ] %}
 
-{% macro media_item(item) -%}
-  <a href="{{ item.url }}" target="_blank">

[juliandoucette, 2017/08/01 23:07:33]

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).

[ire, 2017/08/02 10:35:58]

Done.

-    <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">

[juliandoucette, 2017/08/01 23:07:33]

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.

[ire, 2017/08/02 10:35:58]

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.

[juliandoucette, 2017/08/02 20:31:12]

Acknowledged.

-    <img class="external-link-icon" src="/images/icon-external-link.png" srcset="/images/icon-external-link.svg 2x" alt="">

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

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

[juliandoucette, 2017/08/01 23:07:33]

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]

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

[juliandoucette, 2017/08/02 20:31:12]

Acknowledged.

+{% macro media_coverage_item(item) -%}
+  <a href="{{ item.url }}" target="_blank" title="Open featured {{ item.publication }} article in a new window">
+    <img class="publisher-logo" alt="{{ item.publisher }} logo" src="/images/coverage/{{ item.image }}-1x.png" srcset="/images/coverage/{{ item.image }}-2x.png 2x">
+    <img class="external-link-icon" src="/images/icon-external-link.png" srcset="/images/icon-external-link.svg 2x" alt="External link icon">
   </a>
 {% endmacro %}
 
+<h1 class="heading">
+  <span>Media coverage:</span>
+</h1>
+
 <ul id="media-links">
-  {% for item in media %}
-    <li>{{ media_item(item) }}</li>
+  {% for item in media_coverage %}
+    <li>{{ media_coverage_item(item) }}</li>
   {% endfor %}
 </ul>
+
+<small>(Links open in a new window)</small>

[juliandoucette, 2017/08/02 20:31:13]

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.