search

davidjerleke / embla-carousel

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

[Bug]: Extra snapList point on some cases #707

Closed huri3l closed 8 months ago

huri3l commented 8 months ago

Version

8.0.0-rc19

CodeSandbox

CodeSandbox Link

What browsers are you seeing the problem on?

All of the above

Steps to reproduce

The bug occurs when I am using the library shadcn/ui, which uses embla-carousel under the hood. In the CodeSandbox, I have used the same code I used and got the issue.

In the example, there is an extra step in the Dots and in the Arrows, which could be eliminated.

https://github.com/davidjerleke/embla-carousel/assets/61247833/85f7f629-e038-4a89-aaa9-7527497365e1

Expected Behavior

The expected behavior is that it should have only 4 steps, and not 5, since I have 6 content in total and three visible at a time.

Additional Context

I have debugged a little bit and found out that the problem is not canScrollNext or canScrollPrev, but actually, when embla identifies how many steps it has.

image

As you can see through the image, the snapList goes from 0 to 4, and 3 is a value really close to 1.

Is there anyway to merge this step with the last one?

I am not sure if this is a bug, but if it is not, I haven't found the correct option to use with embla-carousel to fix it. I am already using containScroll: 'trimSnaps' and it is still with the undesired behavior.

Which variants of Embla Carousel are you using?

Before submitting

huri3l commented 8 months ago

Also, the goal is to embla-carousel detect that the 3rd point is really close to the 4th point (in this example) to merge them into one. I suppose this is happening due to the fact that flex-basis is the one who rules how many content will be visible in each slide, and some values of the flex-basis are float, leading to this bug.

davidjerleke commented 8 months ago

@huri3l thanks for your bug report. Before I start investigating this, are you sure that shadcn/ui is using v8.0.0-rc19?

It seems like they still are using v8.0.0-rc15? Because the pull request I linked to isn’t merged yet.

Best, David

huri3l commented 8 months ago

Hi David! Yes, shadcn/ui is using v8.0.0-rc15, but in the CodeSandbox I have used the v8.0.0-rc19 and the bug still occurs.

I have opened this issue here because this behavior is blocking a pull request where I am implementing dots nagivation.

davidjerleke commented 8 months ago

@huri3l thanks for confirming. I’ll have a look when possible.

Best, David

davidjerleke commented 8 months ago

@huri3l I can confirm that this is a bug. Embla has changed from reading getBoundingClientRect to offsetWidth and offsetLeft recently which has the following benefit:

With offset dimensions you loose the precision that getBoundingClientRect gives you though, and that’s why it requires solutions that has “pixel tolerances” to avoid unexpected behavior. Most of these cases have already been solved like grouping slides with slidesToScroll: auto but I simply didn’t predict this case.

I just finished writing tests for these cases. I found two cases where this bug occurs (❌ = remove snap):

The solution to this is to add a pixel tolerance of 1 pixel so half pixel diffs doesn't add another snap point.

Best, David

huri3l commented 8 months ago

Nice David! It seems that this toleration is going to fix the problem.

davidjerleke commented 8 months ago

@huri3l would you mind testing this CodeSandbox to see if you can reproduce the problem?

Another case that triggered the undesired behavior was with 10 slides 20% wide each. If you have a moment, I would appreciate if you could test that and any other relevant case you can think of too.

Thanks! David

huri3l commented 8 months ago

It works wonders @davidjerleke! As far as I have tested, with multiple slides and sizes, there are no problems.

I have also noticed, testing in the current version of the lib, that this problem oftenly occurs when the flex-basis returns a float value. Like the basis-1/3 class, returns 33.33333...%.

I tested with other basis that also returned float and the problem was solved in this CodeSandbox too.

Thanks for the fixing man!

davidjerleke commented 8 months ago

Thanks a lot for taking the time to test the bug fix @huri3l. Much appreciated 👍. I will release v8.0.0-rc20 as soon as possible which will include this fix.

Best, David

davidjerleke commented 8 months ago

@huri3l a bug fix for this was just released in v8.0.0-rc20.

huri3l commented 8 months ago

Amazing! @davidjerleke will you update the PR to update shadcn/ui package to include this new version?

davidjerleke commented 8 months ago

@huri3l I’m not the author of that PR, @Xxsource98 is: