Issue 5727 - Implement non-embedded homepage video on abp.org

Issue 5727 - Implement non-embedded homepage video on abp.org

[ire, 2017-10-10 18:13:36]

Ready for review.

I currently haven't handled Level 2 (Touch Devices). Since touch devices can't
be reliably targeted, the only alternative I can think of is screen size. Any
thoughts?

https://codereview.adblockplus.org/29572820/diff/29572821/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29572820/diff/29572821/includes/index.tmpl...
includes/index.tmpl:136: <p id="video-disclaimer">
Users (currently) can't click directly on the text here to load/play the video.
I can add a link around all the text (besides the privacy policy) that would
link to the video (or trigger the loading/playing), but I feel that would be
unsemantic. What do you think?

[juliandoucette, 2017-10-11 15:16:06]

> I currently haven't handled Level 2 (Touch Devices). Since touch devices can't
be reliably targeted, the only alternative I can think of is screen size. Any
thoughts?

I would default to Level 2 and enhance to level 3. And I think browser.js mobile
and tablet flags are good enough?

[juliandoucette, 2017-10-11 15:19:46]

On 2017/10/11 15:16:06, juliandoucette wrote:
> I would default to Level 2 and enhance to level 3. 

This part of what I just said didn't make any sense :D. Please ignore it. Sorry
for the confusion.

[juliandoucette, 2017-10-11 15:26:43]

> I would default to Level 2 and enhance to level 3. And I think browser.js
mobile and tablet flags are good enough?

Scratch that. Ignore this entire comment.

[juliandoucette, 2017-10-11 15:39:22]

See https://issues.adblockplus.org/ticket/5727#comment:25

[juliandoucette, 2017-10-11 15:52:38]

Partial-LGTM (I think that this is a pushable improvement. But we should improve
it further as-discussed in the ticket.)

https://codereview.adblockplus.org/29572820/diff/29572821/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29572820/diff/29572821/includes/index.tmpl...
includes/index.tmpl:136: <p id="video-disclaimer">
On 2017/10/10 18:13:36, ire wrote:
> Users (currently) can't click directly on the text here to load/play the
video.
> I can add a link around all the text (besides the privacy policy) that would
> link to the video (or trigger the loading/playing), but I feel that would be
> unsemantic. What do you think?

I think that there is supposed to be a play button above the overlay above this
text? (The new abp.org design play button.)

https://codereview.adblockplus.org/29572820/diff/29572821/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29572820/diff/29572821/static/css/index.cs...
static/css/index.css:120: background-color: rgba(255,255,255,0.8);
NIT: I think this opacity makes a negligible difference

[ire, 2017-10-12 19:48:26]

Updated this based on the updated ticket.

https://codereview.adblockplus.org/29572820/diff/29574673/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29572820/diff/29574673/includes/index.tmpl...
includes/index.tmpl:135: <img id="video-play" src="/img/video-external.png"
alt="Play Video">
I forgot to translate this text. Will do in the next patch

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index-mo...
File static/css/index-mobile.css (right):

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index-mo...
static/css/index-mobile.css:40: .show-disclaimer #video-disclaimer
The disclaimer text was too long to be displayed on top of the thumbnail without
also obscuring the play icon, so I decided to move it to just below the
thumbnail on mobile

https://codereview.adblockplus.org/29572820/diff/29574673/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29572820/diff/29574673/static/js/index.js#...
static/js/index.js:31: // Change overlay icon on #video to play icon
I thought it would make more sense to change the url with JavaScript, rather
than having to show/hide and use <noscript> to show the relevant images.

[juliandoucette, 2017-10-13 12:20:11]

Minor issues.

https://codereview.adblockplus.org/29572820/diff/29574673/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29572820/diff/29574673/includes/index.tmpl...
includes/index.tmpl:135: <img id="video-play" src="/img/video-external.png"
alt="Play Video">
On 2017/10/12 19:48:25, ire wrote:
> I forgot to translate this text. Will do in the next patch

Acknowledged.

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index-mo...
File static/css/index-mobile.css (right):

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index-mo...
static/css/index-mobile.css:40: .show-disclaimer #video-disclaimer
On 2017/10/12 19:48:25, ire wrote:
> The disclaimer text was too long to be displayed on top of the thumbnail
without
> also obscuring the play icon, so I decided to move it to just below the
> thumbnail on mobile

Acknowledged.

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:101: position: relative;
NIT: I don't think that this does anything?

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:102: width: 100%;
NIT: I don't think that this does anything?

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:124: box-sizing: border-box;
NIT: I don't think that this does anything?

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:139: #video-play
NIT/Suggest: Move this up a little bit on desktop after the first click

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:144: width: 100px;
NIT: I think that this is too big?

https://codereview.adblockplus.org/29572820/diff/29574673/static/js/index.js
File static/js/index.js (right):

https://codereview.adblockplus.org/29572820/diff/29574673/static/js/index.js#...
static/js/index.js:31: // Change overlay icon on #video to play icon
On 2017/10/12 19:48:25, ire wrote:
> I thought it would make more sense to change the url with JavaScript, rather
> than having to show/hide and use <noscript> to show the relevant images.

Acknowledged.

[ire, 2017-10-16 08:28:01]

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:101: position: relative;
On 2017/10/13 12:20:11, juliandoucette wrote:
> NIT: I don't think that this does anything?

The #video-play icon is positioned absolutely in the center of the thumbnail. On
mobile devices without JS, when the disclaimer text is below the image, this
matters otherwise the #video-play icon will be positioned to the center of the
#video-container

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:102: width: 100%;
On 2017/10/13 12:20:11, juliandoucette wrote:
> NIT: I don't think that this does anything?

It keeps the <iframe id="video"> from extending outside the container

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:124: box-sizing: border-box;
On 2017/10/13 12:20:11, juliandoucette wrote:
> NIT: I don't think that this does anything?

Done.

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:139: #video-play
On 2017/10/13 12:20:11, juliandoucette wrote:
> NIT/Suggest: Move this up a little bit on desktop after the first click

Done.

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:144: width: 100px;
On 2017/10/13 12:20:11, juliandoucette wrote:
> NIT: I think that this is too big?

I'll change it to 80px so it'll be the same on mobile/desktop

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:144: width: 100px;
On 2017/10/13 12:20:11, juliandoucette wrote:
> NIT: I think that this is too big?

Done.

[juliandoucette, 2017-10-16 12:22:01]

LGTM

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:101: position: relative;
On 2017/10/16 08:28:01, ire wrote:
> On 2017/10/13 12:20:11, juliandoucette wrote:
> > NIT: I don't think that this does anything?
> 
> The #video-play icon is positioned absolutely in the center of the thumbnail.
On
> mobile devices without JS, when the disclaimer text is below the image, this
> matters otherwise the #video-play icon will be positioned to the center of the
> #video-container

Acknowledged.

https://codereview.adblockplus.org/29572820/diff/29574673/static/css/index.cs...
static/css/index.css:102: width: 100%;
On 2017/10/16 08:28:00, ire wrote:
> On 2017/10/13 12:20:11, juliandoucette wrote:
> > NIT: I don't think that this does anything?
> 
> It keeps the <iframe id="video"> from extending outside the container 

Acknowledged.