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

Use component event handlers insted of bound a-scene methods #3213

Merged
merged 1 commit into from
Nov 1, 2017

Conversation

dmarcos
Copy link
Member

@dmarcos dmarcos commented Nov 1, 2017

It is more hygienic to use dedicated event handlers than directly wrapped methods of a-scene. This became a problem with some experiments that were attempting at wrapping a-scene methods.

@@ -55,10 +61,10 @@ module.exports.Component = registerComponent('vr-mode-ui', {
if (this.enterVREl || this.orientationModalEl) { return; }

// Add UI if enabled and not already present.
this.enterVREl = createEnterVRButton(this.enterVR);
this.enterVREl = createEnterVRButton(this.onEnterVRButtonClick.bind(this));
Copy link
Member

Choose a reason for hiding this comment

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

I'd bind in init

* Create a modal that tells mobile users to orient the phone to landscape.
* Add a close button that if clicked, exits VR and closes the modal.
* Creates a modal dialog to request the user to switch to landscape orientation.
* *
Copy link
Member

Choose a reason for hiding this comment

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

double star

@@ -46,6 +44,14 @@ module.exports.Component = registerComponent('vr-mode-ui', {
bind(this.toggleOrientationModalIfNeeded, this));
},

onModalClick: function () {
Copy link
Member

Choose a reason for hiding this comment

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

docstrings (Exit VR when modal clicked. - Enter VR when VR button clicked.)

@dmarcos dmarcos merged commit d3c5220 into aframevr:master Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants