Tabs (future): add carousel functionality when tabs overflow container #5355
Conversation
🦋 Changeset detectedLatest commit: ea78314 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 |
628e223 to
2bef8d3
Compare
|
(putting in review for chromatic) |
a45fed0 to
3af91bd
Compare
packages/components/src/__future__/Tabs/subcomponents/TabList/TabList.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/__future__/Tabs/subcomponents/TabList/TabList.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/__future__/Tabs/_docs/Tabs.spec.stories.tsx
Outdated
Show resolved
Hide resolved
| const rightArrow = await canvas.findByTestId('kz-tablist-right-arrow') | ||
|
|
||
| await userEvent.click(rightArrow) | ||
| await new Promise((r) => setTimeout(r, 500)) |
There was a problem hiding this comment.
await waitFor the left arrow should hopefully allow you to not need a timeout :)
There was a problem hiding this comment.
For the first one yes, but then I'll still need the timeout when there's no arrow appearance/disappearance to hook into. So is it worth doing it, or just keep it consistent with the timeout
There was a problem hiding this comment.
Checking that the element is not in the dom when you expect it to disappear probably should work?
I'd just try to avoid timeouts when you have an explicit result you're waiting for, because it's still potentially flaky. It only makes sense to use it when the thing you're testing has functionality that relies on timeouts. It's okay as a last resort if nothing else works though.
There was a problem hiding this comment.
The timeout is actually more robust than that. Your idea works okay when it's testing for disappearance, but hooking into appearance actually breaks this.
The problem with testing for appearance/disappearance is that that doesn't mean the animation is done. The sequence goes:
- Press right -> animation starts -> left arrow appears -> animation finishes.
Even with disappearance, it's closer, but it still doesn't mean the animation is done because of the threshold of 75% (it disappears before the element is completely out of view).
So testing for left arrow appearing ends up trying to press right again before the animation is done and it breaks.
There's another issue as well as mentioned in my last comment: if the example had more tabs in it, or if we reduce the scroll amount, there's not always an arrow appearing or disappearing on each press (only the first and last presses). There would be times where both arrows are showing before the press, and continuing to show after press.
The main thing I don't love at the moment is the magical number on the timeout.
There was a problem hiding this comment.
If we want to go deep on this, we could see if there's a way to hook into/run a callback when the scrolling is finished.
e.g.
https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollend_event
Not supported on Safari tho :(
packages/components/src/__future__/Tabs/subcomponents/TabList/TabList.module.css
Outdated
Show resolved
Hide resolved
3af91bd to
ea78314
Compare
| @@ -0,0 +1 @@ | |||
| export const SCROLL_AMOUNT = 120 | |||
There was a problem hiding this comment.
Nit: Haha, probably didn't need to make a separate file for this since we only use it in the one place. But no matter.
| import { TabList as RACTabList, TabListProps as RACTabListProps } from 'react-aria-components' | ||
| import { | ||
| TabList as RACTabList, | ||
| TabListProps as RACTabListProps, |
There was a problem hiding this comment.
| TabListProps as RACTabListProps, | |
| type TabListProps as RACTabListProps, |
What
Add carousel to Future Tabs when tabs start overflowing the width of the viewport
Screen.Recording.2024-12-09.at.5.15.33.PM.mov
Why
Nicer than having heaps of tabs wrap to a new line