Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(36)

Issue 29499714: Issue 4912 - Update media coverage links on eyeo.com (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 7 months ago by ire
Modified:
2 years, 6 months ago
CC:
Thomas Greiner, jeen
Base URL:
https://hg.adblockplus.org/web.eyeo.com
Visibility:
Public.

Description

Issue 4912 - Update media coverage links on eyeo.com

Patch Set 1 #

Total comments: 25

Patch Set 2 : Use macro, move external link icon to HTML #

Total comments: 15

Patch Set 3 : Refactor alternative text and title for media coverage links #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -11 lines) Patch
M README.md View 1 chunk +4 lines, -0 lines 0 comments Download
R includes/index/media.md View 1 chunk +0 lines, -11 lines 0 comments Download
A includes/index/media.tmpl View 1 2 1 chunk +46 lines, -0 lines 1 comment Download
A includes/index/style.html View 1 2 1 chunk +76 lines, -0 lines 2 comments Download
M pages/index.html View 1 chunk +2 lines, -0 lines 0 comments Download
M static/css/styles.css View 1 chunk +12 lines, -0 lines 0 comments Download
A static/images/coverage/business-insider-1x.png View Binary file 0 comments Download
A static/images/coverage/business-insider-2x.png View Binary file 0 comments Download
R static/images/coverage/cnet.png View Binary file 0 comments Download
R static/images/coverage/economist.png View Binary file 0 comments Download
R static/images/coverage/ft.png View Binary file 0 comments Download
R static/images/coverage/guardian.png View Binary file 0 comments Download
R static/images/coverage/marketingweek.png View Binary file 0 comments Download
A static/images/coverage/media-post-1x.png View Binary file 0 comments Download
A static/images/coverage/media-post-2x.png View Binary file 0 comments Download
A static/images/coverage/new-york-times-1x.png View Binary file 0 comments Download
A static/images/coverage/new-york-times-2x.png View Binary file 0 comments Download
R static/images/coverage/nytimes.png View Binary file 0 comments Download
R static/images/coverage/techcrunch.png View Binary file 0 comments Download
A static/images/coverage/techcrunch-1x.png View Binary file 0 comments Download
A static/images/coverage/techcrunch-2x.png View Binary file 0 comments Download
A static/images/coverage/wall-street-journal-1x.png View Binary file 0 comments Download
A static/images/coverage/wall-street-journal-2x.png View Binary file 0 comments Download
R static/images/coverage/wired.png View Binary file 0 comments Download
A static/images/icon-external-link.png View 1 Binary file 0 comments Download
A static/images/icon-external-link.svg View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 8
ire
2 years, 7 months ago (2017-07-27 16:46:54 UTC) #1
ire
Ready for review
2 years, 7 months ago (2017-07-27 16:50:15 UTC) #2
juliandoucette
Thanks Ire! Please cut me a little vacation slack :p (e.g. for not remembering if ...
2 years, 7 months ago (2017-07-31 20:00:28 UTC) #3
juliandoucette
Added afterthoughts. I also forgot to mention that the media coverage images do not fit ...
2 years, 7 months ago (2017-07-31 22:11:46 UTC) #4
ire
> Thanks Ire! > > Please cut me a little vacation slack :p (e.g. for ...
2 years, 6 months ago (2017-08-01 10:04:06 UTC) #5
juliandoucette
CC Jeen, Thomas @Ire Following up. I'm going to be AFK for most of the ...
2 years, 6 months ago (2017-08-01 23:07:34 UTC) #6
ire
Thanks https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/media.html File includes/index/media.html (right): https://codereview.adblockplus.org/29499714/diff/29499715/includes/index/media.html#newcode11 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 ...
2 years, 6 months ago (2017-08-02 10:35:59 UTC) #7
juliandoucette
2 years, 6 months ago (2017-08-02 20:31:14 UTC) #8
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.)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5