-
Notifications
You must be signed in to change notification settings - Fork 161
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
Stop scrolling when user is scrolling #7
Comments
will look into this - it may be as simple as adding a |
@callmecavs What about the IMO, this will allow the most flexibility without needing to understand jumps implementation. function cancelJump() {
// add requestID to top most variable list.
cancelAnimationFrame(requestID);
} // save the requestID to enable cancelAnimationFrame
requestID = requestAnimationFrame(loop) |
You could just add this in jumper let scrollWheelTouched = false;
const onWheelListener = function(e) {
scrollWheelTouched = true;
window.removeEventListener("wheel", onWheelListener);
};
window.addEventListener("wheel", onWheelListener); And then do a check in the loop // check progress
(timeElapsed < duration && !scrollWheelTouched)
? requestAnimationFrame(loop) // continue scroll loop
: done() That's how I did it in simplesmoothscroll and it has worked great. |
Though that solution works for mouse scroll, there may be other events that should cancel the scroll as well. I would hate to add that in to the jump.js library because then we would have to add a config option and event handler for every case. For example, what about canceling on touch? How is that implemented? With jQuery or another lib? Now we have a jQuery dependency for little value. |
im going to do some thinking on this - id almost prefer not to bind anything to the scroll/touch events, because handlers tend to pile up on them if not careful, and can impact performance for minimal gain. |
Setting a JS variable in one scroll handler is not going to degrade any performance. |
@ChrisCinelli If I understand correctly, the perf discussion is more about where we should add event listeners, not setting 1 variable. Let's assume that jump.js implements the imo, there are too many combination of events to implementation cases to add the event listeners into the jump.js library. A user can have more fine grain control of jump.js when it implements its own event listeners. |
@bretkikehara Adding customization and leaving it up to the implementer is mainly beneficial if each implementer has a different use case for the library. Since there is only one primary use case (scrolling to an element in an efficient manner), I think that each implementer is going to end up trying to write the exact same code to try to solve this problem. Since that's the case, I think the customization should be a fallback and that a default cancelJump function should be made with just touch and wheel events. |
@jljorgenson18 In this sense, I like @bretkikehara branch at: https://github.com/bretkikehara/jump.js I documented at : https://github.com/ChrisCinelli/jump.js/tree/master#cancel-scrolling |
@jljorgenson18 In most cases, I would agree that code duplication should be in the library if there is a bunch of duplication. In this case, a few lines of code is all that will be duplicated with more control over how it is used and how to clean it up. A var jumper = require('jump.js');
function cancel () {
if (jumper.isJumping()) {
jumper.cancel();
}
}
window.addEventListener("wheel", cancel, false);
window.addEventListener("touchstart", cancel, false);
// clean up the events
// window. removeEventListener("wheel", cancel);
// window. removeEventListener("touchstart", cancel); |
If you don't want to include event handlers in the core library, I think @bretkikehara's suggestion is a good one. Alternatively, you could provide an setting, much like |
Thanks guys for your snippets. |
I think user behaviour should overwrite library behaviour. When user starts scrolling, it should stop scrolling. Imho this should be and optional feature.
The text was updated successfully, but these errors were encountered: