davidjerleke / embla-carousel

A lightweight carousel library with fluid motion and great swipe precision.
https://www.embla-carousel.com
MIT License
5.97k stars 180 forks source link

Slides sizes doesnt account for gap with cssgrid + flexbox (incorrect CSS setup) #593

Closed drewlonious closed 11 months ago

drewlonious commented 11 months ago

Bug is related to

Embla Carousel version

Describe the bug

Steps to reproduce

Set slides size to 20% to reproduce

Expected behavior

Additional context

Screenshot 2023-10-09 at 3 13 16 PM
davidjerleke commented 11 months ago

Hi @drewlonious,

Please provide a CodeSandbox that clearly demonstrates the problem. Your issue will be deleted otherwise. This is clearly stated in the contribution guidelines.

Thanks for cooperating.

Best, David

drewlonious commented 11 months ago

@davidjerleke https://codesandbox.io/s/embla-carousel-default-vanilla-forked-zz3jl4?file=/src/css/embla.css

using css grid slider doesnt account for gap

calculates correctly with margin-left/margin-right however

davidjerleke commented 11 months ago

Hi @drewlonious,

Thank you for the CodeSandbox. This isn't a bug but I rather think your math is a bit off. The following CSS says that:

.embla__container {
  /* ...other styles */
  grid-auto-columns: 20%;
  grid-column-gap: 2rem;
}

The total of this is: (20% 5 slides = 100%) + (2rem 4 gaps = 8rem). There are 4 gaps between all 5 slides = 100% + 8rem > 100 (which is more than the viewport width of 100%). Anything more than 100% of the container will of course overflow the container. That's how CSS works.

Change your CSS to this and you're good to go:

.embla__container {
  /* ...other styles */
- grid-auto-columns: 20%; <-- remove this
+ grid-auto-columns: calc(20% - 2rem * 0.8); <-- add this
  grid-column-gap: 2rem;
}

Best, David

drewlonious commented 11 months ago

@davidjerleke This seems like a very unintuitive solution. Gap in CSS grid doesn't effect total width of grid items unlike flexbox.

For example For a simple slider that uses more than > 1 per row, css would be

--slide-gap: 1rem;
--slides-per-row: 1;
--slides-per-row-tablet: 2;
--slides-per-row-desktop: 5;

.embla__container {
  grid-auto-columns: calc(
    (100% / var(--slides-per-row-desktop, var(--slides-per-row-tablet, var(--slides-per-row, 1)))) -
      (
        var(--slide-gap, 0) * (var(--slides-per-row-desktop, var(--slides-per-row-tablet, var(--slides-per-row))) - 1) /
          10
      )
  );
}
davidjerleke commented 11 months ago

@drewlonious Embla Carousel isn’t just another carousel library. It’s different in many ways. It has almost no side effects in contrast other libraries doing all sorts of stuff to your DOM elements. So other carousel libraries that solves everything for you with super duper convenience comes with a price: less flexibility and a lot of unpredictable stuff happening to your elements.

Embla is actually more predictable in the sense that the HTML and CSS you write is what you get. That’s also one reason it’s so lightweight. Another benefit is that it gives you the total freedom of setting up your elements and CSS any way you want. But this comes with a responsibility: You have to know how to write proper CSS.

Now to answer your question about what you call an “unintuitive” solution, you have to remember that you picked that solution. Not me. There are ways to get around the tedious way of calculating slide sizes and spacings. If you know how CSS works it’s easily solved.

One solution is to use the negative margin on the container and a corresponding amount of padding-left on each slide. This way, the spacings between the items will always be included in the slide widths, so you can just set different slide widths based on different breakpoints without worrying about calculating stuff. Here’s an example demonstrating that approach. Just go to that sandbox and change the --slize-size variable (in the css/embla.css file) to anything that adds up to 100%. For example: 20%, 25%, calc(100% / 3) and you'll see.

If this library doesn't suit your needs, please feel free to use a different library. There are too many carousel libs out there that practically does the same thing with just different naming. But don't come here and claim that this is unintuitive because it's different and you don't take time to understand how it works or the benefits of the Embla approach. There's no reason for me to build yet another carousel library doing the exact same thing like all the others. That's a total waste of time.

Poylar commented 11 months ago

@davidjerleke Hello, this was not very clear to me either. Perhaps the fact is that the documentation does not say anything about this, maybe it is worth supplementing it with a short version of your message above?

davidjerleke commented 11 months ago

@Poylar the guides section in the docs will get a total rewrite when I have the time. Until then, feel free to submit a PR with an explanation where you think it would help.

As mentioned before all examples including the carousel generator uses the padding approach mentioned here so you can see how it’s solved there.

Best, David