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

Snappoints on showing multiple slides #19

Closed
mrksmts opened this issue Aug 9, 2019 · 19 comments
Closed

Snappoints on showing multiple slides #19

mrksmts opened this issue Aug 9, 2019 · 19 comments
Labels
feature request New feature or request good first issue Good for newcomers resolved This issue is resolved

Comments

@mrksmts
Copy link

mrksmts commented Aug 9, 2019

Currently I am implementing Embla Carousel in a project where we have multiple slides in the viewport at once, this basically means 2.5 slides are always visible.

When we containScroll and align start the items the snappoints for slide 4 and 5 are the same so it feels there should be one less snappoint for these settings.

I've setup a sandbox forked from one of the other issues to explain this issue: https://codesandbox.io/s/embla-carousel-loop-false-xdpsb?fontsize=14

@davidjerleke
Copy link
Owner

davidjerleke commented Aug 9, 2019

Hello Mark (@mrksmts),

Thank you for taking the time to report this issue + the clear sandbox example 👍🏻.

Have you tried setting the slidesToScroll option to 2? When you have 2 fully visible slides in viewport you can cut redundant snap points using this option.

I’d very much appreciate if you can confirm if this is the functionality you’re looking for.

All the best
David

@davidjerleke davidjerleke added the awaiting response Issue is awaiting feedback label Aug 9, 2019
@mrksmts
Copy link
Author

mrksmts commented Aug 12, 2019

Hi David (@davidjerleke)

I am not sure that would work because we would like to be able to slide per item.

And in the example the slide width is 40% so it would always be 2 but If you make it 250px for example and the carousel 100% it really depends on the width of the viewport and the amount of slides in the view that would determine the snap points.

Best,

Mark

@davidjerleke
Copy link
Owner

davidjerleke commented Aug 12, 2019

Hi Mark (@mrksmts),

I understand your point of view. In the example you provided slide 4 and 5 share the same snap point because of containScroll that cuts excessive scroll at the end.

I don’t know what a good solution for this would look like. The current implementation of containScroll is by design and doesn’t merge snap points even if they are the same (works pretty much like Flickity’s contain). Implicitly merging snap points could lead to confusion for dev users, for example when using options like slidesToScroll.

Maybe we should consider a new option. What do you think?

@mrksmts
Copy link
Author

mrksmts commented Aug 14, 2019

Hi David (@davidjerleke)

What do you think about something like a boolean called trimSnapPositions which basically removes all snap points that are the same? Then you could leave it up to the user.

@davidjerleke
Copy link
Owner

Hi again Mark (@mrksmts),

Thanks for getting back to me. Let me try this in code and see if it’s an easy fix or if there are some unexpected consequences with this 🧐.

I’ll get to it as soon as possible, I have a newborn getting my primary attention at home so thank you for being patient 👶🏻🍼.

Best,
David

@davidjerleke davidjerleke added feature request New feature or request and removed awaiting response Issue is awaiting feedback labels Aug 14, 2019
@SachaMPS
Copy link

Wanted to ask the exact same thing and found this open issue right now.. made more or less the same example... https://codepen.io/sachamps/pen/XWrXvGq

If you could point me to the best place in your code to get a hand on it i would at least try to make a PR which you could review... If not no problem... Congratulations 👶🏻!!!

@davidjerleke
Copy link
Owner

davidjerleke commented Aug 14, 2019

Welcome Sacha (@SachaMPS),

Thank you 👶🏻.

And another good example illustrating the feature request clearly 👍🏻.

What’s your opinion on this?

  1. Snap points that share the same position and don’t trigger any scroll should be merged.
  2. It should be an option for the user.

I’ll point out the code responsible for this as soon as I’ve access to my computer 🙂.

Thank you in advance,
David

@SachaMPS
Copy link

SachaMPS commented Aug 14, 2019

HeyHo @davidjerleke,

i am just reading (and logging :) through your code to find out how all the stuff works.

Right now I would try not to mess with the anything too much at all. I am asking myself if the API call canScrollNext in that case should return false.

EDIT:

As an example what I did on my first try to achieve that

engine.ts - expose a new constant distinctPositions

const distinctPositions = [...new Set(snapPositions)]

index.ts - compare the index with the length of distinctPositions

function test(): boolean {
    const { index, distinctPositions } = slider
    return options.loop || index.get() < (distinctPositions.length - 1)
  }

@davidjerleke
Copy link
Owner

davidjerleke commented Aug 14, 2019

Hi Sacha (@SachaMPS),

We could make canScrollPrev() and canScrollNext() recognise when the carousel is scrollable or not, which is a step in the right direction, but that doesn't solve the problem with dot navigation.

Example:
Let's say we have a carousel setup with the following:

  • 5 slides
  • containScroll: true
  • align: 'start'

👉 See CodeSandbox

In order to setup dot navigation there's a useful API method scrollSnapList(). In the example above, scrollSnapList() will return 5 snap points. However, only 3 of them will trigger an actual scroll. So when we've reached slide 3, the next button won't do anything/scroll further. Now if the dot navigation shows 5 dots it could be confusing for the user:

embla

We need to solve this too...

@SachaMPS
Copy link

SachaMPS commented Aug 14, 2019

@davidjerleke I am not sure if a dot navigation makes sense width that special kind of UserOptions at all... I mean what does the Dot-Navigation mean in that case... I think having 5 slides but only three dots to navigate would be totally irritating. But maybe you are right and talking about UX aspects isnt the right way to think about it..... I will take a look at scrollSnapList tomorrow and give this thing another try...

@davidjerleke
Copy link
Owner

davidjerleke commented Aug 14, 2019

@SachaMPS, it’s not obvious to me that the number of slides has to match the number of dots. In a real world usage example carousels generally don’t have obvious slide numbers like in the example I sent you. They often contain images or maybe a combination of text and an image. I’m not sure if users will be aware of or bother about the number of dots, as long as all slides are reachable?

If slide widths are small enough so 2 or more slides are visible in viewport at the same time, it might be equally annoying for the user to scroll by each slide one at a time. Therefore there is a slidesToScroll option available which is a feature many Carousel plugins have. Take a look here:

👉🏻 CodeSandbox

In the example above slidesToScroll is set to 2 because we have 2 full slides visible in viewport.

That’s why I think we should consider merging excessive snap points that don’t trigger any scroll. But as you can see in the discussion above, I’d be happy if we can find a good solution for this.

@davidjerleke davidjerleke added the investigating Issue is being looked into label Aug 15, 2019
@davidjerleke
Copy link
Owner

davidjerleke commented Aug 19, 2019

Hi Mark (@mrksmts),

Just want to let you know that I'm working on this. I've not decided if:

A. Snap points that don’t trigger any scroll will be merged automatically.
B. It should be an option for the user.

But at this point I've tried this in code (in an ugly way) and I have a working draft. I'm hoping to get some time soon to finish this and wrap it up.

Best,
David

@mrksmts
Copy link
Author

mrksmts commented Aug 20, 2019

Hi David (@davidjerleke)

Thanks! That's great to hear. You could put it in by default and give the user the option to turn it off or the other way around.

Let me know if I can test or help with anything.

Best,
Mark

@davidjerleke davidjerleke added upcoming A feature or bug fix is on its way for this issue and removed investigating Issue is being looked into labels Aug 22, 2019
@davidjerleke davidjerleke added awaiting response Issue is awaiting feedback and removed upcoming A feature or bug fix is on its way for this issue labels Aug 30, 2019
@davidjerleke
Copy link
Owner

davidjerleke commented Aug 30, 2019

Hi Mark (@mrksmts),

I'm very happy to announce that release v2.4.0 comes with the functionality you're looking for 🚀.

I decided to go with Embla automatically merging excessive snap points that share the same position, rather than making it an option. The reason being is that I couldn't think of any use case where it makes sense to have navigation that triggers no scroll at all.

I'd very much appreciate if you could test the new behaviour and give me feedback. Install the latest version:

npm install embla-carousel@latest

...or

yarn add embla-carousel@latest

Thank you in advance 👍.

Kindly,
David

@davidjerleke
Copy link
Owner

Hello Mark (@mrksmts),
Just curious, have you had the change to try if this is working as expected yet?

Thanks!
David

@davidjerleke davidjerleke added the resolved This issue is resolved label Sep 13, 2019
@davidjerleke
Copy link
Owner

Closing due to inactivity. Feel free to re-open this issue if you don’t think it’s working as expected.

Thanks for opening this issue and improving Embla 👌🏻!

@davidjerleke davidjerleke added good first issue Good for newcomers and removed awaiting response Issue is awaiting feedback labels Sep 15, 2019
@mrksmts
Copy link
Author

mrksmts commented Dec 3, 2019

Hi @davidjerleke!

Sorry I didn't take the time get back to you earlier, seems like this works as expected! It is live and working like a charm! Thanks a lot!

Best,

Mark

@davidjerleke
Copy link
Owner

Hello Mark (@mrksmts),

Thank you for getting back to me, I appreciate it. And I'm glad it seems to be working as expected. Enjoy!

Kindly,
David

@davidjerleke
Copy link
Owner

Hello Mark (@mrksmts) and Sacha (@SachaMPS),

I just wanted to let you know that I'm going to release Embla v.3 as soon as possible which will allow devs to choose between keeping redundant snap points or trimming them when using containScroll:

const options = { containScroll: 'trimSnaps' }

// or

const options = { containScroll: 'keepSnaps' }

Cheers!
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 good first issue Good for newcomers resolved This issue is resolved
Projects
None yet
Development

No branches or pull requests

3 participants