-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix/separate big play button #11
Conversation
@@ -351,6 +378,18 @@ const initPlugin = function(player, options) { | |||
return; | |||
} | |||
|
|||
player.removeChild('BigPlayButton'); |
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.
we should null (or set to the vr bpb) out player.bigPlayButton
as well and restore it on reset
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.
👍
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.
Another comment. I also still need to review #10, I only looked at the commits for the BPB changes here.
src/plugin.js
Outdated
const videoEl = player.el().getElementsByTagName('video')[0]; | ||
const container = player.el(); | ||
const defaultProjection = settings.projection; | ||
const oldBigPlayButton = player.BigPlayButton; |
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 should be bigPlayButton
.
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.
Also, we probably don't need to store the old one. If it wasn't removed, we can just get the reference via getChild
and then set it reset
. One less thing to track ourselves here.
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.
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.
Bug given that we add a new one in reset
, we don't need to store a reference here.
8b056a3
to
9b58df0
Compare
9b58df0
to
02ecafb
Compare
chore: Swap webvr for webxr polyfill packages
Fixes #7
Based off of #10