-
Notifications
You must be signed in to change notification settings - Fork 189
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 in safari scroll overflow #228
base: master
Are you sure you want to change the base?
fix in safari scroll overflow #228
Conversation
Codecov Report
@@ Coverage Diff @@
## master #228 +/- ##
==========================================
+ Coverage 53.67% 54.20% +0.53%
==========================================
Files 41 41
Lines 857 867 +10
Branches 304 306 +2
==========================================
+ Hits 460 470 +10
Misses 341 341
Partials 56 56
Continue to review full report at Codecov.
|
@shaodahong , have you got some comment to review? |
src/Dom/scrollLocker.ts
Outdated
overflowY: 'hidden', | ||
}; | ||
|
||
if (container === document.body) { |
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.
Can we judge mobile?
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.
@shaodahong , what do you mean? I don't understand, in a mobile view, body is also present in the DOM
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.
Maybe should if (isMobile && container === document.body)
.
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.
@shaodahong , It's bug actually for safari browser, for example, on IPhone (mobile device ) and IPad (tablet device) this defect is present, and i checked work of these devices, but don't know about Mac
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.
In my desktop browser, everything is normal, so my suggestion is to add this fix to the mobile browser.
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.
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.
@shaodahong , I checked on IPad with isMobile() flag and work incorrect
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.
@shaodahong on Tablet devices isMobile() -> return false
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.
I checked on IPad with isMobile() flag and work incorrect
Yep, so we need judge like this
const isIosDevice = (/iP(ad|hone|od)/.test(window.navigator.platform)
if (isIosDevice) {
//...
}
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.
@shaodahong, on IPad devices now platform return MacIntel
This reverts commit 7745599
@@ -17,6 +18,32 @@ const scrollingEffectClassNameReg = new RegExp( | |||
'g', | |||
); | |||
|
|||
let scrollPosition = 0; | |||
const isIosDevice = /iP(ad|hone|od)|MacIntel/.test(window.navigator.platform); |
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.
We need support ssr, so
const isIosDevice = /iP(ad|hone|od)|MacIntel/.test(window.navigator.platform); | |
const isIosDevice = canUseDom() && window.navigator && | |
window.navigator.platform && | |
(/iP(ad|hone|od)/.test(window.navigator.platform) || | |
(window.navigator.platform === 'MacIntel' && window.navigator.maxTouchPoints > 1))`; |
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.
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.
Ping @PavelRazumov
ant-design/ant-design#23202