Carousel does not support dynamic content

[ben-brown-domaine]

Describe the bug

The carousel component will not work unless the slides are present from the initial render. I can see there is a slotchange event listening for new slides being added/removed, but the initial render breaks if the slide length is 0 due to the newActiveSlide calculation resulting as NaN due to 0 % 0. Subsequently, once dynamic data gets added, the slotchange event does not appear to fire and re-trigger the initializeSlides method. This makes the component only usable for static content, and not, for example, content resulting from a data fetch.

To Reproduce

Try slotting dynamic content into the component:

    <sl-carousel class="carousel"  navigation loop>
        ${this.images?.map(
          (image) => html`
            <sl-carousel-item>
              <img src=${image.src} alt=${image.alt} />
            </sl-carousel-item>
          `
        )}
      </sl-carousel>

The carousel scrolling will be broken

[alenaksu]

Hi @ben-half-helix, I think the issue you are reporting should have been fixed in #1605, but it hasn't been released yet.

[ben-brown-domaine]

Thanks @alenaksu, are you able to confirm by testing locally? I can see you added a check for empty slides that would prevent the NaN error. I didn't see the slotchange event firing in my testing on the current release, but haven't debugged in detail, not sure if this issue will persist given latest updates.

Also, do you know when to expect the release?

[claviska]

@ben-half-helix not sure if this is related, but it's a gotcha a lot of folks aren't aware of. From MDN:

Note: the slotchange event doesn't fire if the children of a slotted node change — only if you change (e.g. add or delete) the actual nodes themselves.

In other words, if you have slides like this:

<sl-carousel>
  <sl-carousel-item>
    <!-- more elements here -->
  </sl-carousel-item>

  <sl-carousel-item>
    <!-- more elements here -->
  </sl-carousel-item>
</sl-carousel-item>

The slotchange event is only emitted when one of the <sl-carousel-item> elements changes. It will not be emitted when any of its children element change.

[ben-brown-domaine]

@claviska I did indeed review this when I saw slotchange didn't fire. However, dynamically rendering the slotted carousel-items means adding actual slot nodes to the DOM, not just changing children. If it helps expedite the release, I can build and test the changes mentioned by @alenaksu and confirm the component works as per my initial code example now, and if slotchange still isn't firing as expected debug + submit a PR. I would love to have this released ASAP so I can use it in production

[claviska]

2.11.1 has been released containing this fix. Please let us know if you have any trouble with it!

[ben-brown-domaine]

@claviska This is great in that it resolves the issue that breaks the carousel when no slides are present on first render. I'm able to populate it based on dynamic data by using JS DOM API, however I've noticed you're using a MutationObserver rather than native slotchange event to fire the slot change handler? I get the following error when trying to dynamically populate using a Lit template:

Uncaught TypeError: Cannot read properties of undefined (reading 'toLowerCase')
    at SlCarousel.isCarouselItem

Which is referring to

  isCarouselItem(node) {
    return node.tagName.toLowerCase() === "sl-carousel-item";
  }

[claviska]

2.11.2 has been released containing the fix in #1684.

[ben-brown-domaine]

Confirming this fully resolves my bug report, it's now easy to populate the carousel with dynamic data. Thanks for the fast turnaround, exited to deploy this on fellowproducts.com!