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

Avoid destroying old player when video changes #145

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ashe540
Copy link

@ashe540 ashe540 commented Feb 25, 2017

This avoids destroying the player when changing the current video. It also prevents the player to stop playing when the video is finished and when user is on a different tab or window. There are a couple of pull requests open for this, but as mentioned in issue #84 this can be solved by simply checking if the player exists before creating a new one. Hopefully we can get this solution to this old issue merged soon.

@brandly
Copy link
Owner

brandly commented Feb 25, 2017

does this handle playlists?

@ashe540
Copy link
Author

ashe540 commented Feb 26, 2017

Added support for playlists. I also included the updated demo in my fork in the gh-pages branch. Let me know what you think.

@brandly
Copy link
Owner

brandly commented Feb 26, 2017

nice! i'm gonna check this out in a bit. at first glance, this looks great!

i think this will affect how events are emitted, so i wanna verify that i understand how things behave now.

if it is changing how events are emitted, we have to treat this update as a change in functionality, and release a new major version, so there's a good chance we're looking at a 2.0.0 release with this code in it.

thanks for helping with this! i know this has been an issue for a while, and i haven't addressed it. other implementations i've seen have been a lot less direct than this one, so i appreciate you taking the time to sort things out 🌟

@ashe540
Copy link
Author

ashe540 commented Feb 26, 2017

Hey, no problem! And yeah that sounds like a good idea. Let me know if I can help out in any way! 😄

@brandly
Copy link
Owner

brandly commented Feb 27, 2017

had some time to look:

looking at the docs, i think we should use the "object syntax" for both videos and playlists.

for videos, this allows us to support startSeconds and endSeconds video playerVars.

take a look at the docs for this repo. start and end can supported here, but there are other parameters, like autoplay, that once you set, you can't take back. does that make sense?

before this PR, every video change created a brand new player object with the given playerVars. i still think this PR is a step in the right direction, but we should consider these things and see how much functionality we can continue to support.

want to take a stab at supporting startSeconds and endSeconds for videos and startSeconds for playlists?

@ashe540
Copy link
Author

ashe540 commented Feb 27, 2017

Yeah I get what you mean. I'll see what I can do about keeping those features intact while keeping the player object. You'll be hearing back from me 😄.

@brandly
Copy link
Owner

brandly commented Apr 11, 2017

@ashe540 hey! any updates?

@ashe540
Copy link
Author

ashe540 commented Apr 12, 2017

@brandly so sorry for the long silence. Unfortunately I have been terribly busy and haven't had a chance to work on this. I'm going to try to make some time during the next week or so. I'll keep you posted.

@brandly
Copy link
Owner

brandly commented Apr 12, 2017 via email

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.

2 participants