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

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

Merged
merged 2 commits into from
Nov 1, 2017
Merged

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

merged 2 commits into from
Nov 1, 2017

Conversation

odisei369
Copy link
Contributor

@odisei369 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

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @odisei369!

src/js/video.js Outdated
@@ -99,6 +99,11 @@ function videojs(id, options, ready) {
throw new TypeError('The element or ID supplied is not valid. (videojs)');
}

// Check if element is included in the DOM
if (Dom.isEl(tag) && !document.body.contains(tag)) {
log.warn('The element supplied is\'t included in the DOM');
Copy link
Member

Choose a reason for hiding this comment

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

This works, though, we tend to switch to double quotes (") if we're including single quotes (') in the text, that way we don't need to escape it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkatsev Thank you. Of course it would look better.
Now I also can see misprint in "isN't".
What should I do with that? Make another commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you push another commit to your branch the PR will update.

@gkatsev
Copy link
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.

@odisei369
Copy link
Contributor Author

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

@gkatsev
Copy link
Member

gkatsev commented Nov 1, 2017

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

@gkatsev
Copy link
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 :)

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