MWPW-155489 [PEP] Add device type as PEP criteria #2931
Conversation
sharmrj
requested review from
salonijain3,
bandana147,
nishantka,
Deva309 and
sonawanesnehal3
as code owners
|
1 similar comment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2931 +/- ##
==========================================
- Coverage 96.24% 96.23% -0.01%
==========================================
Files 236 236
Lines 54253 54281 +28
==========================================
+ Hits 52216 52239 +23
- Misses 2037 2042 +5 ☔ View full report in Codecov by Sentry. |
|
Nala tests seem to be failing for a reason unrelated to my changes |
| @@ -637,9 +637,12 @@ class Gnav { | |||
| const state = getMetadata('app-prompt')?.toLowerCase(); | |||
| const entName = getMetadata('app-prompt-entitlement')?.toLowerCase(); | |||
| const promptPath = getMetadata('app-prompt-path')?.toLowerCase(); | |||
| const hasMobileUA = /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini|Touch/i.test(navigator.userAgent); | |||
There was a problem hiding this comment.
nit: wouldn't isMobile be clearer?
There was a problem hiding this comment.
We have checks like isDesktop that check viewport size, which is what we check by default. I'd like to keep this name to differentiate it from the viewport size checks.
|
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
|
Validation has been done on user agent , actual device testing can be done only once the pr gets merged to stage. Details are updated in |
| @@ -637,9 +637,12 @@ class Gnav { | |||
| const state = getMetadata('app-prompt')?.toLowerCase(); | |||
| const entName = getMetadata('app-prompt-entitlement')?.toLowerCase(); | |||
| const promptPath = getMetadata('app-prompt-path')?.toLowerCase(); | |||
| const hasMobileUA = /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini|Touch/i.test(navigator.userAgent); | |||
There was a problem hiding this comment.
Is there any analytics data pointing to all of these being used by our users? The iPod was discontinued more than 2 years ago. Blackberry, IEMobile and Touch don't seem to be all that popular either according to this.
PEP now checks if the the useragent is one of a number of mobile devices.
Due to our specific viewport breakpoints, PEP was being served to iPhone/iPad users browsing with their device held horizontally. This lead to a requirement for serving PEP only to desktop users.
Note: As MDN recommends, it's preferable to not do useragent sniffing and prefer feature detection instead. In our case this would mean, if we're redirecting to a web app, do the feature detection for that particular webapp before we decide to redirect the user to that webapp. But this is a more complicated problem to solve and it'll need to be looked into (if it's even worth the effort).
Resolves: MWPW-155489
Test URLs: