-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Improving detection of scroll ended #742
Changes from 7 commits
c20bf15
d534319
aa4bcfd
a706712
14678f8
089af39
3264b83
c47959f
d782956
a67e150
477ac94
cf2f8d0
c2e1d93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -289,6 +289,7 @@ export default class Grid extends PureComponent { | |
); | ||
this._onScroll = this._onScroll.bind(this); | ||
this._setScrollingContainerRef = this._setScrollingContainerRef.bind(this); | ||
this._delayScrollEnded = this._delayScrollEnded.bind(this); | ||
|
||
this._columnWidthGetter = this._wrapSizeGetter(props.columnWidth); | ||
this._rowHeightGetter = this._wrapSizeGetter(props.rowHeight); | ||
|
@@ -1036,21 +1037,33 @@ export default class Grid extends PureComponent { | |
} | ||
} | ||
|
||
/** | ||
* Check if the difference between current time and the last scroll ended event is greater. | ||
* than the scrollingResetTimeInterval prop, else schedule this function to execute again. | ||
*/ | ||
_delayScrollEnded() { | ||
const { scrollingResetTimeInterval } = this.props; | ||
if (Date.now() - this._scrollDebounceStart >= scrollingResetTimeInterval) { | ||
this._debounceScrollEndedCallback(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor nit: I realize we weren't doing this before, and it's no big deal, but we might as well clear There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree that the pointer must be cleared, but this hasn't been doing on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right 😄 Sorry, I was just skimming the diff and that callback wasn't visible. Sorry~ 👍 |
||
} else { | ||
this._disablePointerEventsTimeoutId = window.requestAnimationFrame( | ||
this._delayScrollEnded | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Sets an :isScrolling flag for a small window of time. | ||
* This flag is used to disable pointer events on the scrollable portion of the Grid. | ||
* This prevents jerky/stuttery mouse-wheel scrolling. | ||
*/ | ||
_debounceScrollEnded() { | ||
const { scrollingResetTimeInterval } = this.props; | ||
|
||
if (this._disablePointerEventsTimeoutId) { | ||
clearTimeout(this._disablePointerEventsTimeoutId); | ||
window.cancelAnimationFrame(this._disablePointerEventsTimeoutId); | ||
} | ||
|
||
this._disablePointerEventsTimeoutId = setTimeout( | ||
this._debounceScrollEndedCallback, | ||
scrollingResetTimeInterval | ||
this._scrollDebounceStart = Date.now(); | ||
this._disablePointerEventsTimeoutId = window.requestAnimationFrame( | ||
this._delayScrollEnded | ||
); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,6 +75,7 @@ export default class Masonry extends PureComponent { | |
); | ||
this._setScrollingContainerRef = this._setScrollingContainerRef.bind(this); | ||
this._onScroll = this._onScroll.bind(this); | ||
this._delayScrollEnded = this._delayScrollEnded.bind(this); | ||
} | ||
|
||
clearCellPositions() { | ||
|
@@ -290,16 +291,28 @@ export default class Masonry extends PureComponent { | |
} | ||
} | ||
|
||
_debounceResetIsScrolling() { | ||
/** | ||
* Check if the difference between current time and the last scroll ended event is greater. | ||
* than the scrollingResetTimeInterval prop, else schedule this function to execute again. | ||
*/ | ||
_delayScrollEnded() { | ||
const { scrollingResetTimeInterval } = this.props; | ||
if (Date.now() - this._scrollDebounceStart >= scrollingResetTimeInterval) { | ||
this._debounceScrollEndedCallback(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto as for |
||
} else { | ||
this._disablePointerEventsTimeoutId = window.requestAnimationFrame( | ||
this._delayScrollEnded | ||
); | ||
} | ||
} | ||
|
||
_debounceResetIsScrolling() { | ||
if (this._debounceResetIsScrollingId) { | ||
clearTimeout(this._debounceResetIsScrollingId); | ||
window.cancelAnimationFrame(this._debounceResetIsScrollingId); | ||
} | ||
|
||
this._debounceResetIsScrollingId = setTimeout( | ||
this._debounceResetIsScrollingCallback, | ||
scrollingResetTimeInterval | ||
this._scrollDebounceStart = Date.now(); | ||
this._debounceResetIsScrollingId = window.requestAnimationFrame( | ||
this._delayScrollEnded | ||
); | ||
} | ||
|
||
|
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.
Conflict can be solved If you will use class property here instead of bind
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, I'm merging right now.
I was just getting a coffee 😄