-
Notifications
You must be signed in to change notification settings - Fork 597
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 for leftover width less than half of container/screen size #1064
base: main
Are you sure you want to change the base?
Fix for leftover width less than half of container/screen size #1064
Conversation
🦋 Changeset detectedLatest commit: 1b88357 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -121,7 +121,7 @@ describe('useMeasurement', () => { | |||
const { totalPages, scrollOffset } = result.current; | |||
|
|||
expect(totalPages).toBe(2); | |||
expect(scrollOffset).toEqual([0, 500]); | |||
expect(scrollOffset).toEqual([0, 400]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now just the remainder scroll (so the exact amount that needs to be scrolled to fully have the next page in view)
expect(totalPages).toBe(3); | ||
expect(scrollOffset).toEqual([0, 573, 1146]); | ||
expect(totalPages).toBe(4); | ||
// 573 * 0, 573 * 1, 573 * 2, 573 * 3 + (1720 - 573 * 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is useful, but I thought a code comment explaining where the values are coming from for the toEqual
might be helpful?
Happy to edit or delete if it adds more confusion for now
// this test covers that even when the leftover width is less than | ||
// half of the screen width, it should still be scrollable so that user can see | ||
// the small overflow | ||
scrollWidth: 600, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New test that verifies the fix works correctly
Description
During implementation of the carousel, we noticed sometimes a weird behavior where the amount that can be scrolled left does not go all the way to the end of the content.
See example from this before recording
before.mov
You can notice there the scroll stops at the Slide 8, but technically there are 10 slides (so the last slide is numbered 9)
This is due to the number of pages being calculated as a rounded value, hence if the leftover width is less than half of the container size, the number of pages will be rounded down to the lower value and it'll be impossible to scroll to the end with the buttons/scroll indicators.
I propose the following fix:
Here is the after result from the same story at the same screen size:
after.mov
Type of Change
How Has This Been Tested?
Checklist
pnpm run lint
)pnpm run test:ci-with-server
/pnpm run test
)pnpm changeset
.