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

Update tablet detection #4724

Merged
merged 2 commits into from
Oct 13, 2021
Merged

Update tablet detection #4724

merged 2 commits into from
Oct 13, 2021

Conversation

keianhzo
Copy link
Contributor

@keianhzo keianhzo commented Oct 11, 2021

Fixes #3261

Our internal and AFrame's tablet detection seems to be outdated. I've updated it and now we are detecting tablet mobile browsers.

@takahirox I don't have iOS devices so I've only tried in Android tablets.

Copy link
Contributor

@takahirox takahirox left a comment

Choose a reason for hiding this comment

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

Thanks for working on the issue.

But unfortunately Hubs crashes with Uncaught ReferenceError: AFRAME is not defined error.

image

I confirmed it on the dev branch and hubs.local:8080 with npm run dev.

Curious to know if we really need to override A-Frame isMobile(). Is it called in A-Frame internal code we use?

@keianhzo
Copy link
Contributor Author

keianhzo commented Oct 12, 2021

It seems that is-mobile is also being used in the homepage. I've fixed it.

All the methods exposed in is-mobile are copies of methods at https://github.com/aframevr/aframe/blob/master/src/utils/device.js.

We use AFRAME.utils.device.isMobile() in all our code but in one case: https://github.com/mozilla/hubs/blob/b07c5ce3e3aac027dfb2f36e68c6b517f46652a8/src/react-components/home/PWAButton.js#L5 but that makes it hard to keep consistency throughout the app and also hard to update the regular expressions as we don't really update AFRAME that often. As we use mainly AFRAME.utils.device.isMobile() it made sense to me to override the AFRAME implementation.

Would also make sense to override the AFRAME implementation for other methods that we have updated in is-mobile like isIOS()?. We don't really use the AFRAME implementation of those in our code but I'm concerned about AFRAME internal calls to those methods not being consistent with Hubs calls to our implementation.

@takahirox
Copy link
Contributor

I think it would be good if we can replace A-Frame function in our code with our code like from AFRAME.utils.device.isMobile() to isMobile() to reduce the dependency with A-Frame because we try to be independent of A-Frame as much as possible.

If internal A-Frame code we still use calls AFRAME.utils.device.isMobile(), I think it would be better to update our A-Frame repo rather than overriding it in our code.

We don't really want to apply any changes in Three.js code for maintenance we we override some Three.js methods in our code. But we rarely (or will never) upgrade A-Frame so I don't think updating our A-Frame code is a big deal.

@keianhzo
Copy link
Contributor Author

@takahirox not sure if I follow but I'm going to land this and we can refactor if we feel that's what we want.

@keianhzo keianhzo merged commit 6567ebd into master Oct 13, 2021
@keianhzo keianhzo deleted the fix-tablet-detection branch October 13, 2021 15:12
@takahirox
Copy link
Contributor

@keianhzo

not sure if I follow

I suggested either one

  1. Replacing AFRAME.utils.device.isMobile()in our code with our isMobile() like

from

if (AFRAME.utils.device.isMobile()) {
  ...
}

to

import isMobile from './src/utils/is-mobile.js';
if (isMobile()) {
  ...
}
  1. Update AFRAME.utils.device.isMobile() in https://github.com/MozillaReality/aframe

Because I'm not sure if overriding A-Frame function is a good design.

@keianhzo
Copy link
Contributor Author

@takahirox

  1. Replacing AFRAME.utils.device.isMobile()in our code with our isMobile() like

This won't update the internal AFRAME calls so we would end up with two different implementations of isMobile (and the rest of the AFRAME utild functions that we duplicate) https://github.com/search?q=repo%3Aaframevr%2Faframe+extension%3Ajs+path%3A%2Fsrc+isMobile&type=Code

  1. Update AFRAME.utils.device.isMobile() in https://github.com/MozillaReality/aframe

I would say that to make sure that the AFRAME utils methods implementations are consistent between AFRAME and Hubs we should go for your you second suggestion and update our AFRAME fork and always use those utility methods instead of duplicating and pointing to our implementation.

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.

I'm using a tablet, but it asks me to move with keyboard
2 participants