Vue-carousel: Bug: Pagination has extra dot

[quinnlangille]

Following our recent updates to slide performance, a small bug has occurred where pagination dots are extending one unit past our slide count, see below for an example:

This is likely due to some rounding logic somewhere in the new calculations. Should be an easy fix and great for a first time contributor!

[kulaone]

@stop2stare, I like your fix, but do we need to remove dots, or maybe color them as selected?
The dots allow the viewer to know how many slides are out there.

[RyanLiu0235]

@kulaone , I agree with your opinion.

The dots allow the viewer to know how many slides are out there.

But if there are more than one slide per page, say n, and total slideCount is m, then we can only slide m - n + 1 times in one direction. If so, what does the extra n - 1 dots supposed to mean? When we click the extra dots, the carousel won't react anyway.
So the problem becomes:

Do dots means slideCount or times we can slide.

[kulaone]

Do dots means slideCount or times we can slide.
@stop2stare, It's a good question after all the element called pagination, so it needs to be functional as getting the viewer to all possible pages. But as viewers, we got used to seeing it also as a visual "counter" of how many objects/slides are there.

I think my doubts come from the fact that I'm using vue-Carousel in a slightly different way. I disable the pagination and build an icon menu to navigate between slides.
In my case, I marked all the visible slide's icons as "Active".
For example, in a carousel of 6 slides and perPage = 3:
The "menu" start as: 111000
If the viewer clicks on the second icon the carousel will move to the second slide the then menu will be: 011100
But the last 3 icons will point to the 4th slide and the menu will look like 000111 So as you say the carousel won't react but the viewer gets a scene of "where s/he is"

Maybe, best will be to create a new element on vue-Carousel called "Menu" that will bring this functionality to all. I already have the code, just need to make it more global.

[RyanLiu0235]

@kulaone alright, I get it. Maybe we need to highlight perPage dots at the same time rather than one. In this case, dots will make no misunderstanding and the menu functionality is also met.

[RyanLiu0235]

Hi @kulaone , what do you think of this solution?

highlight perPage dots at the same time rather than one.

I mean I do agree that we need some kind of functionality like menu, considering the situation that multiple slides in one page. But I am still not sure that if we should create a new component Menu to meet this need. Maybe we can modify Pagination a little bit instead.
Looking forward to your reply.😄

[kulaone]

@stop2stare,
I like the idea of using pagination is a menu. It will also enrich its functionality. Like the adding a value for the pagination location (top, right, left, bottom)

[quinnlangille]

I think the right solution involves addressing both of your points. @stop2stare's solution will definitely address the 7 dots on a 6 page slide scenario. However, since this issue is just a bug fix, would it be better to open a feature request for more robust pagination? @kulaone your addition sounds great, though I'd like to see what options would look like without adding a new component as well - I'm not against creating a new one, it as long as it's necessary and the issue can't be solved using our existing components. Excited to see what you guys come up with 🎉

[RyanLiu0235]

@quinnlangille I am working on a refactoring of Pagination, but there is something that I have to make sure with you.
The concept of page has to be clearer, since sometimes we confused page with slide.
e.g. now we have a carousel with 8 slides and {perPage: 2}, so it means we have 7 pages (dots) with {scrollPerPage: false} and 4 pages (dots) with {scrollPerPage: true}?

[quinnlangille]

If we have 9 slides and perPage: 3 then there should be 3 dots (3 slides per page, one dot per page) in the pagination. If scrollPerPage is false, we should have 9 dots, one dot per slide. I made a quick wireframe here: https://wireframe.cc/wpBcFF

Free free to annotate if you have any questions!

[RyanLiu0235]

Well, then we CAN'T highlight perPage dots at the same time rather than one...
Hmmm... would you mind clarifying every situation?

1. 6 slides with perPage: 1
2. 6 slides with perPage: 2 and scrollPerPage: false
3. 6 slides with perPage: 2 and scrollPerPage: true
   If this is clarified, the rest would be easy.

[quinnlangille]

1. 6 pages, 6 dots
2. 6 pages, 6 dots (if scrollPerPage is false, pagination should ignore perPage)
3. 3 pages, 3 dots

[Stexxe]

I think this is solution to this issue https://github.com/SSENSE/vue-carousel/pull/221, but I am not sure.

[syneva-runyan]

Hi! Not sure if this is an outstanding issue or not, but happy to jump on it if so! My understanding of reading this is that there are scenarios like the following that should be addressed:

+Six Slides
:per-slide="2"
:scrollPerPage="false"

EXPECTED BEHAVIOR: 6 Dots are shown in the pagination
ACTUAL BEHAVIOR: 5 Dots are shown in the pagination.

Question: Empty slide spaces. Ex, When the 5th dot is highlighted, the 5th slide is shown at the left side of the carousel, and the 6th slide is shown on the right. When the 6th dot is highlighted, the 6th slide would move over to the left, but there is no 7th slide to show on the right side of the page. What do we show in the 2nd slide space on the last page?

[quinnlangille]

Hey @syneva-runyan, thanks for offering to take this one. I think the root cause of this whole issue is a desperate need for refactoring in our slide calculation logic. We're currently working on it in office, but if you'd like to take a crack at improving the logic you're more than welcome!

In regards to your question, I think the best solution is to show an empty place holder slide. i.e - if we have 5 slides and 2 slides per page, the 3rd pagination dot should show the 5th slide and an empty space equal to the width of one slide. Does that seem correct to you?

[syneva-runyan]

Seems right to me, I'll take a look!

[quinnlangille]

This has been released in 0.18.0 :octocat: Thanks @syneva-runyan for the contribution!