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

Device.desktop incorrectly reports 1025px as mobile #635

Open
sperks-syatt opened this issue Apr 17, 2022 · 0 comments
Open

Device.desktop incorrectly reports 1025px as mobile #635

sperks-syatt opened this issue Apr 17, 2022 · 0 comments

Comments

@sperks-syatt
Copy link

sperks-syatt commented Apr 17, 2022

Describe the bug
When the platform was first developed it was determined to support iPads' 1024px viewport with the mobile layout in landscape orientation. In doing this, it was common to use the css media query of min-width: 1024.1px to target desktop viewport because the :not() pseudo-selector wasn't fully supported. However, this was somewhat of a hack, and in JavaScript it would have been more precise to determine the mobile viewport with mobile = maxWidth: 1024px and negate that value for determining desktop. The reason for this is that the hack will always leave you exposed to errors in the future which happened when vtex changed the base unit to rem.

When Changing to rem units the logic for determining what the desktop size was changed from:
const isScreenLarge = useMediaLayout({ minWidth: '1024.1px' }) to
const isScreenLarge = useMediaLayout({ minWidth: '64.1rem' }) which unfortunately bumped the min-width to 1025.6px

To Reproduce
Steps to reproduce the behavior:

  1. Go to any vtex site that uses header-layout.desktop / header-layout.mobile
  2. Resize your browser to 1025px width
  3. Note that the mobile content is being rendered

Expected behavior
I would have expected that the desktop content would be displayed considering that vtex is configured through all other components to have a desktop breakpoint of 64rem

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
The change I'm suggesting is only in order to correct the 1025px breakpoint, however, I've noticed that there are inconsistencies in vtex's overall approach to supporting this 1024px viewport and the introduction of tachyons. I would strongly suggest that vtex considers a major bump to support a mobile first development practice that matches the tachyon approach so that all viewports are min-width: xx where xx is a whole number and is thus included in the larger viewport (1024px becoming desktop in this example).

Suggested temporary fix (a more permanent fix would be to support the tachyon styles config):

  const isTabletScreen = useMediaLayout({ minWidth: '40rem' })
  const isMobileScreen = useMediaLayout({ maxWidth: '64rem' })

  const clientDevice = {
    type: isMobileScreen
      ? isTabletScreen
        ? Device.tablet
        : Device.phone
      : Device.desktop,
    isMobile: isMobileScreen,
  }
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

No branches or pull requests

1 participant