Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
| * Callback fired when autoplay stops. | ||
| */ | ||
| onStop?: () => void; | ||
| }; |
There was a problem hiding this comment.
Curious what y'all think about having an api for this. It felt like a hook was best given all the unique logic just for autoplay, and storing it in common helps reuse, similar to useStepper. But I'm open to ideas.
| throw new Error('useCarouselAutoplayContext must be used within a Carousel component'); | ||
| } | ||
| return context; | ||
| }; |
There was a problem hiding this comment.
My logic for keeping this separate vs CarouselContext is
A) CarouselContext wasn't meant to be used by consumers, only for CarouselItem
B) Autoplay could be triggering updates a lot
| * When a function is provided, it receives the page index and returns a label. | ||
| * @default (pageIndex) => `Go to page ${pageIndex + 1}` | ||
| */ | ||
| goToPageAccessibilityLabel?: string; |
There was a problem hiding this comment.
goToPageAccessibilityLabel was a bug on mobile, we already supported paginationAccessibilityLabel - which is what we meant to use and is how it works on web. I checked and found no uses of this, but am fine if others want me to mark it as deprecated.
| background={isActive ? 'bgPrimary' : 'bgLine'} | ||
| borderColor="transparent" | ||
| data-active={isActive} | ||
| tabIndex={isActive ? undefined : -1} |
There was a problem hiding this comment.
We are dropping tabs accessibility experience here since it wasn't properly/fully implemented. Confirmed with design that doing buttons is also more likely what users will expect here.
| // Handle focus moving to an element inside the carousel items container. | ||
| // Pauses autoplay when focus enters from outside (keyboard navigation a11y). | ||
| // Scrolls to show focused items that are not fully visible. | ||
| const handleFocusIn = useCallback( |
There was a problem hiding this comment.
Greatly improved accessibility here, users will be able to navigate through carousel items by tabbing. Before, they could only tab to items within view, and needed to use navigation or pagination buttons to switch between pages.
This functionality ignores clones from looping as well.
There was a problem hiding this comment.
I am still syncing with Sam to see if we should change mobile.
3 participants
What changed? Why?
This PR
Root cause (required for bugfixes)
Minor bug fixes
UI changes
Mobile
With autoplay, we show the progress of the interval inside the new pagination variant
Web
With autoplay, we show the progress of the interval inside the new pagination variant
Testing
How has it been tested?
Testing instructions
Test out doc site examples and storybook for web and mobile.
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false