Skip to content
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

Implement relative scroll time from distance #167

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

luispuig
Copy link

@luispuig luispuig commented Nov 26, 2020

The SCROLL_TIME is a magic number of 468ms.
I would like to use this great polyfill in https://github.com/luispuig/react-snaplist-carousel but this time is too long.

My first thought was making it configurable but then I read #140 and #65

Hey @vkatushenok! Just got back from holiday. @jeremenichelli and I have spoken about something like this before and I’m generally against making it customizable. If you could find constants that different UAs and OSes use for this I’d be happy to see an approach pursued that tries to more accurately emulate the differences.

I agree

tbh it's pretty arbitrary. Ideally, we would calculate the total scroll diff to make ahead of time and have the SCROLL_TIME be dynamic based upon that.

Here I got the idea of using the scroll distance to calculate the scroll time.

I set up some min and max scroll times.

Left: Firefox, Center: Chrome, Right: Safari using the polyfill

speed

Chrome makes the animation much faster than Firefox and different ease. So, I tried to adjust something in the middle.

P.D: Great work with this polyfill :D

Copy link
Collaborator

@jeremenichelli jeremenichelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this PR Luis, I left some comments, let me know what you think!

src/smoothscroll.js Outdated Show resolved Hide resolved
@@ -221,6 +222,12 @@ function polyfill() {
method = scrollElement;
}

const maxDistance = Math.max(Math.abs(x - startX), Math.abs(y - startY));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment on top of this two lines of math operations (not that is unclear what they do), it will help people reading the code for the first time a lot, to understand the reasoning and why we are doing it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let put it in 2 lines and better for others to modify it in the future.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, feel free to suggest a different explanation, comments, etc...
Also, I added the animation speed which is 1 pixel/ms. It will make it easier to modify it you think that another value matches better the native bahaivour.

@luispuig
Copy link
Author

luispuig commented Dec 7, 2020

@jeremenichelli , what do you think about the changes? Please, let me know what else to change. Thanks :)

@jeremenichelli
Copy link
Collaborator

Sorry for the late response Luis, I've been caught up with personal stuff.

One thing I notice is you are using const and in the build project we don't have transpiling to support old browsers, so I would change those to vars for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants