embla-carousel icon indicating copy to clipboard operation
embla-carousel copied to clipboard

[Bug]: snapList gets broken if some slides are hidden

Open mickaelchanrion opened this issue 1 year ago • 4 comments

Which variants of Embla Carousel are you using?

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

Steps to reproduce

I have to render a carousel with the ability to filter slides by category. The bug occurs when I change the category. Depending on the selected category, the carousel gets broken. It seems like the snap list is not correct.

Expected Behavior

The carousel triggers the event reInit correctly upon category selection but the snap list should be correct.

Additional Context

  • In the reproduction link, the error occurs when selecting the categories 'bar' or 'baz'.
  • I'm using alpinejs to manage the display of slides according to the selected category but there is not much going on here. The only important thing to know is that a slide has display set to none if it should be hidden.
  • Instead of the display: none, I also tried to add/remove the slide elements from the DOM but the problem persists. To reproduce:

Change this:

<div x-show="!category || category === 'bar'" class="embla__slide">
  <div class="embla__slide__number">
    1
    <span>bar</span>
  </div>
</div>

Into this:

<template x-if="!category || category === 'bar'">
  <div class="embla__slide">
    <div class="embla__slide__number">
      1
      <span>bar</span>
    </div>
  </div>
</template>

What browsers are you seeing the problem on?

All browsers

Version

v8.1.7

CodeSandbox

https://codesandbox.io/s/embla-carousel-with-filters-n3pqjz?file=/index.html

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

mickaelchanrion avatar Jul 31 '24 08:07 mickaelchanrion

@mickaelchanrion thanks for your bug report. Did you search for existing issues?

This is expected behavior. Please read what the slides option says. If you want to filter slides, use it to achieve what you want.

davidjerleke avatar Jul 31 '24 08:07 davidjerleke

@davidjerleke Thank you for your reply. Are you saying I should use this option to say whether a slide is active? I've edited the example accordingly:

slides: '.embla__slide.is-active'

It seems to work fine if I select any category but if I select 'all', I only get 3 snaps instead of 6. Do you have an idea why this is happening?

mickaelchanrion avatar Jul 31 '24 09:07 mickaelchanrion

@mickaelchanrion so I finally got some time to investigate this. Embla will automatically react to the following changes:

  • Slide and container size changes with ResizeObserver. In your case, no size is actually changing when you click the category all because this doesn't trigger the ResizeObserver callback. This is also why Embla isn't re-initialized. Then you might ask: Why does the other categories trigger a size change. It seems like hiding some slides with display: none trigger a fractional pixel change or similar. In other words, you're lucky that this is working 🙂.

  • Slide node changes, meaning if slides are added and removed with MutationObserver.

This is expected behavior and not a bug.

How to solve this

You can probably solve this in two ways:

  1. Temporarily remove hidden slides from the DOM and add them back again when they're visible.
  2. When a category changes, manually tell Embla that something has changed by running emblaApi.reInit().

davidjerleke avatar Aug 23 '24 10:08 davidjerleke

@mickaelchanrion please let me know if you intend to try the suggested solutions.

davidjerleke avatar Aug 28 '24 14:08 davidjerleke

@davidjerleke Thank you for your reply.

After facing this problem, I first tried to call reInit() but it doesn't seem to work properly: the carousel breaks right after selecting a category. Moving back to "all" or selecting another category is then fine tho. Here is a reproduction link

Then I tried something similar to what you suggested by removing/adding slides in the DOM. I used a documentFragment to avoid multiple repaints:

const container = document.querySelector('.embla__container')
const slides = [...container.children]

function onCategoryChange(category) {
  const fragment = document.createDocumnetFragment()

  for (const slide of slides) {
    if (!category || category === slide.dataset.category) {
      fragment.append(slide)
    }
  }

  container.replaceChildren(fragment)
}

This works fine but it's more work for the browser.

mickaelchanrion avatar Sep 02 '24 18:09 mickaelchanrion

After facing https://github.com/davidjerleke/embla-carousel/issues/958#issuecomment-2260105880, I first tried to call reInit() but it doesn't seem to work properly: the carousel breaks right after selecting a category. Moving back to "all" or selecting another category is then fine tho.

@mickaelchanrion reInit() is working fine but you're most likely running into a race condition when changing category. I don't know how AlpineJS operates but you need to make sure the category changes are applied before you run emblaApi.reInit(). I've tested his with React and Vue and everything is working fine.

davidjerleke avatar Sep 03 '24 07:09 davidjerleke