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

add warning, if the element given to Video.js is not in the DOM #4698

Merged
merged 2 commits into from Nov 1, 2017

Conversation

Projects
None yet
4 participants
@odisei369
Contributor

odisei369 commented Oct 28, 2017

Description

Issue #4697

Specific Changes proposed

Add warning, if the element given to Video.js is not in the DOM

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors
@gkatsev

Awesome, thanks @odisei369!

Show outdated Hide outdated src/js/video.js
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Oct 31, 2017

Member

@odisei369 hey, would you be up for adding a test for this? You can use this test as an example. Basically, it'll involve mocking log.warn, creating a new player with a random video element, asserting that log.warn was called with the value we want, and then disposing the player we created. If you have questions or concerns, please let me know.

Member

gkatsev commented Oct 31, 2017

@odisei369 hey, would you be up for adding a test for this? You can use this test as an example. Basically, it'll involve mocking log.warn, creating a new player with a random video element, asserting that log.warn was called with the value we want, and then disposing the player we created. If you have questions or concerns, please let me know.

@gkatsev gkatsev merged commit 6f713ca into videojs:master Nov 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@odisei369

This comment has been minimized.

Show comment
Hide comment
@odisei369

odisei369 Nov 1, 2017

Contributor

@gkarsev OK, I'll add the test. With example it would be easy.

Contributor

odisei369 commented Nov 1, 2017

@gkarsev OK, I'll add the test. With example it would be easy.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Nov 1, 2017

Member

@odisei369 thanks! I just merged this PR, so, please make another one when you do :)

Member

gkatsev commented Nov 1, 2017

@odisei369 thanks! I just merged this PR, so, please make another one when you do :)

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Nov 1, 2017

Member

And again, if you have questions, feel free to post them there or stop by and ask on slack http://slack.videojs.com :)

Member

gkatsev commented Nov 1, 2017

And again, if you have questions, feel free to post them there or stop by and ask on slack http://slack.videojs.com :)

@odisei369 odisei369 referenced this pull request Nov 6, 2017

Merged

test: warning, if the element is not in the DOM #4723

0 of 7 tasks complete

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment