[Bug]: UI issues when removing slides

sshmaxime commented 8 months ago

Version

v8.0.0-rc21

CodeSandbox

https://codesandbox.io/p/sandbox/embla-carousel-default-react-forked-ccnhsf?file=%2Fsrc%2Fjs%2Findex.tsx

What browsers are you seeing the problem on?

Google Chrome

Steps to reproduce

The bug occurs when I remove items from the Carousel before the Carousel Item has completely appeared on screen. Or when I remove items before the Carousel has completely scrolled.

https://github.com/davidjerleke/embla-carousel/assets/25589782/ffc1eefb-2a92-462a-bc78-6311fa964faf

Expected Behavior

I should be able to remove Carousel's item smoothly without UI bugs.

Additional Context

No response

Which variants of Embla Carousel are you using?

[X] embla-carousel (Core)
[X] embla-carousel-react
[ ] embla-carousel-vue
[ ] embla-carousel-svelte
[ ] embla-carousel-autoplay
[ ] embla-carousel-solid
[ ] embla-carousel-auto-height
[ ] embla-carousel-class-names
[ ] embla-carousel-docs (Documentation)
[ ] embla-carousel-docs (Generator)

Before submitting

[X] I've made research efforts and searched the documentation
[X] I've searched for existing issues
[X] I agree to follow this project's Contributing Guidelines for bug reports

sshmaxime commented 8 months ago

It works "correctly" (at least no empty space and in middle position) until rc17. Error appears at rc18.

davidjerleke commented 8 months ago

@sshmaxime thank you for your bug request.

As you say, it shouldn't stop with empty space in middle of the transition. It should hard reset, meaning that it should terminate any ongoing transitions and snap into place at once. So that part is a bug. About the other thing: There's no promise from the Embla side that it should remove slides smoothly. Just to set the expectations right.

I'll investigate this when possible.

Best, David

sshmaxime commented 8 months ago

Hey @davidjerleke , thanks for the reply.

Regarding the smooth animation on remove, well I guess that's totally fair. That being said, on 7.1.0 animations are somehow smooth if you don't reInit on slides props change. Maybe there's something to do here, idk.

Just so you know, adding the ability to smoothly add and remove items would bring Embla at the top of the Carousel game (it's already pretty high but yeah).

That being said, thanks for investigating the issue.

davidjerleke commented 8 months ago

@sshmaxime,

Regarding the smooth animation on remove, well I guess that's totally fair. That being said, on 7.1.0 animations are somehow smooth if you don't reInit on slides props change. Maybe there's something to do here, idk.

That's becuase v8 is automatically listening to changes in slide sizes and added/removed slide nodes. Which is what most of the devs out there want. However, you can opt out of the default auto reInit() behaviour by passing a callback to watchSlides. For example, the infinite scroll demo is using a fluid reInit():

import { EngineType } from 'embla-carousel/components/Engine';

function EmblaCarousel(props) {
  const { options } = props

  const [emblaRef, emblaApi] = useEmblaCarousel({
    ...options,
    watchSlides: (emblaApi) => {
      const reloadEmbla = (): void => {
        const oldEngine = emblaApi.internalEngine();

        emblaApi.reInit();
        const newEngine = emblaApi.internalEngine();
        const copyEngineModules: (keyof EngineType)[] = ['location', 'target', 'scrollBody'];
        copyEngineModules.forEach(engineModule => {
          Object.assign(newEngine[engineModule], oldEngine[engineModule]);
        });

        newEngine.translate.to(oldEngine.location.get());
        const { index } = newEngine.scrollTarget.byDistance(0, false);
        newEngine.index.set(index);
        newEngine.animation.start();
      };

      reloadEmbla();
      return false;
    },
  });

  // ...
}

Just so you know, adding the ability to smoothly add and remove items would bring Embla at the top of the Carousel game (it's already pretty high but yeah).

As demonstrated above, it's definitely possible. Especially for simple use cases. The problem with supporting a feature like that is that there's no way to write tests to cover all scenarios that can happen (think about the number of options, methods and events Embla has, and then think about how many combinations are possible). Because devs do all sorts of stuff that will break that, so I don't want a bunch of "false" bug reports where people claim the library is broken because it can't do magic to patch all their crazy cases.

Best, David

sshmaxime commented 8 months ago

Thanks a lot, that works really well !

That being said, that looks way too custom to be in our codebase. Don't you really think that it could be in the codebase as an option ? That'd be lovely. Although, as you said, seems like there is a problem with supporting a feature like that, so idk.

davidjerleke commented 8 months ago

@sshmaxime at the end of the day, I'm maintaining this library and I simply don't have time to handle a lot of "false" bug reports as mentioned in my earlier comment:

The problem with supporting a feature like that is that there's no way to write tests to cover all scenarios that can happen (think about the number of options, methods and events Embla has, and then think about how many combinations are possible). Because devs do all sorts of stuff that will break that, so I don't want a bunch of "false" bug reports where people claim the library is broken because it can't do magic to patch all their crazy cases.

But thank you for your ideas anyway. And thanks for reporting this bug ⭐! I found the culprit and will merge the bug fix soon.

Best, David

davidjerleke / embla-carousel