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

(doc) Adds guide for removing players from the page #1803

Closed
wants to merge 6 commits into from
Closed

(doc) Adds guide for removing players from the page #1803

wants to merge 6 commits into from

Conversation

brycefisher
Copy link
Contributor

Added a guide to explain how to safely remove players from the page.

Ran into a situation like this:
http://jsbin.com/gahususoju

I couldn't find this info anywhere in the guides or documentation, so thought I'd chip in :-)

Added a guide to explain how to safely remove players from the page
Added link to removing players guide
@brycefisher brycefisher changed the title Create removing-players.md (doc) Adds guide for removing players from the page Jan 14, 2015
@gkatsev
Copy link
Member

gkatsev commented Jan 15, 2015

LGTM. @mmcc?

Or...Use Unique Ids
-------------------

If you prefer not to call `dispose()` on a player, you can always create new players with unique ids for that page load.
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably suggest just removing this bit altogether. Any time you remove a player from the DOM (or hide it, or just don't need it anymore), it's a good idea to remove it, even if just from a resource management perspective.

@mmcc
Copy link
Member

mmcc commented Jan 15, 2015

Looks great! The only other thing I'd suggest is adding a section about hiding / showing a player. For instance, if you've got a modal that a player pops up in, you should create and dispose the player when the player shows / hides. If the Flash tech is used and you try to hide it, things will go poorly because, Flash.

Thanks for the PR!

@brycefisher
Copy link
Contributor Author

Thanks for the extra info! I'll update the branch with your feedback.

@brycefisher
Copy link
Contributor Author

If this looks good to you @gkatsev and @mmcc, I'll squash this down into a single commit in a new PR

@gkatsev
Copy link
Member

gkatsev commented Jan 15, 2015

Probably no need, since we squash it up during out merge process.

@brycefisher
Copy link
Contributor Author

okey-dokey!

@mmcc mmcc closed this Jan 16, 2015
mmcc pushed a commit to mmcc/video.js that referenced this pull request Jan 16, 2015
@brycefisher
Copy link
Contributor Author

Woot! Thanks for accepting my PR @mmcc and @gkatsev

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.

3 participants