-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Conversation
There was a problem hiding this 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'); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 |
@gkarsev OK, I'll add the test. With example it would be easy. |
@odisei369 thanks! I just merged this PR, so, please make another one when you do :) |
And again, if you have questions, feel free to post them there or stop by and ask on slack http://slack.videojs.com :) |
Description
Issue #4697
Specific Changes proposed
Add warning, if the element given to Video.js is not in the DOM
Requirements Checklist