fix (carousel): fix overlapping inner columns on infinite scroll#3588
fix (carousel): fix overlapping inner columns on infinite scroll#3588
Conversation
WalkthroughCompute per-slide translateX from offsetLeft for infinite-scroll clones; initialize clones with non-animated transform to avoid flicker; introduce conditional per-swap translation (useSlideTranslateX) and apply matching translations to slides and clones, with RTL-aware signs and special handling for the last clone. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Carousel
participant Slides
participant Clones
User->>Carousel: Initialize carousel
Carousel->>Slides: Measure offsetLeft (first slide, first clone)
Slides-->>Carousel: offsetLeft values
Carousel->>Carousel: Compute slideTranslateX & cloneTranslateX (RTL-aware)
Carousel->>Clones: Create/init clones with will-change, translateX(0), transition:0s
User->>Carousel: Trigger swap/scroll
Carousel->>Carousel: Set useSlideTranslateX = true if needed
alt useSlideTranslateX == true
Carousel->>Slides: Apply transform: translateX(slide.slideTranslateX)
Carousel->>Clones: Apply transform: translateX(clone.cloneTranslateX) (last clone uses special sign)
else
Carousel->>Slides: Apply transform: translateX(0)
Carousel->>Clones: Apply transform: translateX(0)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/block/carousel/frontend-carousel.js (2)
96-105: Avoid persistent will-change/0s transitions; clean up after init to prevent long-lived layersKeeping
will-change: transformandtransition: transform 0son slides/clones can pin them to separate compositor layers and hurt performance. Clear these after the initial, non-animated positioning.clone.classList.add( `stk-slide-clone-${ slideIndex + 1 }` ) clone.style.zIndex = -1 clone.style.willChange = 'transform' clone.style.transform = 'TranslateX( 0 )' original.style.willChange = 'transform' original.style.transform = 'TranslateX( 0 )' // prevents flickering when changing the value of TranslateX original.style.transition = 'transform 0s' clone.style.transition = 'transform 0s' +// Clean up layer hints/transitions after initial layout +requestAnimationFrame( () => { + original.style.transition = '' + clone.style.transition = '' + original.style.willChange = '' + clone.style.willChange = '' +} )Also consider using lowercase
translateXfor consistency with common style guides (CSS is case-insensitive, this is a nit).
339-342: Prefer lowercasetranslateXand guard against undefined slideTranslateXCosmetic but consistent: use
translateX. Also, whenuseSlideTranslateXis true, ensureslide.slideTranslateX/clone.slideTranslateXexist to avoid settingtranslateX(undefined)if indices drift.- slide.style.transform = `TranslateX(${ useSlideTranslateX ? slide.slideTranslateX : 0 })` + slide.style.transform = `translateX(${ useSlideTranslateX && slide.slideTranslateX ? slide.slideTranslateX : 0 })`- clone.style.transform = `TranslateX(${ useSlideTranslateX ? clone.slideTranslateX : 0 })` + clone.style.transform = `translateX(${ useSlideTranslateX && clone.slideTranslateX ? clone.slideTranslateX : 0 })`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/block/carousel/frontend-carousel.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/block/carousel/frontend-carousel.js (1)
src/higher-order/with-visual-guide/visual-guide.js (1)
computedStyles(22-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: build
🔇 Additional comments (1)
src/block/carousel/frontend-carousel.js (1)
319-325: Gate for using swap translations looks fine; ensure reverse path resets transforms cleanlySetting
useSlideTranslateX = trueonly when adding swaps (forward) is reasonable. On reverse, transforms go back to0. Please verify no stale transforms remain on slides/clones when oscillating near the boundary (rapid prev/next). I can script-check for any non-zero inline transforms after a swap sequence if helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/block/carousel/frontend-carousel.js (1)
7-10: Broken iOS detection (always truthy); fix logic.The current check returns a truthy value for most UAs (because -1 is truthy and the boolean logic is inverted), enabling iOS-specific handlers on non‑iOS devices.
Apply this diff:
-function isiOS() { - const userAgent = navigator?.userAgent - return userAgent && ( userAgent.indexOf( 'iPhone' ) === -1 || userAgent.indexOf( 'iPad' ) ) -} +function isiOS() { + const ua = navigator?.userAgent || '' + // Covers iPhone/iPad/iPod; also iPadOS on Safari (Mac UA + touch) + const isIOSUA = /iP(hone|od|ad)/i.test( ua ) + const isTouchMac = /Mac/i.test( ua ) && ( navigator?.maxTouchPoints > 1 ) + return isIOSUA || isTouchMac +}
♻️ Duplicate comments (1)
src/block/carousel/frontend-carousel.js (1)
124-132: Good switch to layout-based delta; also recompute on resize (extract helper).The offsetLeft-based delta fixes variable widths. Ensure it’s recalculated on resize; right now it runs only during first-time clone setup, so deltas get stale after a resize or font load.
Apply this diff here:
- const slideOffsetLeft = this.slideEls[ 0 ].offsetLeft - const cloneOffsetLeft = this.clones[ 0 ].offsetLeft - - const diff = Math.abs( slideOffsetLeft - cloneOffsetLeft ) - this.slideTranslateX = ( this.isRTL ? -diff : diff ) + 'px' - this.cloneTranslateX = ( this.isRTL ? diff : -diff ) + 'px' + this.computeTrackDelta()Then add this method in the class (outside this hunk):
computeTrackDelta = () => { if ( !this.infiniteScroll || !this.clones?.length ) return const slideOffsetLeft = this.slideEls[0].offsetLeft const cloneOffsetLeft = this.clones[0].offsetLeft const diff = Math.abs( slideOffsetLeft - cloneOffsetLeft ) this.slideTranslateX = ( this.isRTL ? -diff : diff ) + 'px' this.cloneTranslateX = ( this.isRTL ? diff : -diff ) + 'px' }And call it whenever initProperties runs after first init:
if ( this.infiniteScroll && this.clones?.length ) { this.computeTrackDelta() }
🧹 Nitpick comments (4)
src/block/carousel/frontend-carousel.js (4)
97-105: Use lowercase translateX and avoid long-lived will-change/transition.
- translateX is case-insensitive but keep it lowercase for consistency.
- Clear willChange and inline transition after init; keeping them set increases memory/painting costs.
Apply this diff:
- clone.style.willChange = 'transform' - clone.style.transform = 'TranslateX( 0 )' + clone.style.willChange = 'transform' + clone.style.transform = 'translateX(0)' original.style.willChange = 'transform' - original.style.transform = 'TranslateX( 0 )' + original.style.transform = 'translateX(0)' // prevents flickering when changing the value of TranslateX original.style.transition = 'transform 0s' - clone.style.transition = 'transform 0s' + clone.style.transition = 'transform 0s'Outside this hunk, after init completes (e.g., right after step 5), clear hints:
requestAnimationFrame( () => { this.clones.forEach( c => { c.style.willChange = ''; c.style.transition = '' } ) this.slideEls.forEach( s => { s.style.willChange = ''; s.style.transition = '' } ) } )
327-331: Optional: use translate3d for smoother swaps.GPU-promote the move to reduce subpixel jank on mobile.
- slide.style.transform = `translateX(${ useSlideTranslateX ? this.slideTranslateX : 0 })` + slide.style.transform = `translate3d(${ useSlideTranslateX ? this.slideTranslateX : '0px' },0,0)` ... - clone.style.transform = `translateX(${ useSlideTranslateX ? this.cloneTranslateX : 0 })` + clone.style.transform = `translate3d(${ useSlideTranslateX ? this.cloneTranslateX : '0px' },0,0)`
307-313: Minor: rename for intent clarity.useSlideTranslateX could be shouldTranslate or willTranslate for readability; logic is fine.
220-224: Optional: throttle resize handler.initProperties on every resize event can thrash. Consider a rAF or 100–200ms debounce before calling initProperties/computeTrackDelta.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/block/carousel/frontend-carousel.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 7.3 and WP 6.5.5
3 participants
Summary by CodeRabbit