Conversation
Enhance EdgeScrollContainer with built-in prev/next arrow buttons and per-item pagination dots. Navigation auto-hides when content fits without scrolling. Migrate all simple card carousels from Swiper.js to the unified EdgeScrollContainer component. Fixes #17945
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Rename the component and file to a standard, recognizable name: - EdgeScrollContainer → Carousel - EdgeScrollItem → CarouselItem - edge-scroll-container.tsx → carousel.tsx - Embla carousel → embla-carousel.tsx (used only by ReleaseCarousel)
|
@nloureiro id suggest for all cards to also have ability to drag on laptop, even with a mouse |
|
@claude review |
|
Claude finished @nloureiro's task in 3m 30s —— View job Code Review: Add arrow navigation to all carouselsTasks:
Overall this is a well-structured PR that cleanly unifies carousel behaviour. The migration pattern is consistent across all affected pages, and the auto-hide logic when content fits is a nice touch. A few issues worth addressing before merge are below. Bugs1. Event listener leak in The api.on("reInit", onSelect) // added
api.on("select", onSelect) // added
return () => {
api?.off("select", onSelect) // only select is removed — reInit leaks
}
return () => {
api?.off("reInit", onSelect)
api?.off("select", onSelect)
}2. RTL active-dot detection is broken ( Active index is found by measuring distance from the container's left edge: const containerLeft = el.getBoundingClientRect().left
items.forEach((item, i) => {
const dist = Math.abs(item.getBoundingClientRect().left - containerLeft)
...
})In RTL, the start edge is on the right side, so the wrong dot will be highlighted when scrolling. This doesn't affect arrow flipping (handled correctly via Fix: use const isRtl = getComputedStyle(el).direction === "rtl"
const containerEdge = isRtl
? el.getBoundingClientRect().right
: el.getBoundingClientRect().left
items.forEach((item, i) => {
const itemEdge = isRtl
? item.getBoundingClientRect().right
: item.getBoundingClientRect().left
const dist = Math.abs(itemEdge - containerEdge)
...
})3. Dead
const outerRef = React.useRef<HTMLDivElement>(null) // created but never used
...
ref={(node) => {
;(outerRef as React.MutableRefObject<HTMLDivElement | null>).current = node
if (typeof ref === "function") ref(node)
else if (ref) ref.current = node
}}Can be simplified — just forward the ref directly and remove <div ref={ref} className={cn("space-y-6", className)}>Accessibility4. Missing keyboard navigation (
Suggest adding: <div
className={cn("space-y-6", className)}
onKeyDown={(e) => {
if (e.key === "ArrowLeft") { e.preventDefault(); scrollByPage("start") }
if (e.key === "ArrowRight") { e.preventDefault(); scrollByPage("end") }
}}
tabIndex={0}
>5. Missing
role="region"
aria-roledescription="carousel"
6.
// Current:
aria-current={i === activeIndex ? "true" : undefined}
// Better:
aria-pressed={i === activeIndex}7. Non-specific
<nav ?? "Carousel navigation"}>Architecture8. Naming conflict between Both files export
Minor9.
10.
11. Moving Summary
The core logic is solid. The most important items to address before merge are the event listener leak (#1), keyboard navigation (#4), and missing ARIA role (#5). |
Bug fixes: - embla-carousel: clean up reInit event listener on unmount - carousel: fix active-dot detection in RTL (use inline-start edge) - carousel: remove dead outerRef and simplify ref forwarding Accessibility: - carousel: add role=region + aria-roledescription=carousel - carousel: add keyboard navigation (ArrowLeft/Right) with tabIndex - carousel: replace aria-current with aria-pressed on pagination dots - carousel: accept aria-label prop and forward to nav for disambiguation Minor: - InnovationSwiper/VideoCourseSwiper: use stable title as React key - InnovationSwiper: w-[100%] → w-full
nloureiro
requested review from
minimalsm,
pettinarip and
wackerow
as code owners
2 participants
Summary
EdgeScrollContainerwith built-in prev/next arrow buttons and per-item pagination dotsEdgeScrollContainerFixes #17945
Design
ChevronPrev/ChevronNextcomponentsPages affected
Carousels NOT migrated (specialized Swiper features)
BentoCardSwiper— card stacking effectSavingsCarousel— slide change animation callbacksStartWithEthereumFlow— programmatic slide controlBuilderSwiper— fractional slidesPerViewTorchHistorySwiper— coverflow 3D effectScreenshotSwiper— dual carousel with programmatic controlTopApps/WhySwiper— analytics slide change callbacksReleaseCarousel— dual synced Embla carouselsStorybook
See Molecules > Navigation > EdgeScrollContainer for all stories (Basic, Few Items, Without Snap, As Child Example).
Test plan