Skip to content

Switch back to Browser gem (LG-5189)#5483

Merged
zachmargolis merged 9 commits intomainfrom
margolis-use-browser
Oct 8, 2021
Merged

Switch back to Browser gem (LG-5189)#5483
zachmargolis merged 9 commits intomainfrom
margolis-use-browser

Conversation

@zachmargolis
Copy link
Contributor

  • Seems to be updated more recently than DeviceDetector
    • See JIRA, examples of mobile being detected incorrectly
  • With a cache layer, performs similarly

- Seems to be updated more recently than DeviceDetector
   - See JIRA, examples of mobile being detected incorrectly
- With a cache layer, performs similarly
browser_platform_name: browser.platform.name,
browser_platform_version: browser.platform.version,
browser_device_name: browser.device.name,
browser_mobile: browser.device.mobile?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed browser_device_type to browser_mobile because that seems to be how we mostly use this property throughout the codebase

Browser gem doesn't provide a simple device type, but if we really cared, we could re-implement it similar to the way ahoy gem does:

https://github.com/ankane/ahoy/blob/68e511dde5cafe08b0a1aa59988d3b57576f02f4/lib/ahoy/visit_properties.rb#L74-L87

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I've only ever used it to query mobile or desktop, so no opposition from me 🤷

May be worth a quick glance to see if we're relying on this in any existing dashboards.

Copy link
Contributor Author

@zachmargolis zachmargolis Oct 8, 2021

Choose a reason for hiding this comment

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

A quick source code search only showed usages outside this repo in our deprecated/defunct analytics ETL pipeline

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

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

👏🏼

(also sorry I created a merge conflict in #5485)

@zachmargolis zachmargolis merged commit d76a557 into main Oct 8, 2021
@zachmargolis zachmargolis deleted the margolis-use-browser branch October 8, 2021 20:33
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.

3 participants