Noissue - Abstracted embedded video for use in blog

Noissue - Abstracted embedded video for use in blog

(I will create an issue soon / before publishing.)

Context:

Ben asked me to help him embed a video into a blog post. Previously, blog posters have been embedding video using an iframe and youtube-nocookie.com (which is not GDPR compliant because it doesn't ask for consent first like our homepage does). As a result, after consulting Judith (Ben consulted Judith, not me), I helped him embed the video the old way, and implemented a new way to replace the old asap.

Ben may post the old way tomorrow if we are not able to finish this change in time at his own risk/discretion.

All of this happened over IRC in the #websites channel Feb 20 2018 (You can download and search the logs if you like.)

[juliandoucette, 2018-02-20 22:28:11]

Added quick review / notes.

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html
File pages/test.html (right):

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html#new...
pages/test.html:1: title=Video test
Detail: I'm not planning to push this page. I just created it to demonstrate how
this abstraction works.

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html#new...
pages/test.html:4: <link rel="stylesheet" href="/css/video.css" />
NIT: The "/" is unnecessary here.

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html#new...
pages/test.html:5: <script src="/js/video.js"></script>
Detail: I wasn't sure about including this js/css separately or as part of main
js/css. So far, videos only appear on the index page and soon-to-be blog posts.
Therefore we *could* only include it there. However, I'm not necessarily against
including it in main js/css to avoid a separate HTTP request. I don't know which
option is how much worse for performance. But I would like to separate this code
via build process when we implement one for adblockplus.org.

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html#new...
pages/test.html:7: document.documentElement.className = "js";
NIT/Detail: This was only done inside index.js previously :/ (that is why I
inlined it here).

(I'll move it to main.js in the next patchset)

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html#new...
pages/test.html:30: <div class="video-parent" style="max-width: 520px;
margin-top: 1em;">
Detail: The first video demonstrates that this component is responsive (full
width) by default. The second demo demonstrates how to shrink the video (by
limiting the width of the parent or container).

https://codereview.adblockplus.org/29703633/diff/29703634/static/css/index.css
File static/css/index.css (right):

https://codereview.adblockplus.org/29703633/diff/29703634/static/css/index.cs...
static/css/index.css:72: #main, #home-video
Detail: I'm not sure if this is still necessary. I didn't test. I just kept it
to be safe.

https://codereview.adblockplus.org/29703633/diff/29703634/static/css/video.css
File static/css/video.css (right):

https://codereview.adblockplus.org/29703633/diff/29703634/static/css/video.cs...
static/css/video.css:16: padding-top: 56.25%;
Detail: I stole this trick from bootstrap. This percentage is proportional to
the aspect ratio of the video (16x9) in such a way that it guarantees that the
padding added will be equal to the height of the video in order to expand the
container while the iframe within is absolute and 100%. There is no other way to
make an iframe responsive that I know about (without JavaScript).

https://codereview.adblockplus.org/29703633/diff/29703634/static/js/video.js
File static/js/video.js (right):

https://codereview.adblockplus.org/29703633/diff/29703634/static/js/video.js#...
static/js/video.js:86: "height=285 " +
Detail: These height/widths are not necessary. I thought I removed them :/.
Will-do in the next patchset.

[juliandoucette, 2018-02-20 22:36:08]

Minor update before I call it a night.

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html
File pages/test.html (right):

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html#new...
pages/test.html:7: document.documentElement.className = "js";
On 2018/02/20 22:28:11, juliandoucette wrote:
> NIT/Detail: This was only done inside index.js previously :/ (that is why I
> inlined it here).
> 
> (I'll move it to main.js in the next patchset)

Done.

https://codereview.adblockplus.org/29703633/diff/29703634/static/js/video.js
File static/js/video.js (right):

https://codereview.adblockplus.org/29703633/diff/29703634/static/js/video.js#...
static/js/video.js:86: "height=285 " +
On 2018/02/20 22:28:11, juliandoucette wrote:
> Detail: These height/widths are not necessary. I thought I removed them :/.
> Will-do in the next patchset.

Done.

[ire, 2018-02-21 09:11:29]

Thanks Julian, I wasn't able to test this out for some reason the patch was not
applying. Any idea what could be the problem?

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html
File pages/test.html (right):

https://codereview.adblockplus.org/29703633/diff/29703634/pages/test.html#new...
pages/test.html:7: document.documentElement.className = "js";
On 2018/02/20 22:28:11, juliandoucette wrote:
> NIT/Detail: This was only done inside index.js previously :/ (that is why I
> inlined it here).
> 
> (I'll move it to main.js in the next patchset)

My bad! Do you want to push this change separately?

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
File includes/scripts/initialize-videos.html (right):

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
includes/scripts/initialize-videos.html:2:
document.addEventListener("DOMContentLoaded", function(event)
Why have this in a separate file? We can check for the existence of
`.video-parent` elements in the videos.js file instead of the extra step of
having to include this script.

[juliandoucette, 2018-02-21 10:22:23]

On 2018/02/21 09:11:29, ire wrote:
> Thanks Julian, I wasn't able to test this out for some reason the patch was
not
> applying. Any idea what could be the problem?

How did you download the patch? If you used curl or wget then it could have
failed because the issue is protected (you have to login to download - curl or
wget would download the login page as a result). I've just tested the last
patchset on the tip of default and it applied successfully. I've also
unprotected this issue.

[juliandoucette, 2018-02-21 10:35:18]

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
File includes/scripts/initialize-videos.html (right):

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
includes/scripts/initialize-videos.html:2:
document.addEventListener("DOMContentLoaded", function(event)
On 2018/02/21 09:11:29, ire wrote:
> Why have this in a separate file? We can check for the existence of
> `.video-parent` elements in the videos.js file instead of the extra step of
> having to include this script.

I prefer to define a class/module in one file and apply it in another because
one may want to import said class/module and not apply it immediately/globally.
I chose to write a" Video class/function/module" instead of an "initialize all
videos script" - if that makes sense.

[ire, 2018-02-22 09:25:42]

On 2018/02/21 10:22:23, juliandoucette wrote:
> On 2018/02/21 09:11:29, ire wrote:
> > Thanks Julian, I wasn't able to test this out for some reason the patch was
> not
> > applying. Any idea what could be the problem?
> 
> How did you download the patch? If you used curl or wget then it could have
> failed because the issue is protected (you have to login to download - curl or
> wget would download the login page as a result). I've just tested the last
> patchset on the tip of default and it applied successfully. I've also
> unprotected this issue.

You were right that was the issue, I didn't realize the review was protected.
It's working now, thanks!

Comments below

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
File includes/scripts/initialize-videos.html (right):

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
includes/scripts/initialize-videos.html:2:
document.addEventListener("DOMContentLoaded", function(event)
On 2018/02/21 10:35:18, juliandoucette wrote:
> On 2018/02/21 09:11:29, ire wrote:
> > Why have this in a separate file? We can check for the existence of
> > `.video-parent` elements in the videos.js file instead of the extra step of
> > having to include this script.
> 
> I prefer to define a class/module in one file and apply it in another because
> one may want to import said class/module and not apply it
immediately/globally.
> I chose to write a" Video class/function/module" instead of an "initialize all
> videos script" - if that makes sense.

(Forgot to say that this is a NIT)

Is there ever a case in which we would have the `.video-parent` and
corresponding children and not want it initialised?

I agree with the decision to create the Video class, I just think there may not
be a need for having a separate/external script to intialise the Video when we
never want uninitialised videos. I guess my suggestion is to initialise the
videos in the main script instead of requiring a separate step of including the
script.

https://codereview.adblockplus.org/29703633/diff/29703642/static/css/video.css
File static/css/video.css (right):

https://codereview.adblockplus.org/29703633/diff/29703642/static/css/video.cs...
static/css/video.css:48: .video-disclaimer
There's no horizontal padding on this element, the text reaches the edges of the
container

https://codereview.adblockplus.org/29703633/diff/29703642/static/css/video.cs...
static/css/video.css:63: .contains-disclaimer .video-disclaimer
Suggest: Have the .contains-declaimer class applied by default and only remove
it when there is JS enabled

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js
File static/js/video.js (right):

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js#...
static/js/video.js:52: videoLink.addEventListener("click", function(event)
NIT: Move this logic to it's own function instead of having it here?

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js#...
static/js/video.js:56: if (this.parent.classList.contains("contains-disclaimer")
=== false)
NIT:

I prefer:

if (!condition)

Over:

if (condition === false)

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js#...
static/js/video.js:70: toggleDisclaimer: function()
The .contains-disclaimer class still shows even when the iframe is visible and
the disclaimer is no longer visible

[juliandoucette, 2018-02-23 13:31:08]

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
File includes/scripts/initialize-videos.html (right):

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
includes/scripts/initialize-videos.html:2:
document.addEventListener("DOMContentLoaded", function(event)
On 2018/02/22 09:25:41, ire wrote:
> 
> (Forgot to say that this is a NIT)
> 
> Is there ever a case in which we would have the `.video-parent` and
> corresponding children and not want it initialised?
> 
> I agree with the decision to create the Video class, I just think there may
not
> be a need for having a separate/external script to intialise the Video when we
> never want uninitialised videos. I guess my suggestion is to initialise the
> videos in the main script instead of requiring a separate step of including
the
> script.

I take your point. Thank you for explaining / pushing back. Done.

https://codereview.adblockplus.org/29703633/diff/29703642/static/css/video.css
File static/css/video.css (right):

https://codereview.adblockplus.org/29703633/diff/29703642/static/css/video.cs...
static/css/video.css:48: .video-disclaimer
On 2018/02/22 09:25:41, ire wrote:
> There's no horizontal padding on this element, the text reaches the edges of
the
> container

Good catch. Done.

https://codereview.adblockplus.org/29703633/diff/29703642/static/css/video.cs...
static/css/video.css:63: .contains-disclaimer .video-disclaimer
On 2018/02/22 09:25:41, ire wrote:
> Suggest: Have the .contains-declaimer class applied by default and only remove
> it when there is JS enabled

Good idea. Done.

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js
File static/js/video.js (right):

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js#...
static/js/video.js:52: videoLink.addEventListener("click", function(event)
On 2018/02/22 09:25:41, ire wrote:
> NIT: Move this logic to it's own function instead of having it here?

Done.

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js#...
static/js/video.js:56: if (this.parent.classList.contains("contains-disclaimer")
=== false)
On 2018/02/22 09:25:41, ire wrote:
> NIT:
> 
> I prefer:
> 
> if (!condition)
> 
> Over:
> 
> if (condition === false)

Good catch. Done.

https://codereview.adblockplus.org/29703633/diff/29703642/static/js/video.js#...
static/js/video.js:70: toggleDisclaimer: function()
On 2018/02/22 09:25:41, ire wrote:
> The .contains-disclaimer class still shows even when the iframe is visible and
> the disclaimer is no longer visible

Good catch. ~Done. (I changed the approach from "contains-disclaimer" to
"hide-disclaimer".)

[ire, 2018-02-26 18:33:18]

LGTM

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
File includes/scripts/initialize-videos.html (right):

https://codereview.adblockplus.org/29703633/diff/29703642/includes/scripts/in...
includes/scripts/initialize-videos.html:2:
document.addEventListener("DOMContentLoaded", function(event)
On 2018/02/23 13:31:07, juliandoucette wrote:
> On 2018/02/22 09:25:41, ire wrote:
> > 
> > (Forgot to say that this is a NIT)
> > 
> > Is there ever a case in which we would have the `.video-parent` and
> > corresponding children and not want it initialised?
> > 
> > I agree with the decision to create the Video class, I just think there may
> not
> > be a need for having a separate/external script to intialise the Video when
we
> > never want uninitialised videos. I guess my suggestion is to initialise the
> > videos in the main script instead of requiring a separate step of including
> the
> > script.
> 
> I take your point. Thank you for explaining / pushing back. Done.

You're welcome :)

https://codereview.adblockplus.org/29703633/diff/29706608/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29703633/diff/29706608/includes/index.tmpl...
includes/index.tmpl:132: <div id="home-video" class="video-parent
contains-disclaimer">
This still has the contains-disclaimer class

https://codereview.adblockplus.org/29703633/diff/29706608/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29703633/diff/29706608/static/js/main.js#n...
static/js/main.js:105: /** Toggle video disclaimer */
NIT: Unnecessary comment, the function title toggleDisclaimer already says this

[juliandoucette, 2018-02-27 13:10:40]

Thanks Ire! New PatchSet up. I addressed your comments and added encodeURI to
the innerHTML assignment just-in-case.

https://codereview.adblockplus.org/29703633/diff/29706608/includes/index.tmpl
File includes/index.tmpl (right):

https://codereview.adblockplus.org/29703633/diff/29706608/includes/index.tmpl...
includes/index.tmpl:132: <div id="home-video" class="video-parent
contains-disclaimer">
On 2018/02/26 18:33:18, ire wrote:
> This still has the contains-disclaimer class

Good catch.

https://codereview.adblockplus.org/29703633/diff/29706608/static/js/main.js
File static/js/main.js (right):

https://codereview.adblockplus.org/29703633/diff/29706608/static/js/main.js#n...
static/js/main.js:105: /** Toggle video disclaimer */
On 2018/02/26 18:33:18, ire wrote:
> NIT: Unnecessary comment, the function title toggleDisclaimer already says
this

Agreed.

[ire, 2018-02-28 09:23:18]

On 2018/02/27 13:10:40, juliandoucette wrote:
> Thanks Ire! New PatchSet up. I addressed your comments and added encodeURI to
> the innerHTML assignment just-in-case.

Ack. Good idea. LGTM again

[juliandoucette, 2018-02-28 20:27:29]

Done https://hg.adblockplus.org/web.adblockplus.org/rev/8f2d63231f5b.