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

Detect both deprecated and 1.0 WebVR API support #1374

Merged
merged 1 commit into from
Apr 14, 2016

Conversation

mkeblx
Copy link
Contributor

@mkeblx mkeblx commented Apr 14, 2016

Description:
-Naming update for WebVR 1.0 spec.

Changes proposed:
-Add check for new API method name for device querying.

// Webvr polyfill configuration.
window.hasNonPolyfillWebVRSupport = !!navigator.getVRDevices;
// WebVR polyfill configuration.
window.hasNonPolyfillWebVRSupport = !!navigator.getVRDevices || !!navigator.getVRDevices;
Copy link
Member

Choose a reason for hiding this comment

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

My eyes must be missing something, but these look the same!

@dmarcos
Copy link
Member

dmarcos commented Apr 14, 2016

@cvan Does this look good to you?

@dmarcos
Copy link
Member

dmarcos commented Apr 14, 2016

@mkeblx Can you squash your commits?

@mkeblx
Copy link
Contributor Author

mkeblx commented Apr 14, 2016

I needed this to get working in GearVR browser with 1.0 API. But saw #1132 so don't know if have plans for adding 1.0 support in a different fashion.

@ngokevin ngokevin merged commit d48cbaf into aframevr:master Apr 14, 2016
@ngokevin
Copy link
Member

Just tested out GitHub squash merging. What's our opinion on that?

@dmarcos
Copy link
Member

dmarcos commented Apr 14, 2016

I wanted @cvan feedback. I'm not sure this should have landed. We have no support for WebVR 1.0 in a-frame yet. I believe the value of hasNonPolyfillWebVRSupport is not correct after this patch.

@ngokevin
Copy link
Member

My bad, for some reason I thought the second comment was cvan's.

@cvan
Copy link
Contributor

cvan commented Apr 14, 2016

I'm actually working on this in #1132... but thanks.

@dmarcos
Copy link
Member

dmarcos commented Apr 14, 2016

@cvan this PR has been merged. Are you ok with this change or do you want to handle it differently?

@cvan
Copy link
Contributor

cvan commented Apr 14, 2016

No, it's fine. But have to change a few other things. Finishing up my PR as we speak.

pushmatrix pushed a commit to pushmatrix/aframe that referenced this pull request Apr 23, 2016
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.

4 participants