New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow embeds via <video-js> element #4640

Merged
merged 29 commits into from Nov 13, 2017

Conversation

Projects
None yet
6 participants
@gkatsev
Member

gkatsev commented Oct 3, 2017

Description

Add the ability to initialize Video.js with an element named video-js. This is because sometimes, seeing the native element is undesirable, plus, it's cool to have our own element.
Can be used just like the video embed. For IE9-only we have a vjs-source element that's copied to a source element because IE9 is strict about keeping source elements inside of video elements.

TODO

  • tests need to be updated
  • fix tests in IE8
  • Limit this feature to IE10+ so we won't need a vjs-source element?
  • documentation
  • we should potentially update Player.getTagSettings instead of my workaround for sources to work properly with vjs-source.
  • We may need a vjs-track as well for IE9.
Show outdated Hide outdated src/js/setup.js
@@ -0,0 +1,165 @@
<!DOCTYPE html>

This comment has been minimized.

@gkatsev

gkatsev Oct 12, 2017

Member

this file will get autocopied into sandbox/embeds.html on the first npm start run by the user.
It has 18 different video.js embeds grouped by whether data-setup is used or not, how the source is added, and the 3 different embed types: video, video+player div wrapper, just div.

@gkatsev

gkatsev Oct 12, 2017

Member

this file will get autocopied into sandbox/embeds.html on the first npm start run by the user.
It has 18 different video.js embeds grouped by whether data-setup is used or not, how the source is added, and the 3 different embed types: video, video+player div wrapper, just div.

if (divEmbed) {
el = this.el_ = tag;
tag = this.tag = document.createElement('video');

This comment has been minimized.

@gkatsev

gkatsev Oct 12, 2017

Member

we probably should try to be smarted about this. Maybe we need an audio element instead but we might not have a source at this point. Probably more than good enough for first release.

@gkatsev

gkatsev Oct 12, 2017

Member

we probably should try to be smarted about this. Maybe we need an audio element instead but we might not have a source at this point. Probably more than good enough for first release.

This comment has been minimized.

@ldayananda

ldayananda Oct 17, 2017

Contributor

Isn't it possible that the div will contain a video element already?

@ldayananda

ldayananda Oct 17, 2017

Contributor

Isn't it possible that the div will contain a video element already?

This comment has been minimized.

@gkatsev

gkatsev Oct 30, 2017

Member

Missed this previously. Yes but I will say it's misconfigured in that case.

@gkatsev

gkatsev Oct 30, 2017

Member

Missed this previously. Yes but I will say it's misconfigured in that case.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 12, 2017

Member

So, other than adding a lot of tests, we should decide if we're ok with using data-setup on the div elements. Otherwise, this is pretty much good to go, I think.

Member

gkatsev commented Oct 12, 2017

So, other than adding a lot of tests, we should decide if we're ok with using data-setup on the div elements. Otherwise, this is pretty much good to go, I think.

}
.wrapper {
display: grid;

This comment has been minimized.

@gkatsev

gkatsev Oct 12, 2017

Member

yeah, grid!
screen shot 2017-10-12 at 16 07 17

@gkatsev

gkatsev Oct 12, 2017

Member

yeah, grid!
screen shot 2017-10-12 at 16 07 17

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 13, 2017

Member

In a grooming discussion, we talked about the div and data-setup issue. Because div is a lot more used and data-setup is a relatively generic name, we are at risk of trying to initialize a div that was not given to video.js. One suggestion that was brought up is to rename data-setup to be data-vjs-setup and keep data-setup for video/audio elements and perhaps deprecate it altogether.
What are your thoughts @videojs/core-committers?

Member

gkatsev commented Oct 13, 2017

In a grooming discussion, we talked about the div and data-setup issue. Because div is a lot more used and data-setup is a relatively generic name, we are at risk of trying to initialize a div that was not given to video.js. One suggestion that was brought up is to rename data-setup to be data-vjs-setup and keep data-setup for video/audio elements and perhaps deprecate it altogether.
What are your thoughts @videojs/core-committers?

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Oct 15, 2017

Member

Is the div embed meant to take over as the primary embed method or is it an optional feature for React-like situations?

data-setup is definitely a convenience thing. It's specifically useful in situations where a user doesn't have access to add javascript but can add HTML (some CMSs). Other than that, if this is an optional feature I'd be in favor of dropping data-setup for the div and see if anyone misses it. Otherwise data-vjs-setup seems fine too.

What I'd really love to see is a videojs web component instead of a div, if that at all makes sense to do here.

Member

heff commented Oct 15, 2017

Is the div embed meant to take over as the primary embed method or is it an optional feature for React-like situations?

data-setup is definitely a convenience thing. It's specifically useful in situations where a user doesn't have access to add javascript but can add HTML (some CMSs). Other than that, if this is an optional feature I'd be in favor of dropping data-setup for the div and see if anyone misses it. Otherwise data-vjs-setup seems fine too.

What I'd really love to see is a videojs web component instead of a div, if that at all makes sense to do here.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 16, 2017

Member

Haven't thought about whether this should be the default yet. The current pain-point is that if you use the video tag, you end up quickly flashing the default video element before video.js gets initialized. So, this will solve it. However, this will also work better for the react-like situations compared to what we currently have with the "ingested player div".

I do like the idea of a web-component, but I think that isn't necessarily equivalent to a div embed. Maybe just a custom element? However, native custom element support is pretty small right now and I would prefer not to have to include a polyfill for it.

Member

gkatsev commented Oct 16, 2017

Haven't thought about whether this should be the default yet. The current pain-point is that if you use the video tag, you end up quickly flashing the default video element before video.js gets initialized. So, this will solve it. However, this will also work better for the react-like situations compared to what we currently have with the "ingested player div".

I do like the idea of a web-component, but I think that isn't necessarily equivalent to a div embed. Maybe just a custom element? However, native custom element support is pretty small right now and I would prefer not to have to include a polyfill for it.

@ldayananda

This comment has been minimized.

Show comment
Hide comment
@ldayananda

ldayananda Oct 17, 2017

Contributor

I'm in favor of using data-vjs-setup and deprecating data-setup

Contributor

ldayananda commented Oct 17, 2017

I'm in favor of using data-vjs-setup and deprecating data-setup

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Oct 17, 2017

Contributor

data-vjs-setup sounds like a sensible way to go.

Contributor

mister-ben commented Oct 17, 2017

data-vjs-setup sounds like a sensible way to go.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 18, 2017

Member

The broken PR was interesting because it revealed that IE9 doesn't allow using a source element outside of a video element. We'd want to make sure that this is documented well before this PR is merged.

Member

gkatsev commented Oct 18, 2017

The broken PR was interesting because it revealed that IE9 doesn't allow using a source element outside of a video element. We'd want to make sure that this is documented well before this PR is merged.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 19, 2017

Member

At the video.js TSC meeting, we talked about whether we can just use a video-js element and a vjs-source element. At first glance is that we can but with the div embed we can just re-use that as the player div and create a tech video element inside of it. However, it seems like we won't be able to do that with the video-js element. Will investigate further.

Member

gkatsev commented Oct 19, 2017

At the video.js TSC meeting, we talked about whether we can just use a video-js element and a vjs-source element. At first glance is that we can but with the div embed we can just re-use that as the player div and create a tech video element inside of it. However, it seems like we won't be able to do that with the video-js element. Will investigate further.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Oct 20, 2017

Member
Member

heff commented Oct 20, 2017

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 23, 2017

Member

Oh, I think I figured out my issue. I hadn't updated the code to look for video-js over div and then I didn't actually copy the child elements over properly.

Member

gkatsev commented Oct 23, 2017

Oh, I think I figured out my issue. I hadn't updated the code to look for video-js over div and then I didn't actually copy the child elements over properly.

@gkatsev

So, with these changes, you can now use a video-js element! And also vjs-source elements (this mostly as a workaround for IE9).
moved things left to do to the top post

Show outdated Hide outdated src/js/player.js
@@ -31,6 +31,7 @@ const autoSetup = function() {
// through each list of elements to build up a new, combined list of elements.
const vids = document.getElementsByTagName('video');
const audios = document.getElementsByTagName('audio');
const divs = document.getElementsByTagName('video-js');

This comment has been minimized.

@gkatsev

gkatsev Oct 23, 2017

Member

we now look for video-js elements for data-setup, and so we definitely can keep data-setup now.

@gkatsev

gkatsev Oct 23, 2017

Member

we now look for video-js elements for data-setup, and so we definitely can keep data-setup now.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 23, 2017

Member

@videojs/core-committers Should we only allow video-js in browsers newer than IE9? Then we won't even need to have vjs-source support.

Member

gkatsev commented Oct 23, 2017

@videojs/core-committers Should we only allow video-js in browsers newer than IE9? Then we won't even need to have vjs-source support.

@gkatsev gkatsev changed the title from WIP initial div embed PR to feat: allow embeds via `<video-js>` element Oct 23, 2017

@gkatsev gkatsev changed the title from feat: allow embeds via `<video-js>` element to feat: allow embeds via <video-js> element Oct 23, 2017

@misteroneill

Looks good overall. I wonder if some of the variable names should be changed so they're not called divs... although, I can't think of a better option off the top of my head!

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 24, 2017

Member

Also, do we want the elements to be called video-js and vjs-source or just videojs and vjssource?

Member

gkatsev commented Oct 24, 2017

Also, do we want the elements to be called video-js and vjs-source or just videojs and vjssource?

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Oct 26, 2017

Contributor

Custom element names should contain a hyphen for namespacing.

video-* is probably too likely to conflict with something eventually. videojs-player might be safer.

Contributor

mister-ben commented Oct 26, 2017

Custom element names should contain a hyphen for namespacing.

video-* is probably too likely to conflict with something eventually. videojs-player might be safer.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 26, 2017

Member

Yup, probably best to keep the hyphen, though it isn't technically a real custom element.
I don't think that any potential conflicts that video-js can have are big enough and having it match the name is super nice, which is why I was asking about whether we should just go with videojs at the element name.

Member

gkatsev commented Oct 26, 2017

Yup, probably best to keep the hyphen, though it isn't technically a real custom element.
I don't think that any potential conflicts that video-js can have are big enough and having it match the name is super nice, which is why I was asking about whether we should just go with videojs at the element name.

@@ -0,0 +1,83 @@
# How to Embed the Video.js player

This comment has been minimized.

@gkatsev

gkatsev Oct 26, 2017

Member

after I added this page I remember that we have the setup.md file. This page can probably stay as is but setup.md probably should be updated a bit as well and point to this page.

@gkatsev

gkatsev Oct 26, 2017

Member

after I added this page I remember that we have the setup.md file. This page can probably stay as is but setup.md probably should be updated a bit as well and point to this page.

This comment has been minimized.

@gkatsev

gkatsev Oct 30, 2017

Member

Linked from setup.md now.

@gkatsev

gkatsev Oct 30, 2017

Member

Linked from setup.md now.

@misteroneill

Some minor grammatical points and one question about class.

Show outdated Hide outdated docs/guides/embeds.md
Show outdated Hide outdated docs/guides/embeds.md
Show outdated Hide outdated docs/guides/embeds.md
Show outdated Hide outdated docs/guides/embeds.md
@pagarwal123

This comment has been minimized.

Show comment
Hide comment
@pagarwal123

pagarwal123 Nov 7, 2017

Contributor

QA-LGTM

Contributor

pagarwal123 commented Nov 7, 2017

QA-LGTM

@gkatsev gkatsev merged commit d8aadd5 into master Nov 13, 2017

4 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@gkatsev gkatsev removed the needs: LGTM label Nov 13, 2017

@gkatsev gkatsev deleted the div-embed branch Nov 13, 2017

gkatsev added a commit that referenced this pull request Nov 16, 2017

perf: null out els on dispose to minimize detached els (#4745)
A we retained a lot of references to DOM elements in various components. Here we clear it up. Also, make sure that we remove unused listeners as they can retain objects as well.
Update evented mixin to null out the eventBusEl_ after the component is disposed.
Add a feature for components, to tell it not to auto-initialize the evented mixin.
Re-enable the tests that were removed in #4640.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment