Skip to content
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(player): ingest a player div for videojs #3856

Merged
merged 12 commits into from
Dec 19, 2016
Merged

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Dec 13, 2016

If the videojs embed code (a video element) is wrapped in a div with the
'data-vjs-player' attribute on it, that element will be used for the
player div and a new one will not be created. In addition, on browsers
like iOS that don't support moving the media element inside the DOM, we
will not need to clone the element and we could continue to re-use the
same video element give to us in the embed code.

This could also be extended in the future to change our embed code to a
div-only approach if we so choose.

@gkatsev
Copy link
Member Author

gkatsev commented Dec 13, 2016

I guess this needs some additional unit tests.

mmcc
mmcc previously approved these changes Dec 13, 2016
Copy link
Member

@mmcc mmcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice, this should help a lot for React folks

misteroneill
misteroneill previously approved these changes Dec 14, 2016
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tentatively approved pending fixed tests and addition of a new test specifically for this feature. 😄

@gkatsev
Copy link
Member Author

gkatsev commented Dec 14, 2016

Fixing the tests we currently have is super easy. I forgot to check that tag.parentNode exists. Adding more tests is a bit more labor intensive, though.

@misteroneill
Copy link
Member

Well, fixing the existing tests I think was more important, but I do think it'd be nice to have a positive test case for the functionality. We've gotten ourselves into trouble lately with low-level player initialization assumptions! 😆

@gkatsev
Copy link
Member Author

gkatsev commented Dec 14, 2016

First up are a set of tests for movingMediaElementInDOM and that Html5's createEl method does the right thing when used together with the playerElIngest tech option. Next up is a videojs test that makes sure that create a player wrapped properly the playerElIngest option is set up correctly for the the tech.

@gkatsev gkatsev dismissed stale reviews from mmcc and misteroneill December 14, 2016 19:33

Added tests

If the videojs embed code (a video element) is wrapped in a div with the
'data-vjs-player' attribute on it, that element will be used for the
player div and a new one will not be created. In addition, on browsers
like iOS that don't support moving the media element inside the DOM, we
will not need to clone the element and we could continue to re-use the
same video element give to us in the embed code.

This could also be extended in the future to change our embed code to a
div-only approach if we so choose.
@gkatsev gkatsev force-pushed the player-div-ingestion branch from d0eabd2 to a85eb5b Compare December 15, 2016 20:51
@gkatsev
Copy link
Member Author

gkatsev commented Dec 16, 2016

@mmcc @misteroneill tests are written and passing, please review!

@VenkateshChekuri
Copy link

When I am trying to update the video by changing the source, it's not changing the video.
I am using video.js and React.js integration github code, but i am trying to change the video like this.
RenderVideo(path){
const videoJsOptions = {
autoplay: true,
controls: true,
sources: [{
src: '/path/to/video.mp4',
type: 'video/mp4'
}]
}

return <VideoPlayer { ...videoJsOptions } />

  {props.imgs[index].mediaType === 'VIDEO' && (
      <div className={styles.videoContainer}> 
             {RenderVideo(props.imgs[index].primaryLink)}
      </div>
    )}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants