-
Notifications
You must be signed in to change notification settings - Fork 174
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
MWPW-158059 [MEP] PHONE_SIZE limit is too large to exclude smaller tablets #2854
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2854 +/- ##
=======================================
Coverage 96.09% 96.09%
=======================================
Files 215 215
Lines 53950 53969 +19
=======================================
+ Hits 51841 51860 +19
Misses 2109 2109 ☔ View full report in Codecov by Sentry. |
@@ -7,7 +7,7 @@ import { | |||
import { getEntitlementMap } from './entitlements.js'; | |||
|
|||
/* c8 ignore start */ | |||
const PHONE_SIZE = window.screen.width < 768 || window.screen.height < 768; | |||
const PHONE_SIZE = window.screen.width < 600 || window.screen.height < 600; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 600
as breakpoint would exclude some smartphones when on landscape mode. Typically the smallest tablet has 768
of width (iPad mini), why not use 767
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why the line checks width and height. Either being smaller avoids miscategorizing a device in landscape. Essentially, I trying to check that the SMALLEST dimension is less than our checkpoint.
Someone pointed me to a site that lists the viewports for a bunch of devices and I've updated to go down to 550 instead. Trying to catch the smaller tablets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I should mention. This came up in reference to a specific use case: Samsung Galaxy tablets. They are smaller than iPads.
Note: this is listed a high priority because it is needed for MAX.
Detailed Description: experience columns using tablet are not catching smaller tablets
................................
URL: https://main--cc--adobecom.hlx.page/products/photoshop-lightroom/features?target=off&mep=
................................
Steps to Reproduce:
Expected Results: MEP button indicates Android & Tablet is chosen
................................
Actual Results: MEP button indicates mobile-device is chosen
Resolves: MWPW-158059
Test URLs: