SlidesToScroll breaks when screen size is smaller than slide width

rubberbandage commented 8 months ago

Bug is related to

[X] embla-carousel (core package)
[X] 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

embla-carousel-react 8.0.0-rc14

with dependencies on

embla-carousel "8.0.0-rc14"
- embla-carousel-reactive-utils "8.0.0-rc14"

Describe the bug

Currently experiencing an issue where if the screensize is smaller than the width of a slide, an error is thrown

Please note that for this; I am using the following styling for a slide;

.embla__slide {
  flex: 0 0 auto;
  min-width: 0;
}

Should flex-basis with auto not be appropriate - please disregard issue; as I have gone against the documentation listed here https://www.embla-carousel.com/get-started/react/

.embla__slide {
  flex: 0 0 100%;
  min-width: 0;
}

Thank you

Read on...

I am using auto as I didn't want to lock slides into any particular sizing, and allow the children to provide their width (and these can be differing) The component is working beautifully and as intended, and I am really loving this carousel; however, I have come across a use case which breaks with only the above change, and having the screen size shorter than a slide

After some debugging, I have located a potential area of interest - bySize(array)

Given an array of 5 items of equal width; but the screen size is smaller than the width; an additional item is returned from bySize(array)

I believe the bug to be within bySize, and returning an extra empty item at beginning of array;

Snapshot at time of issue

  function bySize(array) {
    if (! array.length) return [];
    return arrayKeys(array).reduce((groups, rectB) => {
      const rectA = arrayLast(groups) || 0;
      const isFirst = rectA === 0;
      const isLast = rectB === arrayLastIndex(array);
      const edgeA = containerRect[startEdge] - slideRects[rectA][startEdge];
      const edgeB = containerRect[startEdge] - slideRects[rectB][endEdge];
      const gapA = ! loop && isFirst ? direction.apply(startGap) : 0;
      const gapB = ! loop && isLast ? direction.apply(endGap) : 0;
      const chunkSize = mathAbs(edgeB - gapB - (edgeA + gapA));
      if (chunkSize > viewSize) groups.push(rectB);
      if (isLast) groups.push(array.length);
      return groups;
    }, []).map((currentSize, index, groups) => {
      const previousSize = Math.max(groups[index - 1] || 0);
      return array.slice(previousSize, currentSize);
    });
  }

The downstream usages of this value fail on finding a 'right' property; which with an empty item, leads to undefined

  function measureSizes() {
    return groupSlides(slideRects).map(rects => arrayLast(rects)[endEdge] - rects[0][startEdge]).map(mathAbs);
  }

CodeSandbox

The link to a CodeSandbox that demonstrates the bug clearly. https://codesandbox.io/s/embla-carousel-arrow-dots-react-forked-cf9g7f

Steps to reproduce

Set .embla__slide with flex: 0 0 auto Set EmblaCarousel options to { slidesToScroll: 'auto' }

Open sandbox, and drag resizeable window to less than width of a slide.

Expected behavior

A clear and concise description of what you expected to happen.

Additional context

Add any other context about the problem here, like screenshots or similar.

davidjerleke commented 8 months ago

Hi @rubberbandage,

Thank you for your bug report. If I’m understanding you correctly, this limitation is known. In other words, Embla doesn’t guarantee that having slides wider than the viewport will work. From the slide sizes section in the docs:

Best, David

rubberbandage commented 8 months ago

Understood, thank you for directing me to this page David

I'll need to have a word with the designer to see how we can tweak the UI so we can abide by this rule; I may be able to approach it with slides having 100% on small screen, and auto for anything larger.

If there were no plans to address the comment about make sure slide sizes don't exceed the size of the viewport - I'm happy for this bug to be closed.

Thank you David

davidjerleke commented 8 months ago

@rubberbandage at the time of writing I’m not planning to look into this. Most cases with slides wider than the viewport works as expected except slidesToScroll: auto. But as mentioned it’s not supported officially.

However, the slidesToScroll feature has sufficient tests so if you would like to try solving it you’re welcome to try. Bear in mind that a good solution doesn’t only solve the problem but it also doesn’t increase the bundle size much.

Let me know if you intend to look into it.

Best, David

rubberbandage commented 8 months ago

Likely not actively at this stage, if the above works for current scenario, thought I am curious about finding a solution; so if I find some time up my sleeve, I'll see what I can do - will keep in mind code coverage and bundle size.

Should I find time, I will open a PR

Thank you again

davidjerleke / embla-carousel