fix: add direction tracking to carousel component #1574
Conversation
|
@maj0rika is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
|
""" WalkthroughThe Carousel component was enhanced to track slide navigation direction explicitly by setting Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CarouselComponent
participant IndicatorsComponent
participant ThumbnailsComponent
participant StateStore
User->>IndicatorsComponent: Click indicator button (idx)
IndicatorsComponent->>StateStore: Update state { index: idx, forward: idx > currentIndex, lastSlideChange }
StateStore->>CarouselComponent: State updated (index, forward)
CarouselComponent->>CarouselComponent: Animate slide based on forward
User->>ThumbnailsComponent: Click thumbnail (idx)
ThumbnailsComponent->>StateStore: Update state { index: idx, forward: idx > currentIndex (with wrap), lastSlideChange }
StateStore->>CarouselComponent: State updated (index, forward)
CarouselComponent->>CarouselComponent: Animate slide based on forward
Note over CarouselComponent: Auto slide loop interval
CarouselComponent->>CarouselComponent: If forward call nextSlide else prevSlide
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
maj0rika
force-pushed
the
fix--add-direction-tracking-to-carousel-transitions
branch
from
07972c3 to
7942bd1
Compare
maj0rika
changed the title
…tracking-to-carousel-transitions
|
It doesn't work. The direction of slide when clicking an indicator behind still move forwards. |
|
Hi! |
|
It seems Control buttons works, but Indicators and Thumbnails don't work. |
![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 (3)
src/lib/carousel/Thumbnails.svelte (3)
15-20: Redundant$effectre-assignsimagesand can be safely removedThe reactive statement only runs once (when
imagesis initiallyundefined), after whichimagesis frozen and no longer tracks$state.images.
Because the#eachblock already falls back to$state.images(images || $state.images), the extra assignment adds complexity without benefit and creates the risk of the two sources diverging.-$effect(() => { - if (!images) { - images = $state.images; - } -});Removing it keeps a single source of truth and avoids accidental stale data.
34-39: Avoid mutating_stateinside the storeupdatecallback
Writable.updateexpects you to return the new value; mutating the draft object before spreading can lead to subtle bugs if other references to the same object exist.-state.update((_state) => { - _state.forward = isForward; - _state.index = idx; - _state.lastSlideChange = new Date(); - return { ..._state }; -}); +state.update((_state) => ({ + ..._state, + forward: isForward, + index: idx, + lastSlideChange: new Date() +}));This keeps state updates immutable and predictable.
46-55: Add a keyed#eachfor stable DOM and smoother transitionsWithout a key Svelte re-uses elements by index, which can cause focus/animation glitches when the images array changes order or length.
{#each images || $state.images as image, idx (image.id ?? idx)} <!-- … --> {/each}Using a stable key (e.g.,
image.idor the index as a fallback) improves DOM diffing and prevents accidental re-mounts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/lib/carousel/Carousel.svelte(5 hunks)src/lib/carousel/Indicators.svelte(1 hunks)src/lib/carousel/Thumbnails.svelte(2 hunks)src/lib/types.ts(2 hunks)src/routes/component-data/Carousel.json(1 hunks)src/routes/docs/components/carousel.md(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/carousel/Carousel.svelte
🔇 Additional comments (7)
src/routes/component-data/Carousel.json (1)
9-9: New optionalthumbnailprop added for Carousel component.This addition of the
thumbnailproperty to the Carousel component's props list supports the new ability to render thumbnails as a child component with shared state.src/lib/carousel/Indicators.svelte (1)
18-27: Direction tracking implemented for indicator clicks.The indicator click handler now updates the state with direction information by determining if the clicked indicator represents moving forward or backward relative to the current index. This properly fixes the issue where direction wasn't being tracked when navigating via indicators.
The implementation correctly:
- Determines direction with
isForward = idx > $state.index- Updates the shared state with the forward flag and current timestamp
- Uses an immutable update pattern with
{ ..._state }src/lib/types.ts (2)
393-393: New type definition for thumbnail slot added to CarouselProps.The
thumbnailproperty has been correctly added to theCarouselPropsinterface as an optional Snippet without parameters, supporting the new design where thumbnails are rendered as a child component through a slot.
428-428: Theimagesproperty in ThumbnailsProps is now optional.Making the
imagesproperty optional allows the Thumbnails component to retrieve images from the shared state context when not explicitly provided, supporting the new context-based state sharing model.Note that the
indexproperty has been removed (not shown in diff) which is consistent with the component now using the shared state for tracking the current index.src/routes/docs/components/carousel.md (3)
124-124: Documentation updated to reflect new state-sharing architecture.The explanation now clearly indicates that Thumbnails should be used as a child of Carousel and shares the same state store, which aligns with the implementation changes.
138-140: Example code updated to use the new thumbnail slot.The example properly demonstrates how to use the new
thumbnailsnippet slot to render thumbnails inside the Carousel component, reflecting the architectural change to use context-based state sharing.
238-244: Advanced customization example updated for thumbnail integration.The example now correctly shows how to customize thumbnails using the new slot-based approach, including how to customize individual thumbnail styling based on selection state.
|
Hi!
Initially, I tried determining the direction based on the As a side benefit, this also resolved an existing issue where Let me know if anything still needs tweaking — happy to follow up! |
|
v1.0.6 fixed #1551 |
Closes #1551
📑 Description
Fixes an issue with carousel animations where the transition looked the same whether moving forward or backward.
This update introduces a
forwardstate to track navigation direction —truewhen moving to the next slide,falsefor the previous one.Now the carousel correctly animates in the appropriate direction based on user interaction.
Status
✅ Checks
ℹ Additional Information
This PR fixes the issue reported in #1551 where the carousel animation was the same when moving backward as when moving forward. By tracking the direction with a
forwardstate property (set totruefor next slide andfalsefor previous slide), we enable the carousel to use appropriate transition animations for each direction.Commit message:
fix: add direction tracking to carousel transitionsSummary by CodeRabbit