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

Issue 29572820: Issue 5727 - Implement non-embedded homepage video on abp.org (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by ire
Modified:
1 month, 4 weeks ago
Reviewers:
juliandoucette
Base URL:
https://hg.adblockplus.org/web.adblockplus.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Updated levels #

Total comments: 19

Patch Set 3 : Addressed NITs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -6 lines) Patch
M includes/index.tmpl View 1 2 1 chunk +11 lines, -1 line 0 comments Download
M static/css/index.css View 1 2 1 chunk +66 lines, -2 lines 0 comments Download
M static/css/index-mobile.css View 1 2 1 chunk +22 lines, -2 lines 0 comments Download
A static/img/video-external.png View 1 Binary file 0 comments Download
A static/img/video-play.png View 1 Binary file 0 comments Download
A static/img/video-thumbnail.jpg View 1 Binary file 0 comments Download
M static/js/index.js View 1 2 chunks +49 lines, -0 lines 0 comments Download
M templates/default.tmpl View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 11
ire
2 months ago (2017-10-10 18:09:10 UTC) #1
ire
Ready for review. I currently haven't handled Level 2 (Touch Devices). Since touch devices can't ...
2 months ago (2017-10-10 18:13:36 UTC) #2
juliandoucette
> I currently haven't handled Level 2 (Touch Devices). Since touch devices can't be reliably ...
2 months ago (2017-10-11 15:16:06 UTC) #3
juliandoucette
On 2017/10/11 15:16:06, juliandoucette wrote: > I would default to Level 2 and enhance to ...
2 months ago (2017-10-11 15:19:46 UTC) #4
juliandoucette
> I would default to Level 2 and enhance to level 3. And I think ...
2 months ago (2017-10-11 15:26:43 UTC) #5
juliandoucette
See https://issues.adblockplus.org/ticket/5727#comment:25
2 months ago (2017-10-11 15:39:22 UTC) #6
juliandoucette
Partial-LGTM (I think that this is a pushable improvement. But we should improve it further ...
2 months ago (2017-10-11 15:52:38 UTC) #7
ire
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#newcode135 includes/index.tmpl:135: <img id="video-play" ...
2 months ago (2017-10-12 19:48:26 UTC) #8
juliandoucette
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#newcode135 includes/index.tmpl:135: <img id="video-play" src="/img/video-external.png" alt="Play Video"> On 2017/10/12 ...
2 months ago (2017-10-13 12:20:11 UTC) #9
ire
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.css#newcode101 static/css/index.css:101: position: relative; On 2017/10/13 12:20:11, juliandoucette wrote: > NIT: ...
1 month, 4 weeks ago (2017-10-16 08:28:01 UTC) #10
juliandoucette
1 month, 4 weeks ago (2017-10-16 12:22:01 UTC) #11
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.
Sign in to reply to this message.

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