Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task done
Tracked by #321
MaxDesignFR opened this issue Jun 26, 2023 · 8 comments · Fixed by #524
Closed
1 task done
Tracked by #321
Labels
feature request New feature or request resolved This issue is resolved

Comments

@MaxDesignFR
Copy link

MaxDesignFR commented Jun 26, 2023

Bug is related to

  • embla-carousel (core package)

Embla Carousel version

  • v8.0.0-rc07

Describe the bug

  • When there are not enough slides in view, containScroll: 'trimSnaps' will align the slides to the left, which I suppose is expected to be centered in some case (see expected behavior).

CodeSandbox

Expected behavior

  • When both Embla.canScrollPrev() and Embla.canScrollNext() are false, the slides are expected to be centered by default. If one of those is true though, then it is expected to be aligned left, as is right now.

Additional context

  • I need to use containScroll: 'trimSnaps' because sometimes there are not enough slides in the view, and Embla can not enable loop option, the leading and trailing spaces are not wanted. When adding or removing slides, sometimes both Embla.canScrollPrev() and Embla.canScrollNext() are false, and in this case I believe Embla should align center the carousel.

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?

@MaxDesignFR MaxDesignFR added the bug Something isn't working label Jun 26, 2023
@davidjerleke
Copy link
Owner

davidjerleke commented Jul 3, 2023

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
Copy link
Author

MaxDesignFR commented Jul 3, 2023

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:

  • The breakpoints option, but here it feels more like a responsiveness problem, exact breakpoints can't always be known when we're dealing with unknown/random number of slides.
  • Removing containScroll option so that align;'center' is preserved even when there is not enough slides to fill the container. Downside is that there are leading and trailing spaces such as that: https://i.vgy.me/3BL81G.png 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.

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
Copy link
Owner

davidjerleke commented Jul 4, 2023

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:

contain-scroll.mov

Note this in the screen recording:

  • When containScroll is disabled, Embla will respect alignment for all slides which is center in this example.
  • When containScroll is enabled, Embla will override slide alignments for enough slides to remove/clear the leading/trailing space. So in this particular setup, the first and the last slide won't respect the center alignment because they need to alter positions to clear the empty space.

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
Copy link
Author

MaxDesignFR commented Jul 4, 2023

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 davidjerleke added feature request New feature or request and removed bug Something isn't working labels Jul 5, 2023
@davidjerleke
Copy link
Owner

davidjerleke commented Jul 5, 2023

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
Copy link
Owner

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

@davidjerleke davidjerleke added the upcoming A feature or bug fix is on its way for this issue label Jul 5, 2023
davidjerleke added a commit that referenced this issue Jul 8, 2023
This was referenced Jul 8, 2023
davidjerleke added a commit that referenced this issue Jul 8, 2023
Make `containScroll: 'trimSnaps'` default
@davidjerleke davidjerleke added resolved This issue is resolved and removed upcoming A feature or bug fix is on its way for this issue labels Jul 8, 2023
@davidjerleke
Copy link
Owner

To be released with v8.0.0-rc09.

@davidjerleke
Copy link
Owner

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

Best,
David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request resolved This issue is resolved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants