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

Improve carousel behavior when slide content width is inferior to container's one #61

Closed
aurelienbobenrieth opened this issue May 10, 2020 · 10 comments
Labels
feature request New feature or request resolved This issue is resolved

Comments

@aurelienbobenrieth
Copy link

aurelienbobenrieth commented May 10, 2020

Hi ! First of all, thanks for this great library.

I am actually working on an infinite looping carousel with dynamic content (meaning various slides number). I ran into a bothering problem when it comes to having a carousel containing less slides than needed to properly loop without layout breaking.

If the number of slides times the width of a slide is inferior to the width of the container plus half the width of a slide, embla does not manage to find a nice solution to display items.

It should imho, either not init embla and leave the layout as it is or clone items to fit container's width.

You can see what I mean with the examples below :

@davidjerleke
Copy link
Owner

davidjerleke commented May 11, 2020

Hello Aurélien (@aurelienbobenrieth),
Thank you for your kind words 👍.

And thanks for taking time to open this issue. I also appreciate the clear CodeSandbox examples you provided ⭐. Just a small detail regarding the carousel fixed sandbox. Seems like a lot of people don't know this, but Embla supports mixing different slide widths out of the box, so it's not safe to assume the slide widths will be the same.

However, I think it's worth to mention that the behaviour you mention isn't necessarily a bug. Different carousel libraries handle this in different ways, or not at all. For example, the Flickity library doesn't, even though you have to pay to use it commercially. Other libraries clone HTML and so on.

I don't think cloning and start messing with HTML is a good solution in order to make the loop functionality work, when there's not enough slides to cover the loop gap. The component responsible for looping the slides actually has the information needed in order to decide if the user has enough slides to cover the loop gap or not. So right now I'm working on Embla v.3 (next major version) and my plan is to make Embla fallback to loop: false when the user doesn't have enough slides to cover the loop gap.

What's your thoughts about this solution?

Kindly,
David

@aurelienbobenrieth
Copy link
Author

aurelienbobenrieth commented May 13, 2020

Hello David and thank you for your quick reply.

Considering the fact that slides can have different widths, you would still be able to compute every slide width within a foreach loop or so. To enable loop you would just need to have slides width superior to slider's one.

However, with this behavior an issue still remains on the current version. When you enable looping and you have slides with width slightly superior to slider's one BUT an odd number of slides you still have a broken display on some resolution. This is due to two things :

  • The fact that you never clone any slide
  • The fact that carousel is always centered on one item
    Which cause to having necessarily a gap on one side of the slider (see attached below)

embla

About your proposed fallback solution, I am not sure it would solve the issue since it would change the desired layout for no reason in most of the cases (I mean why would you want to create a slider if all the content can be seen without it). The most suitable solution imho would be just not to init slider.

@davidjerleke
Copy link
Owner

davidjerleke commented May 13, 2020

Hi Aurélien (@aurelienbobenrieth),
Thanks for the additional explanation.

Considering the fact that slides can have different widths, you would still be able to compute every slide width within a foreach loop or so.

Of course, sorry for being unclear. I didn’t argue against your solution. My point was that a potential solution needs to cover the possibility of variable slide sizes.

The fact that carousel is always centered on one item

This isn’t always the case. Two options can manipulate this:

I think it makes sense as you say, not to initialize the carousel when the slide content doesn’t cover the viewport. But there’s a case where the slides will cover the viewport just slightly, but not enough to make the loop effect work. In these cases, preventing the carousel from initializing would make a small area of the sliding content inaccessible. In these cases, falling back to loop: false would solve that issue.

So this is my suggested solution in short:

  • Not enough slide content to cover the viewport => Don’t init the carousel at all.
  • Enough slide content to cover the viewport but not to cover the loop effect => Fallback to loop: false.

Does this make sense to you?

Kindly
David

@aurelienbobenrieth
Copy link
Author

Oh yes thank you, I did not think about slidesToScroll/align, maybe I should try to play with these params, and eventually adjust them dynamically according to content.

And considering the two fallbacks solution you're suggesting, it now totally makes sense for me :)

Thank you for taking my talks into consideration,
Kind regards,
Aurélien

@davidjerleke davidjerleke added the feature request New feature or request label May 14, 2020
@davidjerleke
Copy link
Owner

Thank you for opening this issue and taking time to share your ideas and thoughts Aurélien (@aurelienbobenrieth). I'm happy to discuss improvements and new ideas 👍.

I'm currently working on the next major version Embla v3, and I'm hoping to solve this together with that release. If not, I'll work on this right after the release of v3.

This feature will follow this specification.

Best,
David

@davidjerleke
Copy link
Owner

Hello again Aurélien (@aurelienbobenrieth), I hope you're well 👍.

I've been working on this feature for a while and realized that not initializing the carousel when its content isn't enough to cover the carousel viewport could lead to unwanted behavior. If you're using Embla and utilize the align option, you would expect the slides to align according to your alignment choice.

Here's a carousel with align: center initialized:
initialized

And here's the same carousel not initialized:
not initialized

Also a case where you want the alignment to work even when the content doesn't cover the viewport:

not-enough-to-cover-viewport

With that said, I've implemented the check if the carousel has enough content to loop. If not, it will fallback to loop: false automatically. This will be released with Embla v.3, so thank you for taking time opening this issue 🙂.

Cheers!
David

@aurelienbobenrieth
Copy link
Author

Hello David,
I'm fine thanks for asking, hope you're doing great as well !

For the first problem I ended up coping it with a thing like such :

.carousel {
     justify-content : center;        // Choose the desired alignment

     &.is-inited {                    // Custom class when embla is inited
        justify-content : unset !important; 
    }
}

@davidjerleke
Copy link
Owner

davidjerleke commented Jun 9, 2020

Thanks for your input @aurelienbobenrieth 👍.

Of course, It's solvable like you suggest. But seems like Embla users are used to the automatic alignment so I will keep this.

But the loop fallback part is solved and turned out great thanks to your suggestion.
Again, thanks!

Partly implemented and released with version 3.

@davidjerleke davidjerleke added resolved This issue is resolved and removed next major version labels Jun 12, 2020
@jordanade
Copy link

My users are manually cloning items to solve this issue. Wish there was a automatic solution for this.

@davidjerleke
Copy link
Owner

@jordanade this is a design choice to prevent a lot of bugs and weird behaviour. It's always best if users do this themselves so they can have full control over event listeners and similar that can't be cloned. Please read this:

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

No branches or pull requests

3 participants