davidjerleke / embla-carousel

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

containScroll: 'trimSnaps' unexpected behavior when there is not enough slides in view #507

Closed MaxDesignFR closed 1 year ago

MaxDesignFR commented 1 year ago

Bug is related to

Embla Carousel version

Describe the bug

CodeSandbox

Expected behavior

Additional context

Fix I use (dirty?)

Screen capture before fix is applied: https://i.vgy.me/WpzzoW.gif Screen capture after fix is applied: https://i.vgy.me/m0D51b.gif

The only difference is that when both Embla.canScrollPrev() and Embla.canScrollNext() are false, the slides are centered instead of aligned left.

What I did for that is simple, but it's a bad experience since it feels like a "hack", and I have yet to find a better way. Here is a snippet of code that is self-explanatory:

this.container = this.querySelector('.container');
this.sliderOptions = { loop: true, slidesToScroll: 'auto', containScroll: 'trimSnaps' };
this.slider = EmblaCarousel(this.viewPort, this.sliderOptions);
this.slider.on('reInit', this.onReInit.bind(this));

onReInit() {
   const canScrollPrev = this.slider.canScrollPrev();
   const canScrollNext = this.slider.canScrollNext();
   const justifyCenter = !canScrollPrev && !canScrollNext;
   this.container.style.justifyContent = justifyCenter ? 'center' : 'flex-start';
}

// this method is used to add/remove slides. I need to reset the justify-content value, otherwise it messes with the carousel layout
buildSlider() {
   this.container.style.justifyContent = 'flex-start';
   ... logic to add and remove slides here, after which reInit method is called
}

I did not figure out a native way to align center the slides in this use case. Doing this workaround seems quite bad, can Embla core fix this, or is there a better way to handle this?

davidjerleke commented 1 year ago

Hi @MaxDesignFR,

Thank you for your question. The containScroll option will always override alignment for enough slides to cover the leading and trailing space. In this special case, when there aren't enough slides to scroll, clearing space means aligning the slide content to the left.

There's nothing wrong with your fix. I guess there are multiple ways to check if the carousel isn't scrollable because of lack of enough content. Another one being:

const justifyCenter = this.slider.scrollSnapList().length === 1;

Best, David

MaxDesignFR commented 1 year ago

Thanks for reviewing my post. Your way is actually more concise, way to go! (Edit: actually, I get error this.slider.scollSnapList is not a function (executed after init event). Using setTimeout with no argument fixes this).

You may close the issue if you like. I'll share some feedback though: I am personally a little puzzled about the containScroll option in the library, and the fact that it can override the default align: 'center' option. Why is this the expected behaviour? Maybe it could be preserved using the transform: translate3d property instead of toggling justify-content:center with custom code?

My use case is a little niche because I add/remove slides, so sometimes there are not enough of them to fill the container space ; but I can imagine having a single slider with 4/5 slides with loop:true, working fine in mobile-laptop, but desktop will have the slides stuck to the left because the container can't slide anymore. Here is what I've considered:

I'm just throwing my personal experience using Embla, I trust you are the best judge to decide what's relevant, furthermore I really like what has been done with the library, recent updates are really neat, so until this gets more attention I don't mind using the fix we discussed, it's functional.

Thanks for your attention.

davidjerleke commented 1 year ago

Hi @MaxDesignFR,

Your way is actually more concise, way to go! (Edit: actually, I get error this.slider.scollSnapList is not a function (executed after init event). Using setTimeout with no argument fixes this).

I'm sorry, I didn't spell it correctly. It's not scollSnapList() but scrollSnapList():

❌ const justifyCenter = this.slider.scollSnapList().length === 1;
✅ const justifyCenter = this.slider.scrollSnapList().length === 1;

I am personally a little puzzled about the containScroll option in the library, and the fact that it can override the default align: 'center' option. Why is this the expected behaviour?

Do you mean for this special case or in general? If the latter, watch this:

https://github.com/davidjerleke/embla-carousel/assets/11529148/de47e099-efcd-4d6e-b807-f77dba406cac

Note this in the screen recording:

Here is what I've considered:

Removing containScroll option so that align;'center' is preserved even when there is not enough slides to fill the container.

Have you tried something along these lines?

let previousIsScrollable = true

function centerIfNotScrollable(emblaApi) {
  const isScrollable = emblaApi.scrollSnapList().length === 1;
  if (isScrollable === previousIsScrollable) return;

  const containScroll = isScrollable ? 'trimSnaps' : null
  previousIsScrollable = isScrollable
  emblaApi.reInit({ containScroll });
}

const emblaApi = EmblaCarousel(emblaNode, {
  containScroll: 'trimSnaps',
  loop: true
});

emblaApi
  .on('init', centerIfNotScrollable)
  .on('reInit', centerIfNotScrollable)

By the way I wonder what are the use cases for having blank spaces around the container, for me it feels like it should not be default behaviour.

Devs use this setup sometimes, especially when slide widths are small like 25% or similar. So I don't think we should remove that option. But I agree that maybe containScroll should be enabled by default. I've actually considered this but never changed it because no one has asked for this up until now.

furthermore I really like what has been done with the library, recent updates are really neat

Thanks a lot! The Secret project will be revealed soon so be sure to check that out.

Best, David

MaxDesignFR commented 1 year ago

I'm sorry, I didn't spell it correctly. It's not scollSnapList() but scrollSnapList():

Well that's my bad too. I must have copy paste first, hence the this.slider.scollSnapList is not a function, and then I must have use the good syntax in setTimeout, drawing a false conclusion, so silly ;)

I get your point for containScroll and I reckon it can have its use cases where the leading & trailing spaces are prefered. Though I would argue that having blank spaces by default (they can be large) is most likely to be perceived as a problem to fix with containScroll: 'trimSnaps', meanwhile not having them by default looks perfectly fine, and the option to have those blanks spaces still exist. I'm trying to be objective but at the same time, it is also a personal preference.

That said, I remember going through closed issue to find some videos about differences with trimSnaps and keepSnaps, I somehow get it now, but I believe the documentation for this option may not be enough, especially because we do not visualize/understand the cogs of snap points. If that was for me, a video link in the doc for containScroll option — to highlight the differences — would have been very appreciated, because this is one of the few options that is quite hard to get until you see the difference.

Have you tried something along these lines?

function centerIfNotScrollable(emblaApi) {
  const isScrollable = emblaApi.scrollSnapList().length > 0;
  const containScroll = isScrollable ? 'trimSnaps' : null

  emblaApi.reInit({ containScroll });
}

const emblaApi = EmblaCarousel(emblaNode, {
  containScroll: 'trimSnaps',
  loop: true
});

emblaApi
  .on('init', centerIfNotScrollable)
  .on('reInit', centerIfNotScrollable)

This code triggers reInit recursively does it not (which I suppose would fail)? For now the justify-content:center works for me, seems more of a hack but does the job. Also something odd that I noticed, for me emblaApi.scrollSnapList() logs [NaN] when the container can't scroll anymore, which therefore has a length of 1. Just putting this out there in case.

I'm definitely keeping an eye on the Secret project, I did have a look at this issue, but to my regret nothing was leaked ;)

davidjerleke commented 1 year ago

I get your point for containScroll and I reckon it can have its use cases where the leading & trailing spaces are prefered. Though I would argue that having blank spaces by default (they can be large) is most likely to be perceived as a problem to fix with containScroll: 'trimSnaps', meanwhile not having them by default looks perfectly fine, and the option to have those blanks spaces still exist.

I agree and think your reasoning makes sense. I will make containScroll: 'trimSnaps' default in the stable v8 release which at the time of writing is just around the corner 🙂. I'm going to label this issue a feature request though because it was designed this way when it was built.

That said, I remember going through closed issue to find some videos about differences with trimSnaps and keepSnaps, I somehow get it now, but I believe the documentation for this option may not be enough, especially because we do not visualize/understand the cogs of snap points. If that was for me, a video link in the doc for containScroll option — to highlight the differences — would have been very appreciated, because this is one of the few options that is quite hard to get until you see the difference.

Thanks for the feedback! Yes, it's a challenge to explain how containScroll works and I have actually planned to rewrite the guides section in the docs with visual demonstrations.

My biggest challenge by far with this library is to find ways to automate things and get rid of repetitive questions about core functionality - especially because I'm the sole dev in this project. I want to offload that kind of repetitive work to focus more on the library core, build more features, plugins and improve the documentation. Bottom line: I have this planned and I'm also hoping that the Secret project will reduce the number of questions I get about basic Embla functionality.

This code triggers reInit recursively does it not (which I suppose would fail)? For now the justify-content:center works for me, seems more of a hack but does the job. Also something odd that I noticed, for me emblaApi.scrollSnapList() logs [NaN] when the container can't scroll anymore, which therefore has a length of 1. Just putting this out there in case.

Yes, sorry about that. I was pretty tired when writing that and I missed two things:

  1. You should check for the length 1 and not 0. Because when the carousel isn't scrollable it still has one scroll snap, not zero:
const justifyCenter = this.slider.scrollSnapList().length === 1;
  1. About the recursive infinite loop: I totally forgot adding a variable previousIsScrollable: boolean to check what the previous state was in order to conditionally run reInit() only when it has changed. But as you say adding the justify-content: center; is probable easier to do.

About the [NaN] thing when there's only one scroll snap I've noticed that. But it doesn't break any functionality and only happens when there's a single snap so I haven't prioritized that among other things.

Best, David

davidjerleke commented 1 year ago

Note to self: make containScroll: 'trimSnaps' default.

davidjerleke commented 1 year ago

To be released with v8.0.0-rc09.

davidjerleke commented 1 year ago

@MaxDesignFR the default value for containScroll is now 'trimSnaps' in v8.0.0.-rc09.

Best, David