includes/index/media.html - Issue 29499714: Issue 4912 - Update media coverage links on eyeo.com

Side by Side Diff: includes/index/media.html

Issue 29499714: Issue 4912 - Update media coverage links on eyeo.com (Closed) Base URL: https://hg.adblockplus.org/web.eyeo.com

Patch Set: Created July 27, 2017, 4:46 p.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 <h1 class="heading centered">
	juliandoucette 2017/07/31 20:00:27 Should this be H1? Should this be H1? juliandoucette 2017/07/31 22:11:45 (I see that you have converted the existing markdo Show quoted text 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.) ire 2017/08/01 10:04:05 Yes I did just convert the existing markdown. It s Show quoted text 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.
	2 <span>Media coverage:</span>
	juliandoucette 2017/07/31 20:00:27 Why do we need a span here? Why do we need a span here? juliandoucette 2017/07/31 22:11:45 (I see that you have converted the existing markdo Show quoted text 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.) ire 2017/08/01 10:04:04 See answer above Show quoted text 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
	3 </h1>

	4

	5 <p id="media-links">
	juliandoucette 2017/07/31 20:00:28 NIT: I think an unordered list is more appropriate NIT: I think an unordered list is more appropriate? (But a paragraph is also fine?) ire 2017/08/01 10:04:05 I agree with you. Done. Show quoted text 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.
	6 <a href="http://www.mediapost.com/publications/article/289691/adblock-plus-com es-to-new-york.html" target="_blank">
	juliandoucette 2017/07/31 20:00:27 NIT: I think this is a good candidate for nofollow NIT: I think this is a good candidate for nofollow noreferrer? juliandoucette 2017/07/31 22:11:45 (I forgot that we will probably add nofollow noref Show quoted text 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.) ire 2017/08/01 10:04:05 Acknowledged. Show quoted text 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.
	7 <img alt="Media Post" src="/images/coverage/media-post-1x.png" srcset="/imag es/coverage/media-post-2x.png 2x">

	8 <span class="sr-only">Opens in a new window</span>
	juliandoucette 2017/07/31 20:00:27 NIT: Is there a good reason to put this in an .sr- 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? ire 2017/08/01 10:04:04 See my comment in the next patch Show quoted text 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
	9 </a>

	10

	11 <a href="https://www.nytimes.com/2015/08/20/technology/personaltech/ad-blocker s-and-the-nuisance-at-the-heart-of-the-modern-web.html" target="_blank">
	juliandoucette 2017/07/31 20:00:27 NIT: We have new window hints for desktops and scr 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. juliandoucette 2017/07/31 22:11:45 (Or we could show a tooltip/button on hover state Show quoted text 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:05 I’m not sure how to solve this: We can’t accurate Show quoted text 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? juliandoucette 2017/08/01 23:07:33 Two things come to mind... 1. Could we detect a n Show quoted text 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. ire 2017/08/02 10:35:57 I don't think we can do so reliably, because the " Show quoted text > 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. Show quoted text > 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.
	12 <img alt="The New York Times" src="/images/coverage/new-york-times-1x.png" s rcset="/images/coverage/new-york-times-2x.png 2x">

	13 <span class="sr-only">Opens in a new window</span>

	14 </a>

	15

	16 <a href="https://www.wsj.com/articles/adblock-plus-chief-till-faida-says-consu mers-are-fed-up-with-current-online-ads-1462981668" target="_blank">
	juliandoucette 2017/07/31 20:00:27 NIT: I would have created a macro for this ~list-i 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. ire 2017/08/01 10:04:04 Done. Show quoted text 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. ire 2017/08/01 10:04:05 I agree. I thought of doing that initially but did Show quoted text 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.
	17 <img alt="The Wall Street Journal" src="/images/coverage/wall-street-journal -1x.png" srcset="/images/coverage/wall-street-journal-2x.png 2x">

	18 <span class="sr-only">Opens in a new window</span>

	19 </a>

	20

	21 <a href="https://techcrunch.com/2016/05/09/adblock-plus-closes-in-on-a-billion -downloads/" target="_blank">

	22 <img alt="TechCrunch" src="/images/coverage/techcrunch-1x.png" srcset="/imag es/coverage/techcrunch-2x.png 2x">

	23 <span class="sr-only">Opens in a new window</span>

	24 </a>

	25

	26 <a href="http://www.businessinsider.com/theres-nothing-wrong-about-the-way-adb lock-plus-makes-money-2015-9" target="_blank">

	27 <img alt="Business Insider" src="/images/coverage/business-insider-1x.png" s rcset="/images/coverage/business-insider-2x.png 2x">

	28 <span class="sr-only">Opens in a new window</span>

	29 </a>

	30 </p>

OLD	NEW

« no previous file with comments | « README.md ('k') | includes/index/media.md » ('j') | includes/index/style.html » ('J')