-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(Responsive): use root element client width #2531
fix(Responsive): use root element client width #2531
Conversation
💖 Thanks for opening this pull request! 💖 Here is a list of things that will help get it across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
Codecov Report
@@ Coverage Diff @@
## master #2531 +/- ##
==========================================
- Coverage 99.74% 99.74% -0.01%
==========================================
Files 160 154 -6
Lines 2744 2712 -32
==========================================
- Hits 2737 2705 -32
Misses 7 7 Continue to review full report at Codecov.
|
I wish we had mobile tests :) Thanks for the investigation and contribution. |
src/addons/Responsive/Responsive.js
Outdated
@@ -60,7 +60,7 @@ export default class Responsive extends Component { | |||
constructor(...args) { | |||
super(...args) | |||
|
|||
this.state = { width: isBrowser() ? window.innerWidth : 0 } | |||
this.state = { width: isBrowser() ? document.documentElement.clientWidth : 0 } |
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.
This is probably worth adding an inline comment with the GitHub issue URL and a one-line description as to why this is being used.
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.
Okay added a comment and reference to the PR. Will be squashed if it's fine :)
Released in |
* fix(Responsive): use root element client width * Add PR reference
There is currently an issue with the Responsive component regarding the expected rendering behavior. While the element should render if it's visible the
fitsMinWidth
andfitsMaxWidth
can return miss matches depending on the state of mobile devices. Zooming on webkit based browser at iOS (mobile Safari, Google Chrome) triggers theresize
event at whichwindow.innerWidth
returns an incorrect value.By using the measurements of the root element of the document (document.documentElement) rather than the window viewport the detection is more reliable. Instead of capturing the modified width during gestures like pinch to zoom the actual one of the document will be used.