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

fix: player controls #96

Merged
merged 2 commits into from
Jun 8, 2018
Merged

fix: player controls #96

merged 2 commits into from
Jun 8, 2018

Conversation

brandonocasey
Copy link
Contributor

Fixes #92

Right now its now a lot of the default player behaviors are overridden by the other controls. This pr brings them back and makes them work how we would expect.

  1. A click/tap with no movement (ie not moving around the scene) should toggle playback
  2. When moving around the scene the control bar should not come up. It should only come up when playback is toggled or the control bar is moved over/clicked.
  3. The context menu from right click should show up.

@@ -39,6 +39,11 @@ export default function(options) {

code = globalReplace(code, 'rotateLeft', 'scope.rotateLeft');
code = globalReplace(code, 'rotateUp', 'scope.rotateUp');
// comment out the context menu prevent default line...
code = globalReplace(code,
"scope.domElement.addEventListener\\( 'contextmenu'",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comments out https://github.com/mrdoob/three.js/blob/dev/examples/js/controls/OrbitControls.js#L226 so that we don't preventDefault on the contextmenu. It makes sense to do this if you are using pan controls with right click, but we are not.

@brandonocasey brandonocasey force-pushed the fix/player-controls branch from c72450e to 8ace4ad Compare May 31, 2018 20:03
@ishita12
Copy link

ishita12 commented Jun 6, 2018

QA LGTM


onMoveStart(e) {

// if the player does not have a controlbar or
Copy link
Member

Choose a reason for hiding this comment

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

Can this use Video.js's tap event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me see if that works, looks like its probably a better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK so I think that it would be too large of a refactor to convert this to a component and use the tap event. I think this will have to happen in another pr.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good.

// video.js listens for user activity on the video element
// and makes the user active when the mouse moves.
// We don't want that for 3d videos
this.oldReportUserActivity = this.player.reportUserActivity;
Copy link
Member

Choose a reason for hiding this comment

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

not a huge fan of these but I don't think there's currently a way to disable it. Maybe we should have one?
Though, I guess this is also fairly self-contained and restores it on dispose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought about adding one, but it seems like this plugin would be the only use case.

@@ -497,7 +498,9 @@ class VR extends Plugin {
canvas: this.renderedCanvas,
orientation: videojs.browser.IS_IOS || videojs.browser.IS_ANDROID || false
});
this.canvasPlayerControls = new CanvasPlayerControls(this.player_, this.renderedCanvas);
Copy link
Member

Choose a reason for hiding this comment

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

should we add this as a child of the player rather than just instantiating it here?

Copy link
Member

Choose a reason for hiding this comment

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

oh, it's not a real component, just an object.

@brandonocasey brandonocasey merged commit 21b66ca into master Jun 8, 2018
@brandonocasey brandonocasey deleted the fix/player-controls branch June 8, 2018 15:11
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