search

davidjerleke / embla-carousel

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

[Bug]: beforeUnmount destroys too soon during Vue transitions #869

Open sambedingfield opened 1 month ago

sambedingfield commented 1 month ago

Which variants of Embla Carousel are you using?

Steps to reproduce

The bug occurs when I transition away a Vue component containing an Embla carousel, causing it to be destroyed too early. This causes a flicker of an unstyled carousel during the transition.

For a common example, imagine a transition on an app route and clicking a link from within a carousel slide. The user sees the destroyed state of the carousel before the page transitions away.

Expected Behavior

That either the clean up can be prevented or delayed during beforeUnmount.

Additional Context

To clarify, this is a known Vue thing, and not unique to Embla. It happens with a lot of libraries that clean up using Vue's beforeUnmount hook.

A solution could be using a mutation observer or providing a timeout prop to delay the destroying. Here's how it got discussed for Splide.

This could require a similar solution to the React issue here https://github.com/davidjerleke/embla-carousel/issues/540, where "freezing" styles could work.

I tried the following workaround, but it doesn't consider slides that are translated during loops :

beforeUnmount(){
  const x = this.embla.internalEngine().location.get();
  const $cont = this.embla.containerNode();
  $cont.style.transform = 'translateX('+x+'px)';
}

What browsers are you seeing the problem on?

All browsers

Version

v8.0.4

CodeSandbox

https://codesandbox.io/p/devbox/reverent-almeida-nh8pkr

Before submitting

davidjerleke commented 1 month ago

@sambedingfield thanks for your bug report. Is this expected behaviour for onBeforeUnmount:

Or is it a bug? It seems like some of the comments suggest it's by design? This was the PR where the onUnmounted was replaced with onBeforeUnmount:

I'm not a seasoned Vue dev by any means but would re-introducing onUnmounted fix the problem? @meirroth and/or @sadeghbarati, any thoughts on this?

sambedingfield commented 1 month ago

It doesn't look like it's going to change, so... by design ¯\_(ツ)_/¯ Without any kind of destroy delay or style freeze via Embla there's no other workaround (that I know of) to prevent this. So feel free to flag this as a feature request instead!

To clarify, both onUnmounted and onBeforeUnmount trigger beforehand, irrelevant to parent Vue transitions.

sadeghbarati commented 1 month ago

Will create Vue SFC playground to see differences between Vue versions

IIRC they refactored Transition component alot lately

meirroth commented 1 month ago

@davidjerleke I'd be concerned about making such a change without extensive testing, especially given any potential errors or performance changes https://github.com/davidjerleke/embla-carousel/issues/672#issuecomment-1872952987.

@sambedingfield Is it possible for you to share a minimal reproduction of the problem you're experiencing so that we can visualize it, debug it, and possibly find a solution? One option I can think of is to use the embla-carousel core without the wrapper and manually destroy it whenever you want. It's actually quite simple, as demonstrated by the Vue 2 example I provided earlier.

sadeghbarati commented 1 month ago

@meirroth

Here the Vue SFC playground

It appears that there are issues with Transition and onBeforeUnmount

sambedingfield commented 1 month ago

@meirroth My CodeSandbox is in the original report, here: https://codesandbox.io/p/devbox/reverent-almeida-nh8pkr And you're correct, a workaround could be to not use the Vue version.

meirroth commented 1 month ago

@sambedingfield Sorry I missed it 🤦‍♂️ Thanks for sharing.

It seems to me that the issue persists after replacing onBeforeUnmount with onUnmounted as you can see in this Vue SFC playground using embla-carousel core.

meirroth commented 1 month ago

@davidjerleke Just a thought, do you think this can be fixed by leaving translations as is on destroy? https://github.com/davidjerleke/embla-carousel/blob/61c050c64b8631f68e091d9e6e1ce96ccad568ad/packages/embla-carousel/src/components/EmblaCarousel.ts#L139C12-L139C21

davidjerleke commented 1 month ago

@meirroth it can, but not every user would want that. I'm not sure how to solve this in an optimal way. Maybe we can add a parameter to destroy() that's a boolean:

function destroy(keepStyles: boolean): void {
  // ...
}

On the other hand, that wouldn't solve this request which is related to this to some extent:

sadeghbarati commented 1 month ago

https://stackblitz.com/edit/basic-embla-carousel-dthb3k

https://svelte.dev/repl/0e18b50b771c496dbf11a7d330a46164?version=3.31.2