davidjerleke / embla-carousel

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

[Bug]: Loop option disregards viewport padding, causing visual artifacts #1045

Closed MaxDesignFR closed 2 weeks ago

MaxDesignFR commented 1 month ago

Which variants of Embla Carousel are you using?

Steps to reproduce

The bug occurs when loop option is activated and the embla viewport has padding. The first and last slide transforms while still being visible in the viewport. I suppose the padding is not taken into calculation.

What's the point of adding padding to the viewport?

There may be some use cases, the most common one (from experience) is to have a more immersive sliding experience to avoid hard cuts at the edges of the carousel. This is done by compensating the padding with negative margins. This is necessary because the carousel is quite likely to be inside a parent node with padding, but in most cases you don't want the carousel to respect parent padding for the reason I just explained. Here's a screen capture to illustrate the issue: https://files.maxdesign.expert/2024/10/chrome_OTU8FEnph5

Expected Behavior

The loop option should take into account viewport padding to prevent a slide from transforming while still being in view.

Additional Context

I suppose I could try to work around this and add an extra wrapper to apply the padding + negative margins:

<div class="wrapper">
   <div class="embla">
     <div class="embla__container">
       <div class="embla__slide">Slide 1</div>
       <div class="embla__slide">Slide 2</div>
       <div class="embla__slide">Slide 3</div>
     </div>
   </div>
</div>

I haven't tried that but I guess that should work. But it seems like a extra hassle, and I believe that in real use case the carousel can indeed be contained in a parent with padding, and that this should not be detrimental to having hard cut edges or forcing us to add an extra wrapper solely for this purpose.

It's why I posted this as an issue and not a feature request.

What browsers are you seeing the problem on?

Chrome

Version

8.3.0

CodeSandbox

https://codesandbox.io/p/sandbox/embla-carousel-loop-vanilla-forked-4q8kgn?workspaceId=1baa5a85-c84b-452a-95ef-fa2f70f71ebe

Before submitting

davidjerleke commented 1 month ago

@MaxDesignFR thanks for your question. Please make the sandbox public so I can access it.

MaxDesignFR commented 1 month ago

Sorry about that. It's now public.

davidjerleke commented 1 month ago

@MaxDesignFR could you show me an image or similar of what you want to achieve with that padding? I don't understand at this point.

MaxDesignFR commented 1 month ago

Sure, highly appreciate your prompt remply!

Here's an example from a real project. The carousel would typically be nested in a section with padding. Most of the times, by design you can't necessarily avoid that. image

Now to get an immersive scrolling view, you'd need to apply the same padding + negative margins to the carousel viewport (or add an extra wrapper but that's a lot of nesting), so that you don't get hard cut edges, otherwise this would look like that:

image

But the issue with having padding on the viewport with loop options, is that the slide transforms before it gets out of the padded area, here's a screen capture:

https://github.com/user-attachments/assets/803bf4c3-7554-4291-847c-3fc8b069d232

davidjerleke commented 1 month ago

@MaxDesignFR it's expected that the carousel breaks if you start adding stuff to the viewport that covers the overflow. So it's not a bug. Why don't you set negative margin on the element that wraps the viewport instead? Like so:

<div class="embla"> <!-- Add negative margin here that eliminates the page frame padding -->

  <div class="embla__viewport">
    <div class="embla__container">
      <div class="embla__slide">Slide 1</div>
      <div class="embla__slide">Slide 2</div>
      <div class="embla__slide">Slide 3</div>
    </div>
  </div>

</div>
MaxDesignFR commented 1 month ago

Yes I also thought possible to add an extra wrapper, but my point is that:

  1. We'd have to add a wrapper for this sole purpose of avoiding hard cut edges
  2. I'd expect that items should not disappear while being in the viewport. Since padding creates extra space within an element, embla should not do transform operations within the padding area

It's just my view on this, I'm not saying I'm right. The negative margins do not create the issue, it seems to be the padding on the viewport. It's totally possible to add an extra wrapper, but I thought padding might be handled differently to avoid this type of visual artefact.

Let me know if I can provide more details or use cases. If you'd like to dismiss the issue it's no problem, I reckon there is a valid workaround but I don't see it as a solution necessarily. I suppose it also depends if considering the viewport padding messes with other working parts of embla.

davidjerleke commented 4 weeks ago

Yes I also thought possible to add an extra wrapper, but my point is that:

  1. We'd have to add a wrapper for this sole purpose of avoiding hard cut edges I'd expect that items should not disappear while being in the viewport. Since padding creates extra space within an element, embla should not do transform operations within the padding area

@MaxDesignFR I see no problem with this because Embla is designed this way. Two elements you should be careful adding styles to are:

Because it has to have some limitations in order to function properly. If you respect that, you can do almost anything to all other elements in contrast to most other carousel libraries. Embla is one of the libraries that give developers nealy total control over their HTML.

The alternative would be that the library solves the thing you mention for convenience. But the price tag for that is high. We would have to make internal calculations for all possible option combinations more complex than they already are, and the library would gain weight. Additionally, there are other problems related to intent interpretation: What does the user want to achieve with padding here and margin there? How should the carousel behave with different options set? I don't think it's worth it if you can just wrap the core elements of the carousel with an additional element - which is much easier than making the library significantly harder to maintain.

I also want to mention that even though you or any other dev expect this library to behave in a certain way, it won't be considered a bug if they're by design. Which is the case here.

With that said, thanks for your suggestion and I appreciate you taking time to share it 👍.

Best, David

MaxDesignFR commented 4 weeks ago

Hi David,

Thanks for taking the time to wrap your head around this. I can’t fully evaluate what’s involved in considering padding for the viewport. If it’s a high price tag, I trust your assessment; it’s no deal breaker to add an extra wrapper. At this point, I'm mostly sharing feedback.

That said, I still think my point is somewhat valid and not overly niche. Just looking at embla example page in mobile, you can see that all carousels have hard-cut edges due to padding on a parent element (a common design approach that's hard to avoid).

I firmly believe it’s better design to avoid those hard edges on mobile; it’s visually more appealing, offers more usable space, and makes for a better experience overall. it just makes for a better experience. Conversely, I don’t see any advantage to the hard-cut design. From auditing many websites using various carousels, this more immersive design seems to be gaining popularity.

If this approach is or become the design norm for mobile carousels, which it should (not that there’s a set "norm" but you get the idea), it would imply that Embla should always be wrapped in an extra wrapper div by default doesn't it? Why not but this isn’t addressed in the docs and would also result in a lot of div nesting imo. That’s why I was hoping for viewport padding to be accounted for when translating slides in loop mode. Margins on the viewport works as expected so I don't see a problem with that. As for padding interpretation, I reckon this should be clear how padding can affect the carousel (I understand that at this point it's not supposed to be used at all).

Just food for thought. I'm a UI/UX creep and I reckon not all front-end devs mind about this stuff. I’m happy to discuss further, but you can definitely close the issue, adding an extra wrapper is definitely acceptable if that makes the library ligther and/or easier to maintain.


Edit:

Actually, now that I tried to achieve this with an extra wrapper, it don't seem possible with the requirement that no padding can be applied to the viewport. I tried a lot of things but I can't get it right. The design I'm talking about (no hard-cut edges) seems obtainable as long as:

  1. There is no padding on any parent element or
  2. Loop is not used (in which case I don't see adverse effect of adding padding + negative margins to the viewport)
davidjerleke commented 4 weeks ago

That said, I still think my point is somewhat valid and not overly niche. Just looking at embla example page in mobile, you can see that all carousels have hard-cut edges due to padding on a parent element (a common design approach that's hard to avoid).

I firmly believe it’s better design to avoid those hard edges on mobile; it’s visually more appealing, offers more usable space, and makes for a better experience overall. it just makes for a better experience. Conversely, I don’t see any advantage to the hard-cut design. From auditing many websites using various carousels, this more immersive design seems to be gaining popularity.

@MaxDesignFR I'm not arguing against your opinion that hard cut edges is nicer. On the contrary, I agree that it's nicer without. But I haven't prioritized to fix the hard cut edges on the documentation website. It's super easy to solve with an extra wrapper though.

Many devs that come here with input assume I have infinite time to spend on this project. I don't. With two toddlers, I have 15-20 minutes every three days to spend on this project. I have to make that time count and make prioritizations. And this project has no decent funding/sponsors so I don't see that changing soon. Since hard cut edges is super easy to solve with an extra wrapper, I don't focus on that but on more important things. As I mentioned there's a cost of making calculations more complex.

And in your case, I would actually argue that you should have the following in your toolkit when building websites:

I have these components in all my projects because I know that many customers want some sections to respect the page frame and some to span full width of the screen without hard cuts. My first thought wouldn't be arguing that libraries I use should solve this locally.

Here's a sandbox with a little example of a trivial page frame (my page frame components are usually more sophisticated in my projects) I've created just to demonstrate what I mean:

Screenshot 2024-10-30 at 10 51 15
MaxDesignFR commented 4 weeks ago

I get your point, you don't even owe me an explanation by the way. Despite that I figure you must care for this project given the time you give here, it's really a gem of a repo, tell that to your toddlers all right ;)

In all seriousness I have considered sponsoring but I'm not in a confortable position to do so, sharing feedback is my contribution and I don't expect you to do anything with it or even consider it, it's your call entirely (I reckon it can be a time waster to try to please everyone and provide solution people can't figure out by themselves). So just saying, it's okay to disregard.

Back to the topic: (I've moved on, just dicussing as long as you're interested). Your demo is viable because there is enough slides to loop. But if there's not enough slides I'd encounter the issue where I can't deal with the carousel outer padding: https://codesandbox.io/p/sandbox/embla-carousel-loop-vanilla-forked-vvt3t3?workspaceId=1baa5a85-c84b-452a-95ef-fa2f70f71ebe

So the way I see it, since it's not necessarily predictable whether there will be enough slides to loop (in my case it's not), I could check the loop option value in the internalEngine and adjust some class or css dynamically to preserve the initial outer padding when loop is false. But I don't really want to go this route because it feels like working around a problem, especially as a new user (don't get me wrong, it's still nice that Embla opens its internalEngine to allow for that), Personally I have skipped the loop option and it's no deal breaker.

I may be completely off but it's a limitation I have however niche it may be, or I'm just less good than the average front-end and can't figure out a more obvious solution (booh me). And so I'm looping back to my original point (pen intended), as it seems to me padding + negative margins on the viewport would not require to go through this process, thus making the library more approachable and working out of the box for this type of scenario. That said I agree with all that you said before.

Slightly off topic, maybe it could be a nice addition to add an example of a full frame mobile carousel in the documentation.

davidjerleke commented 3 weeks ago

@MaxDesignFR thank you for your kind words.

I hope I didn't come across as if I'm begging for sponsorship. I'm not. Just wanted to simply explain that I have very little time, and because of that my hands are tied and I have to prioritize hard. As you say, contributions can also get things done.

Your demo is viable because there is enough slides to loop. But if there's not enough slides I'd encounter the issue where I can't deal with the carousel outer padding.

I don't see the need to change padding in these cases because you can align your slides just like a loop carousel when loop is disabled by setting containScroll: false:

const OPTIONS: EmblaOptionsType = {
  loop: true,
  containScroll: false, // Add this. It will only take effect internally if loop is false.
}

Here's the updated sandbox for demo purposes. And here's how it will look when it falls back to loop: false:

image
MaxDesignFR commented 3 weeks ago

Okay that's interesting even though I don't think it will get me where I want.

I was not aware of containScroll: false. I just noticed in the documentaiton that false is allowed as type, but the doc only explains 'trimSnaps' and 'keepSnaps', which imho I don't think is possible to visualize what they do unless experimenting with them and see how it affects the carousel. Right from the doc I could not see that containScroll: false will only take effect internally if loop is false and I can't see what it would do compared to trimSnaps for instance. I believe we already discussed containScroll in the past and I still believe to this day it deserves more explanation in the doc ; it may be the only option in Embla that is not self-explanatory. Your doc is truly amazing and a stregnth for Embla, I think adding some visual hints of what it does would help (a video preview in a modal or a link to a predefined example with the same carousel under each containScroll option).

Even then, what I'm trying to achieve seems difficult with Embla unless I disable loop, here's my take on it: https://files.maxdesign.expert/2024/11/chrome_3bx8gDIK0H

I'm not saying you have to figure out the solution for me or change Embla, I'm just saying that if that's even possible it's not simple to figure it out by using Embla straight out of the box. The way I see this it's better for me to not use loop or I'd be manipulating the css dynamically based on the loop state which I can get from internalEngine (a little convoluted thus I'd rather not use loop).

davidjerleke commented 3 weeks ago

@MaxDesignFR I understand now. Thanks for explaining. So what you want to achieve has to be done like this then. And here's the demo.

I was not aware of containScroll: false. I just noticed in the documentaiton that false is allowed as type, but the doc only explains 'trimSnaps' and 'keepSnaps', which imho I don't think is possible to visualize what they do unless experimenting with them and see how it affects the carousel. Right from the doc I could not see that containScroll: false will only take effect internally if loop is false and I can't see what it would do compared to trimSnaps for instance. I believe we already discussed containScroll in the past and I still believe to this day it deserves more explanation in the doc ; it may be the only option in Embla that is not self-explanatory.

Yes this is missing due to my lack of time unfortunately. I personally don't need the docs because I've built this and I'm maintaining it so know how it works. Basically the docs will become whatever developers are ready to contribute with.

MaxDesignFR commented 2 weeks ago

Hi David,

Thanks for following up on that and providing a demo, as I thought going throuhg the internalEngine() was the way to go. This is partially why I'd likely never turn my back on Embla because we can always fix up something to fit any scenarios. Althgouh as a new comer I think this is far from ideal to have to go through this, maybe it's too niche and most won't bother, but it's how I need most of my sliders to work/look in loop mode. Without loop I apply padding directly to the viewport ; although I should not I have not noticed drawbacks.

Basically the docs will become whatever developers are ready to contribute with.

I hear you and it makes total sense (taking notes ;)). Thanks again for taking the time to investigate this. I'll close the issue since the workaround you provided is managable.

davidjerleke commented 5 days ago

Hi @MaxDesignFR,

Just wanted to let you know that the ClassNames plugin now has a new feature that sets a loop class name on the carousel in cases when the carousel has enough slides to loop.

So if you're using ClassNames, it's a breeze to style your carousel based on loop/non-loop carousels.