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

Extend mobile browser detection #2440

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Extend mobile browser detection #2440

merged 1 commit into from
Oct 6, 2015

Conversation

tremby
Copy link

@tremby tremby commented Oct 5, 2015

On top of the existing detections of Android and IOS, detect Windows Phone, IE Mobile, Blackberry and BB10.

As discussed in #1848 and #2439

@tremby
Copy link
Author

tremby commented Oct 5, 2015

One thing I question is whether the tests should really be case-insensitive. I would lean towards them not being. Maybe it doesn't make much difference.

if /iP(od|hone)/i.test(window.navigator.userAgent)
return false
if /Android/i.test(window.navigator.userAgent)
return false if /Mobile/i.test(window.navigator.userAgent)
if /IEMobile/i.test(navigator.userAgent)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you're missing a window. on all of these conditions.

Copy link
Author

Choose a reason for hiding this comment

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

It would still work, would it not? window being the global object and all. But I'll add it for consistency.

Copy link
Author

Choose a reason for hiding this comment

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

Replaced the commit.

On top of the existing detections of Android and IOS, detect Windows
Phone, IE Mobile, Blackberry and BB10.

Original patch in JS by Stephane, converted to CS and committed by Bart
Nagel <bart@tremby.net>.

Closes #1848 and #2439.
@tjschuck
Copy link
Member

tjschuck commented Oct 6, 2015

This looks good to me, but hopefully we can get a 👍 from a more qualified @harvesthq/chosen-developers -- maybe @koenpunt if we're lucky?

@koenpunt
Copy link
Collaborator

koenpunt commented Oct 6, 2015

I'm not aware of the actual user agent strings, but this seems good to me

tjschuck added a commit that referenced this pull request Oct 6, 2015
@tjschuck tjschuck merged commit adf64cc into harvesthq:master Oct 6, 2015
@tjschuck
Copy link
Member

tjschuck commented Oct 6, 2015

Thanks @tremby and @StephKoenig (and @koenpunt)!

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