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]: Render when slides are wider than view size #805

Closed tolu-fuelled closed 5 months ago

tolu-fuelled commented 6 months ago

Hi David,

Thanks for a great library. You efforts are much appreciated.

This PR addresses an issue I ran into - when the first slide is wider than the view size, an exception is thrown. This happens because of an underlying assumption when creating scroll groups by size (see test case).

Happy to make additional changes if any suggestions.

davidjerleke commented 6 months ago

@tolu-fuelled thanks a lot for your contribution! I will check this out as soon as possible ✨.

One thing I noticed (that isn’t easy to know): We can delete the last commit with the patch version bumps on this bug fix branch, because this will be done automatically on the main branch by running yarn version:patch - which automatically creates a release and pushes it to the remote. The next patch version 8.0.1 will include several bug fixes.

Thanks a lot and I’ll let you know when I’ve reviewed this.

Best, David

tolu-fuelled commented 6 months ago

@davidjerleke yes of course. commit with the patch version bump has been reverted.

suriyaprakash-s commented 6 months ago

This is awesome, thanks a lot for the fix @tolu-fuelled I have been waiting for this for long time. @davidjerleke When can we expect this to be officially released?

davidjerleke commented 5 months ago

@suriyaprakash-s I can never give time estimates because I work on this library on my unpaid spare time.

However, reviewing this PR is next up after a minor release with some other bug fixes that has higher priority.

tolu-fuelled commented 5 months ago

@davidjerleke made the requested changes and added a few more tests.

The RTL snaps don't add up to me if you don't mind giving it a second look. I ran the react playground - the slides render left to right but they move right to left. So slides render [1, 2, 3, 4, 5] but move [1, empty, empty, empty, empty]. I'm probably missing something.

davidjerleke commented 5 months ago

@davidjerleke made the requested changes and added a few more tests.

Thanks! It looks great 👍.

The RTL snaps don't add up to me if you don't mind giving it a second look. I ran the react playground - the slides render left to right but they move right to left. So slides render [1, 2, 3, 4, 5] but move [1, empty, empty, empty, empty]. I'm probably missing something.

@tolu-fuelled you've set the direction option to 'rtl' but my guess would be that you've forgot to set the content direction to RTL too, because Embla respects the native browser direction and doesn't shift around your slides to achieve a RTL carousel. From the docs:

[!NOTE]
Note: When using rtl, the content direction also has to be set to RTL, either by using the HTML dir attribute or the CSS direction property.

Example:

<body dir="rtl">
  ...
</body>

Maybe that's the culprit?

Best, David

tolu-fuelled commented 5 months ago

@davidjerleke that’s exactly it.

davidjerleke commented 5 months ago

@tolu-fuelled great work! Thanks for solving this problem and stabilizing Embla with relevant tests. It will prevent the bug from reoccurring in the future 👍.

davidjerleke commented 5 months ago

@tolu-fuelled this has been released in v8.0.2.