-
-
Notifications
You must be signed in to change notification settings - Fork 829
If on trackpad, don't mess with horizontal scrolling. #3148
Conversation
trackpad heuristic is 'if 15 minutes of no horizontal scrollwheel events, assume user may have switched to mousewheel'
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.
otherwise lgtm, I think. Feels a bit more complicated than it needs to be, but it does appear to do things.
@@ -152,7 +164,7 @@ export default class IndicatorScrollbar extends React.Component { | |||
const additionalScroll = e.deltaY < 0 ? -50 : 50; | |||
|
|||
// noinspection JSSuspiciousNameCombination | |||
const val = Math.abs(e.deltaY) < 25 && safeToBoost ? (e.deltaY + additionalScroll) : e.deltaY; | |||
const val = Math.abs(e.deltaY) < 25 ? (e.deltaY + additionalScroll) : 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 breaks vertical scrolling for non-smoothscrolling browsers (ie: windows)
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.
yup, this was a thinko; suggestions welcome
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.
instead of zero, e.deltaY
, like it was before
this._checkAgainForTrackpad = now + (15 * 60 * 1000); // 15min | ||
} | ||
else { | ||
// if we haven't seen any horizontal scrolling for >15 minutes, assume |
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 should bump this down to a minute if we're not sampling.
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.
sgtm
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.
lgtm, although the linter is angry. I think both linting problems would be solved by putting the else
with one line higher with the closing brace.
I am assuming @ara4n wants this to land but is busy with other things, so merging. |
The trackpad heuristic is 'if 15 minutes of no horizontal scrollwheel events, assume user may have switched to mousewheel'