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

Responsive: incompatible change using document.documentElement.clientWidth #2614

Closed
andrewferk opened this issue Mar 7, 2018 · 3 comments
Closed

Comments

@andrewferk
Copy link
Contributor

andrewferk commented Mar 7, 2018

Steps

  1. Ensure scrollbars are enabled for your OS or browser
  2. Render <Responsive maxWidth={767}>Only mobile</Responsive>
  3. Set the window width to 768px

Expected Result

The "Only mobile" text does not render/display.

Actual Result

The "Only mobile" text does render/display.

Version

0.78.3

My analysis for the issue

This is due to #2531, which updated Responsive to use document.documentElement.clientWidth, instead of window.innerWidth. They are very technically different measurements, and the change has introduced the unexpected result I showed above. This is because clientWidth does not include the width of the scrollbar in the measurement, so it appears the width is 768px - 17px. Also, I have many tests that rely on stubbing window.innerWidth, and 0.78.3 has broken them all.

My proposal to fix the issue and more

  1. Revert fix(Responsive): use root element client width #2531 and use window.innerWidth as the default way of measuring width for the Responsive component. Semantic UI's responsive breakpoints should take into account the scrollbar, which document.documentElement.clientWidth does not. Without reverting back, the use of <Responsive {...Responsive.mobileOnly} />, etc. will not work as expected.
  2. Add a new property to Responsive that allows developers to use a custom measurement for the width: <Responsive getWidth={() => document.documentElement.clientWidth} />. This would allow @autarc measure what he wants without introducing the backwards incompatible change.
  3. Fix the false-positive tests in Responsive-test.js. I have noticed some of the Responsive tests aren't testing the right thing, and they are passing when they should not. I'll fix this and explain what was wrong.
  4. Only set state when width actually changes. Right now, every window.resize event causes Responsive to set the width, causing unneeded renders. This is mostly notable on mobile devices which hide and show the URL bar at the top on vertical scrolling. Scrolling on mobile devices that cause the URL bar to show/hide, triggers a window.resize event and Responsive renders, when not needed.
@welcome
Copy link

welcome bot commented Mar 7, 2018

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you've completed all the fields in the issue template so we can best help.

We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

@andrewferk andrewferk changed the title Recent backwards incompatible change to Responsive component using document.documentElement.clientWidth Recent backwards incompatible change to Responsive component using document.documentElement.clientWidth Mar 7, 2018
@layershifter
Copy link
Member

Looks awesome, feel free to open PR.

@layershifter layershifter changed the title Recent backwards incompatible change to Responsive component using document.documentElement.clientWidth Responsive: incompatible change using document.documentElement.clientWidth Mar 11, 2018
@levithomason
Copy link
Member

Fixed in #2621

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants