Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Delta Between Two Patch Sets: includes/index/media.tmpl

Issue 29499714: Issue 4912 - Update media coverage links on eyeo.com (Closed) Base URL: https://hg.adblockplus.org/web.eyeo.com
Left Patch Set: Use macro, move external link icon to HTML Created Aug. 1, 2017, 9:58 a.m.
Right Patch Set: Refactor alternative text and title for media coverage links Created Aug. 2, 2017, 10:33 a.m.
Left:
Right:
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
Left: Side by side diff | Download
Right: Side by side diff | Download
LEFTRIGHT
1 <h1 class="heading centered"> 1 {% set media_coverage = [
2 <span>Media coverage:</span>
3 </h1>
4
5 {% set media = [
juliandoucette 2017/08/01 23:07:33 NIT: I think "media" is a little too general / mis
ire 2017/08/02 10:35:59 Done.
6 { 2 {
7 "url": "http://www.mediapost.com/publications/article/289691/adblock-plus-co mes-to-new-york.html", 3 "url": "http://www.mediapost.com/publications/article/289691/adblock-plus-co mes-to-new-york.html",
8 "publication": "Media Post", 4 "publisher": "Media Post",
juliandoucette 2017/08/01 23:07:33 NIT: Are these "publication"s? Suggest: author, pu
ire 2017/08/02 10:35:58 I think "publisher" works better. Author implies t
9 "image": "media-post" 5 "image": "media-post"
10 }, 6 },
11 { 7 {
12 "url": "https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blocke rs-and-the-nuisance-at-the-heart-of-the-modern-web.html", 8 "url": "https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blocke rs-and-the-nuisance-at-the-heart-of-the-modern-web.html",
13 "publication": "The New York Times", 9 "publisher": "The New York Times",
14 "image": "new-york-times" 10 "image": "new-york-times"
15 }, 11 },
16 { 12 {
17 "url": "https://www.wsj.com/articles/adblock-plus-chief-till-faida-says-cons umers-are-fed-up-with-current-online-ads-1462981668", 13 "url": "https://www.wsj.com/articles/adblock-plus-chief-till-faida-says-cons umers-are-fed-up-with-current-online-ads-1462981668",
18 "publication": "The Wall Street Journal", 14 "publisher": "The Wall Street Journal",
19 "image": "wall-street-journal" 15 "image": "wall-street-journal"
20 }, 16 },
21 { 17 {
22 "url": "https://techcrunch.com/2016/05/09/adblock-plus-closes-in-on-a-billio n-downloads/", 18 "url": "https://techcrunch.com/2016/05/09/adblock-plus-closes-in-on-a-billio n-downloads/",
23 "publication": "TechCrunch", 19 "publisher": "TechCrunch",
24 "image": "techcrunch" 20 "image": "techcrunch"
25 }, 21 },
26 { 22 {
27 "url": "http://www.businessinsider.com/theres-nothing-wrong-about-the-way-ad block-plus-makes-money-2015-9", 23 "url": "http://www.businessinsider.com/theres-nothing-wrong-about-the-way-ad block-plus-makes-money-2015-9",
28 "publication": "Business Insider", 24 "publisher": "Business Insider",
29 "image": "business-insider" 25 "image": "business-insider"
30 } 26 }
31 ] %} 27 ] %}
32 28
33 {% macro media_item(item) -%} 29 {% macro media_coverage_item(item) -%}
34 <a href="{{ item.url }}" target="_blank"> 30 <a href="{{ item.url }}" target="_blank" title="Open featured {{ item.publicat ion }} article in a new window">
juliandoucette 2017/08/01 23:07:33 NIT: I think we should describe the action (openin
ire 2017/08/02 10:35:58 Done.
35 <img class="publication-logo" alt="{{ item.publication }} (Opens in a new wi ndow)" src="/images/coverage/{{ item.image }}-1x.png" srcset="/images/coverage/{ { item.image }}-2x.png 2x"> 31 <img class="publisher-logo" alt="{{ item.publisher }} logo" src="/images/cov erage/{{ 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
ire 2017/08/02 10:35:58 Agreed on using "{{ item.publication }} logo". I'
juliandoucette 2017/08/02 20:31:12 Acknowledged.
36 <img class="external-link-icon" src="/images/icon-external-link.png" srcset= "/images/icon-external-link.svg 2x" alt=""> 32 <img class="external-link-icon" src="/images/icon-external-link.png" srcset= "/images/icon-external-link.svg 2x" alt="External link icon">
ire 2017/08/01 10:04:06 I purposefully left a null alternative text here b
juliandoucette 2017/08/01 23:07:33 Acknowledged. I would suggest something like: al
ire 2017/08/02 10:35:59 Makes sense. I changed the alt, but added the titl
juliandoucette 2017/08/02 20:31:12 Acknowledged.
37 </a> 33 </a>
38 {% endmacro %} 34 {% endmacro %}
39 35
36 <h1 class="heading">
37 <span>Media coverage:</span>
38 </h1>
39
40 <ul id="media-links"> 40 <ul id="media-links">
41 {% for item in media %} 41 {% for item in media_coverage %}
42 <li>{{ media_item(item) }}</li> 42 <li>{{ media_coverage_item(item) }}</li>
43 {% endfor %} 43 {% endfor %}
44 </ul> 44 </ul>
45
46 <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
LEFTRIGHT

Powered by Google App Engine
This is Rietveld