-
Notifications
You must be signed in to change notification settings - Fork 487
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 onLaneScroll not working on different screen sizes #422 #431
Conversation
src/controllers/Lane.js
Outdated
@@ -26,7 +26,7 @@ class Lane extends Component { | |||
const node = evt.target | |||
const elemScrollPosition = node.scrollHeight - node.scrollTop - node.clientHeight | |||
const {onLaneScroll} = this.props | |||
if (elemScrollPosition <= 0 && onLaneScroll && !this.state.loading) { | |||
if (elemScrollPosition <= 1 && onLaneScroll && !this.state.loading) { |
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.
Would < 1
also work here?
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.
Yes, that'd work too. Normally the difference is never going to be 1 or 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.
Hi guys! It's very cool that you fixes that issue!
I believe it's better to describe this line and put a link to the issue. Because if you do not know the reason, then this code looks strange.
Also I prefer < 1
)
src/controllers/Lane.js
Outdated
@@ -26,7 +26,7 @@ class Lane extends Component { | |||
const node = evt.target | |||
const elemScrollPosition = node.scrollHeight - node.scrollTop - node.clientHeight | |||
const {onLaneScroll} = this.props | |||
if (elemScrollPosition <= 0 && onLaneScroll && !this.state.loading) { | |||
if (elemScrollPosition <= 1 && onLaneScroll && !this.state.loading) { |
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.
Hi guys! It's very cool that you fixes that issue!
I believe it's better to describe this line and put a link to the issue. Because if you do not know the reason, then this code looks strange.
Also I prefer < 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.
@oliviervanbulck thank!
Will merge if @corvec agree )
🎉 This PR is included in version 2.2.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
#422