davidjerleke / embla-carousel

A lightweight carousel library with fluid motion and great swipe precision.
https://www.embla-carousel.com
MIT License
5.42k stars 164 forks source link

Carousel stuttering with many items when setting state on scroll #458

Closed fabz1001 closed 1 year ago

fabz1001 commented 1 year ago

Bug is related to

Embla Carousel version

Describe the bug

I'm trying to eliminate the stuttering issue that seems to appear whenever I set a state on scroll, particularly the setSlidesInView one, in the codesandbox I provided. Being it a lighter version of my actual code, in this example the issue starts manifesting with way more items (~1500) than I have (~100). If I comment out that setSlidesInView either in my code or in the example, the carousel will work just perfectly fine. I wonder if there is a way to get around this issue I'm facing as I think I've tried them all, among throttle, flushSync and what not, to no avail. The goal is to lower the opacity of the immediate slide out of the view.

I appreciate any kind of help and/or any suggestions on how I can improve my code. Thanks!

CodeSandbox

Steps to reproduce

Expected behavior

davidjerleke commented 1 year ago

Hi @fabz1001,

This is not a bug with Embla. Setting the React slidesInView state every time the scroll event fires will re-render the component every single time, even when slides in view haven't changed, because embla.slidesInView() returns a new array every time. So React will check the reference between arrays and re-render even though the actual slidesInView content hasn't changed. Arrays are compared by reference so:

[] === [] // is false
[0, 1, 2] === [0, 1, 2] // is false

Here's a different approach. It doesn't update any slidesInView state. Instead, it just toggles the slide --hidden classes directly. If you're still experiencing performance problems, you can throttle your scroll logic. But this may still stutter on mobile devices. 2000 slides is a looooot of slides!? I would consider different UX alternatives for my users rather than having that many slides to scroll which would take forever.

Another approach would be to use IntersectionObserver instead of embla.slidesInView(). I think this approach is best because it’s natively optimized in the browser and you don’t have to listen to any scroll event. IntersectionObserver will only fire when something actually change which will keep the number of re-renders to a minimum.

Let me know how it goes.

Best, David

fabz1001 commented 1 year ago

Hi @davidjerleke,

First of all, I do apologize for posting this as a bug since, as you said, this is not.

πŸŽ‰ I have applied the fix you suggested and it seems much smoother indeed!

And yes, they're a lot of slides. However, in my case I have much less of them but definitely more complex and heavy, so I had to exaggerate a bit to make the issue more evident in the sandbox :)

I have another question but I will post it in a more appropriate place.

Thank you again David, Embla rocks!

davidjerleke commented 1 year ago

Hi @fabz1001,

First of all, I do apologize for posting this as a bug since, as you said, this is not.

No worries, I just wanted to point that out based on my reasoning πŸ™‚.

I have applied the fix you suggested and it seems much smoother indeed!

Which fix did you go with? I think IntersectionObserver will yield the smoothest behavior.

And yes, they're a lot of slides. However, in my case I have much less of them but definitely more complex and heavy, so I had to exaggerate a bit to make the issue more evident in the sandbox :)

I see! Thanks for clarifying. I get why you did that now πŸ‘.

Best, David

fabz1001 commented 1 year ago

I went for the direct class toggling + throttling solution.

I am using the IntersectionObserver for an infinite loading kind of logic already (It's something I have to optimize as well. I will create a stripped down version as soon as possible but in a different section). I will surely try it also for the opacity one but a bit later, as I have another priority for now :)

I will keep you posted!

Thank you, Fabio

davidjerleke commented 1 year ago

Closing this issue then. Cheers!

fabz1001 commented 1 year ago

Hi @davidjerleke πŸ™‚

I am pleased to inform you that the suggested IntersectionObserver approach is actually working very well πŸ†.

For accessibility purposes I am setting and removing some attributes instead of toggling a hidden class. I will just need to find a clean way to set tabIndex to -1 for any inner clickable elements as well.

I'll leave my codesandbox here for any possible users who wish to implement a similar feature.

Have a great day! πŸ™Œ

davidjerleke commented 1 year ago

I'm glad to hear that @fabz1001 πŸŽ‰. And thanks for sharing your solution πŸ‘. I'm sure that someone will find it useful.

Cheers, David