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

fix: handle indices properly when cellAlign is center or right #952

Merged
merged 23 commits into from Aug 10, 2022

Conversation

fritz-c
Copy link
Contributor

@fritz-c fritz-c commented Aug 5, 2022

Description

There are a variety of intertwined bugs related to slide index bounds (what the dots correspond to, also what determines whether prev/next buttons should be disabled) caused when cellAlign and scrollMode are thrown into the mix.

  1. With autoplayReverse set to true and cellAlign set to something other than left, the component starts with an odd offset: issue repro. Note how the rightmost slide, which we should presumably start from, is partially concealed, and we cannot click the button to move over to it.

  2. Autoplay does not go to the last slide with cellAlign of center: issue repro

  3. Scrolling to the right, we cannot reach slide 4: issue repro

This PR fixes the above, as well as performs a bit of generic refactoring.

NOTE: the fixes change some scrollMode="remainder"-like behaviors that were baked-in before. For example, this demo disables the "NEXT" button once the sixth slide is in view (e.g., slideIndex = 4).
With the fixes in this PR, the same scenario (demo from PR preview) would see the "NEXT" button enabled at that point, so the user could click again, resulting in slide 6 on the left side and whitespace on the right.
This could be seen as a breaking change. I tend to see it as making the component more closely follow the behavior that the component props defines. Setting scrollMode="remainder" will produce the same result that was baked-in before, if that is what is desired.
This change reduces the assumptions we are making about the library user's use case. For example, without the fix, the current-slide class will never be assigned to the final slide in the scenario above, whereas it can be assigned with the fix in place.
This change was omitted, as the flexibility with current-slide I pitched here didn't really make sense when you were jumping two slides at a time (=skipping every other slide) anyway. Also, it seemed like a bit of a trap that you could have three slides shown per page with a collection of 9 slides, and still end up with 4 pages.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

A variety of unit tests appearing in the PR, and manually

@fritz-c fritz-c requested a review from gksander August 5, 2022 20:59
@vercel
Copy link

vercel bot commented Aug 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nuka-carousel ✅ Ready (Inspect) Visit Preview Aug 9, 2022 at 5:02PM (UTC)

cy.get('.slide.slide-visible')
.should('have.length', 1)
.find('img')
.should('have.attr', 'data-slide', 'Slide 6');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This modification corresponds to the change in behavior in the scenario laid out in the PR description.

@@ -22,16 +26,13 @@ context('Infinity Carousel', () => {
cy.get('button').contains('Prev').should('not.be.disabled');
cy.get('button').contains('Next').should('not.be.disabled').click();

cy.wait(500);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cypress will wait for animations to finish, so the cy.waits are unnecessary:
https://docs.cypress.io/guides/core-concepts/interacting-with-elements#Animations

@fritz-c
Copy link
Contributor Author

fritz-c commented Aug 9, 2022

Regarding the changes in starting index for right-aligned stories:
When working with scrollMode="page"+cellAlign="left", this library prevents scrolling once all slides are reachable. So a carousel with 9 slides and slidesToShow={3} will only have three navigation dots, one for each "page", corresponding to indices [0, 3, 6].
With the identical case, but flipped around via cellAlign="right", we need to start from index 2 to similarly minimize unnecessary pages, for a result of [2, 5, 8]. Essentially, the algorithm to generate such an array works from the tail end of the slides, at slideCount - 1, moving towards the front until all slides have been reached.

This has the effect of creating some potentially puzzling offsets if you start from the leftmost end of the carousel, as is default.
e.g., whitespace gap between the start of the slideshow and the leftmost side

But when using the carousel from right-to-left, whether it be with autoplayReverse on or by setting slideIndex to the last slide initially, this initial index makes perfect sense.

Copy link
Contributor

@gksander gksander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!!

@fritz-c fritz-c merged commit 60036ad into main Aug 10, 2022
@fritz-c fritz-c deleted the fix/handle-index-with-cellalign-center-right branch August 10, 2022 13:23
@github-actions github-actions bot mentioned this pull request Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants