davidjerleke / embla-carousel

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

Wrong previousScrollSnap value when in loop mode #234

Closed renaudcollet closed 3 years ago

renaudcollet commented 3 years ago

Bug is related to

Embla Carousel version

Describe the bug

Wrong previousScrollSnap value when initialized with loop: true

CodeSandbox

https://codesandbox.io/s/embla-carousel-loop-vanilla-forked-vcd89?file=/src/js/index.js

Steps to reproduce

Check the console log, Previous and Current have the same value

Expected behavior

In that case Previous should have index value of the last slide

Additional context

davidjerleke commented 3 years ago

Hi @renaudcollet,

This is not a bug but rather intended behaviour. When you initialise the carousel, there is no previousScrollSnap yet, because previousScrollSnap doesn't refer to which slide that comes before the selectedScrollSnap. It actually refers to the previously visited snap by the user.

An example: Let's say that you're on slide index 3 and scroll to index 6 by clicking dot navigation. In this case, previousScrollSnap will be 3 (and not 5).

This is why they both hold the same value when the carousel is initialised.

Best, David

renaudcollet commented 3 years ago

Hi @davidjerleke

Thank you for your explanation, and for developing this nice ligthweight library.

Indeed I misunderstood the purpose of previousScrollSnap, I thought of it as a way to get the slide placed on the left of the current visited slide, which value is returned by selectedScrollSnap.

It could be useful to have methods to get the slide on the left/right of the current selected item, to add css classes to those items.

Best

davidjerleke commented 3 years ago

Hi @renaudcollet,

Thanks! No worries. If you know a better way of explaining previousScrollSnap in the docs, feel free to make a contribution and maybe it will be more clear to others what this method does.

I'm working on v6 with the plugin system now #230 and I'll consider adding the methods you mention. Until then, try this:

// The parameter direction should be 1 for forward, -1 for backward
function getAdjacentScrollSnap(direction) {
  const engine = embla.dangerouslyGetEngine()
  return engine.index.clone().add(direction).get()
}

// Usage
const nextAdjacentSnap = getAdjacentScrollSnap(1)
const prevAdjacentSnap = getAdjacentScrollSnap(-1)

Let me know if it helps.

Best, David

renaudcollet commented 3 years ago

Hi @davidjerleke,

Your definition is good actually

Get the index of the previously selected snap point

Could add, but redundant

, the last snap point visited by the user

Anyway what led me wrong is that I was really looking for a method returning the previous / next slide index. I wrote it differently with a counter, but your way of doing it is very interesting.

Thanks again Best

davidjerleke commented 3 years ago

Thanks @renaudcollet!

The benefits of using my solution is that it uses the internal Embla engine. So that counter will take different options into account, like slidesToScroll and so on…

Best, David