Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No transitions on first slide while using navigation buttons when destroy and reinit the carousel #672

Closed
2 of 9 tasks
sadeghbarati opened this issue Dec 31, 2023 · 15 comments · Fixed by #677
Closed
2 of 9 tasks
Labels
bug Something isn't working resolved This issue is resolved

Comments

@sadeghbarati
Copy link
Contributor

sadeghbarati commented Dec 31, 2023

Bug is related to

  • embla-carousel (core package)
  • embla-carousel-react
  • embla-carousel-vue
  • embla-carousel-svelte
  • embla-carousel-autoplay
  • embla-carousel-auto-height
  • embla-carousel-class-names
  • embla-carousel-docs (documentation)
  • embla-carousel-docs (generator)

Embla Carousel version

  • v8.0.0-rc17

Describe the bug

No transition occurs on first slide on hitting next button when destroy and reinit the Embla Carousel

CodeSandbox / StackBlitz

https://stackblitz.com/edit/vitejs-vite-k4menh?file=src%2Fcomponents%2FCarousel.vue

Steps to reproduce

  1. Go to stackblitz repro
  2. Click on Destroy/Reinit Carousel
  3. Click next button
  4. No transitions occurs on first slide

Expected behavior

slide should have normal transitions

Additional context

None

@sadeghbarati sadeghbarati added the bug Something isn't working label Dec 31, 2023
@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Dec 31, 2023

Related

shadcn-ui/ui#2230

@davidjerleke
Copy link
Owner

@sadeghbarati thank you for your bug report. Try this and see if you can reproduce the problem?

@davidjerleke davidjerleke added the awaiting response Issue is awaiting feedback label Dec 31, 2023
@sadeghbarati
Copy link
Contributor Author

It's working fine now 🙏

What was the problem

@davidjerleke
Copy link
Owner

davidjerleke commented Dec 31, 2023

@sadeghbarati you'll see the code changes when I create the PR. I will release a patch release with the bug fix as soon as possible. It will be v8.0.0-rc18. Thanks for testing it!

@davidjerleke davidjerleke added upcoming A feature or bug fix is on its way for this issue and removed awaiting response Issue is awaiting feedback labels Dec 31, 2023
@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Dec 31, 2023

Just one thing before the publish embla-carousel-vue

Isn't better to use onBeforeUnmount instead of onUnmounted?

@davidjerleke
Copy link
Owner

davidjerleke commented Dec 31, 2023

Isn't better to use onBeforeUnmount instead of onUnmounted?

@sadeghbarati yeah maybe that’s better? I’m not a seasoned Vue dev. Most of my experience is with React. Why is it better? And why is the current approach working as expected?

@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Dec 31, 2023

Both are working well, only just to separate the embla destroying process from Vue reactivity cleanup

@davidjerleke
Copy link
Owner

@sadeghbarati feel free to try it out in the latest StackBlitz fork I shared and submit a PR with the onBeforeUnmount hook instead of onUnmounted.

Best,
David

@sadeghbarati
Copy link
Contributor Author

Thank You ❤️‍🔥

@davidjerleke
Copy link
Owner

@sadeghbarati so you will create a PR 🙂?

@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Dec 31, 2023

Sure thing!

But It was easy to contribute if I was pnpm and pnpm workspaces (Adding Vue in playground directory) 😁

@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Dec 31, 2023

Sry it took too long I was trying to figure out the yarn and creating playground for Vue and also styled-components injected styles which I ignored and copy static styles into main style file

Should I have to include embla-carousel-playground-vue in this PR #673? although the build files are different from what I see in npm


npmjs - embla-carousel-vue/esm/index.d.ts

export { EmblaOptionsType, EmblaEventType, EmblaPluginType, EmblaCarouselType } from 'embla-carousel/index.ts';
export { EmblaCarouselVueType } from './components/emblaCarouselVue.ts';
export { default } from './components/emblaCarouselVue.ts';

local build - embla-carousel-vue/esm/index.d.ts

export { EmblaCarouselVueType } from './components/emblaCarouselVue.ts';
export { default } from './components/emblaCarouselVue.ts';

@davidjerleke
Copy link
Owner

@sadeghbarati thanks for the PR 👍🏻! The build files differ because of this upcoming change which I haven’t released yet.

I think the playground should be a separate PR just to separate things. You can check out the vanilla and react playgrounds to see how the styles are imported from the documentation website examples. You don’t need to add your own that way. And I would recommend you to setup a Vite playground just like the vanilla and react to make it easier for devs to jump between the playgrounds when testing things.

Thanks for your efforts!

@sadeghbarati
Copy link
Contributor Author

Ok will create new PR for playground later after the new release

Thanks for the opportunity ❤️

davidjerleke added a commit that referenced this issue Jan 3, 2024
davidjerleke added a commit that referenced this issue Jan 3, 2024
@davidjerleke davidjerleke added resolved This issue is resolved and removed upcoming A feature or bug fix is on its way for this issue labels Jan 3, 2024
@davidjerleke
Copy link
Owner

davidjerleke commented Jan 3, 2024

@sadeghbarati a bug fix for this was just released in v8.0.0-rc18. Try it out and let me know if it's working as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working resolved This issue is resolved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants