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
pettinarip
left a comment
pettinarip
left a comment
There was a problem hiding this comment.
@nloureiro thanks for taking this on! is a real win for the codebase :)
A couple of things I noticed while testing the preview:
- RTL is broken. On
/ar/and/ur/the dots and arrows navigate in the wrong direction — clicking "next" jumps backwards, the Previous button is enabled at the visual start, etc. This affects all 8 migrated carousels. - Dots don't reach the last position when the end is already visible. Screenshot below — the Next arrow is disabled (correctly, last card is in view) but the active dot is stuck at position 3 of 6 instead of advancing to 6.
Beyond those specific bugs, the bigger question I have is architectural: why a full rewrite instead of building on top of the shadcn Carousel we already have?
The embla-carousel.tsx this PR creates is literally shadcn's Carousel. Embla has RTL solved natively, handles the scroll-handler perf concerns for free, and ships battle-tested keyboard/touch/focus behavior.
I think we'd get further by renaming embla-carousel.tsx back to carousel.tsx as the canonical primitive and layering our edge-to-edge bleed + auto-hiding nav rather than maintaining a parallel scroll-snap implementation alongside it.
Just my opinion but I find this behavior fairly disruptive on desktop. Attempting to select text produces click-and-drag behavior which cannot be bypassed. Currently this also produces a "click" on mouse-up when click-dragging over a link (slide to navigate, but then the link is simultaneously followed). Is the desire here for when the user doesn't have easy access to horizontal scrolling (e.g., mouse wheel with only on direction)? 
This hover effect is brand new to the repo as well, not used anywhere else.. Feels a bit off and adds quite a lot of space between the arrow and dots when not hovering |
Not sure if I follow all details of your comment but I will try to look into it |
I will review the behavior. Thank you for raising it. The hover shadow is the same as we used on buttons, no? |
4 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