davidjerleke / embla-carousel

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

canScrollNext does not stop soon enough if more than one slide is visible #341

Closed tordans closed 2 years ago

tordans commented 2 years ago

Bug is related to

Embla Carousel version

Describe the bug

Try using the slider at https://radwege-check.de/

Code: https://github.com/FixMyBerlin/fixmy.safetycheck/blob/develop/src/components/Homepage/Presets/PresetSlider.tsx

The issue – as far as I understand it – is, that canScrollNext does not handle the special case when more than one slide is visible at the same time.

I worked around it with this custom code for desktop. But that only seems to work well for the first tab of the sliders which has 6 slides. The one with 5 slides "over scrolls", which might be because my check does not look if the slide count is odd/even.

Expected behavior

I am looking for a way to use canScrollNext that will allow to disable the button once all available slides are visible to the user.

Screenshot

image
davidjerleke commented 2 years ago

Hi @tordans,

Thank you for your question.

The issue – as far as I understand it – is, that canScrollNext does not handle the special case when more than one slide is visible at the same time.

canScrollNext and canScrollPrev have nothing to do with what slides are in view or not. It is purely index based and will blindly follow the scroll snaps that Embla creates based on what options you pass to the carousel instance. If I understand you correctly, you're looking for the containScroll option:

Clear leading and trailing empty space that causes excessive scrolling. Use trimSnaps to only use snap points that trigger scrolling or keepSnaps to keep them.

const [emblaRef, emblaApi] = useEmblaCarousel({
  containScroll: 'trimSnaps',
  // ...other options
})

On a side note: Please trigger the select function on reInit too:

emblaApi
  .on('select', onSelect)
  .on('reInit', onSelect) // <-- Add this

Let me know if this is what you want to achieve.

Best, David

davidjerleke commented 2 years ago

Hi @tordans,

Have you had the chance to read my response?

Best, David

davidjerleke commented 2 years ago

I'm closing this as resolved due to lack of response. Feel free to reopen this if the provided solutions wasn't what you were looking for. If that's the case, please provide clear instructions of what you want to achieve.