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]: Scroll broken after direction update #832

Closed meirroth closed 2 months ago

meirroth commented 2 months ago

Which variants of Embla Carousel are you using?

Steps to reproduce

The bug occurs when triggering reInit after changing direction.

Click the button "Change dir" and try dragging the carousel, you'll see you can't scroll all the way as you would expect.

Expected Behavior

I expect Embla to work smoothly after switching direction. Also would expect Embla to detect option change, as David mentioned options are reactive https://github.com/davidjerleke/embla-carousel/issues/744#issuecomment-1987153419.

Additional Context

Setting direction RTL initially works as expected.

What browsers are you seeing the problem on?

Firefox, Chrome

Version

v8.0.2

CodeSandbox

https://stackblitz.com/edit/github-jafces?file=app.vue

Before submitting

meirroth commented 2 months ago

I'm migrating from vueper-slides in my Nuxt.js production project. Embla looks very processing and I can't wait to get it up and running 😃

davidjerleke commented 2 months ago

@meirroth Embla makes use of native browser direction so you need to set <body dir="rtl"> before setting the carousel direction option to rtl. This is mentioned in the docs. Have you tried that?

meirroth commented 2 months ago

@davidjerleke I did that. Notice useHead(). You can inspect HTML and see dir="rtl" on the HTML element.

davidjerleke commented 2 months ago

@meirroth so that happens before direction option is set?

meirroth commented 2 months ago

@davidjerleke Not sure what you mean. useHead sets dir on the HTML tag, and updates it when the value changes.

davidjerleke commented 2 months ago

@meirroth log this AFTER direction has changed:

emblaApi.value.internalEngine().options.direction

What do you get? Does it match your direction state?

meirroth commented 2 months ago

@davidjerleke It logs the correct direction: image

davidjerleke commented 2 months ago

@meirroth I don’t have access to my computer so will check your StackBlitz out tomorrow.

meirroth commented 2 months ago

@davidjerleke Thank you!

sarussss commented 2 months ago

Hi @meirroth I think it works normally Are you sure? Change dir in html tag Runs before reinstalling embla

davidjerleke commented 2 months ago

@meirroth as far as I can see, the document dir hasn't finished updating when you re-initialise the carousel with the direction "rtl". See this StackBlitz and check the console when you click on the button (and also see the comments in the code). You need to do things in the correct order:

  1. Update the document dir attribute.
  2. AFTER that, you have to update the options like this options.value = { direction: dir.value } and Embla will re-initialise because its options are reactive. No need to call reInit yourself.

I'm not a Vue expert by any means, so I don't know if there's a way to wait for the DOM to update before changing the options, in this case we want to wait for the document dir attribute. If there isn't, I would recommend you to use MutationObserver on the HTML element and change options when the dir attribute changes.

Best, David

meirroth commented 2 months ago

@davidjerleke Thank you investigating this issue and for the explanation! You are right, i18n or useHead is not fast enough to update HTML dir for Embla, so I got it working by setting dir manually on the carousel and using watchPostEffect to update the Embla direction. https://stackblitz.com/edit/github-gmh6db-bjqkxt?file=app.vue

Embla accepting reactive options is awesome!! I think this should be mentioned in the docs, unless it already is and I missed it. Would you like me to open a PR?

davidjerleke commented 2 months ago

@meirroth I'm glad you solved it 🎉.

Embla accepting reactive options is awesome!! I think this should be mentioned in the docs, unless it already is and I missed it. Would you like me to open a PR?

Sure. I would appreciate a PR! Maybe add an additional note (admonition) here?

[!NOTE]
One important thing to note though: The options for the core vanilla package embla-carousel are NOT reactive, but the React, Vue, Solid and Svelte wrappers have reactive options. I think it makes sense to make that clear so vanilla JS users don't expect their options to be reactive.

Best, David

meirroth commented 2 months ago

@davidjerleke Done https://github.com/davidjerleke/embla-carousel/pull/836

davidjerleke commented 2 months ago

@meirroth thanks for taking time to improve the documentation!