Feat/arian carousel cherry pick - #55
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
bda0abd to
2e2ba01
Compare
There was a problem hiding this comment.
@AntoniaStr i polished this a bit and addred a code snippet and descriptions
554c6a0 to
83c1989
Compare
Co-Authored-By: Cursor <cursoragent@cursor.com>
c9406a2 to
1dd92f6
Compare
![functn-monitoring[bot]](https://avatars.githubusercontent.com/in/3738576?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Code Agent review
The PR adds a new project carousel component, a showcase page, a snippet test wrapper, and a nav entry. The implementation is structurally sound with good attention to Embla-based carousel primitives, animation handling, and responsive layout. Three findings are flagged: an accessibility concern with the title markup, a React key anti-pattern in the crossfade stack, and a type duplication that could confuse downstream consumers.
Filtered 1 docs file(s) from review. LLM excluded 1 likely-irrelevant file(s): src/assets/index.ts.
Found 3 item(s) — 3 as inline comment(s) below.
Generated by code-agent (OpenCode + DeepSeek) · view review logs
| isBlack ? 'text-white' : '', | ||
| )} | ||
| > | ||
| {collectionLabel} |
There was a problem hiding this comment.
🟠 MEDIUM — Carousel title rendered as
instead of a heading element
The carousel's collection label and title are wrapped in a <p> tag (lines 329–342). Screen readers navigating by heading will skip this content. Since SlideContentBlock already uses <h2> for per-slide headings, the outer title should also be a heading (e.g. <h2>) to establish a meaningful heading hierarchy. Consider changing the outer container to <h2> (or the appropriate level for the page context) while keeping the same visual styling.
| > | ||
| {items.map((item, i) => ( | ||
| <MotionTag | ||
| key={i} |
There was a problem hiding this comment.
🟡 LOW · 🧹 quality — CrossfadeStack uses array index as React key
In the CrossfadeStack component (line 118) items are keyed by array index (key={i}). While this works for the current usage (a static carousel that never reorders/filters slides), it is a React anti-pattern that can cause stale animation state or incorrect reconciliation if the list ever changes dynamically. The CarouselTrack in the same file correctly uses key={index} for the image track, so the same pattern is inconsistent here. Consider using a stable unique identifier from the slide data (e.g. slide.altText or a synthetic id field) if available.
| heading?: string; | ||
| year?: string; | ||
| description?: string; | ||
| image: StaticImageData & { alt?: string }; |
There was a problem hiding this comment.
🟡 LOW · 🧹 quality — ProjectCarouselSlide type duplicates existing ProjectCarouselSlideData with a narrower image type
The new ProjectCarouselSlide type (line 29) defines image: StaticImageData & { alt?: string }, while src/assets/index.ts already exports ProjectCarouselSlideData with image: ImageMedia (which is { type: 'image' } & StaticImageData & { alt?: string }). The showcase.tsx passes placeholderProjectList (typed as ProjectCarouselSlideData[]) to ProjectCarousel, which works structurally but relies on type: 'image' being silently ignored by next/image. Downstream consumers looking at the component's exported type will expect a static-import shape, whereas the canonical data in @/assets has an extra discriminator field. Consider either importing and reusing ProjectCarouselSlideData from @/assets (if the component is tied to that data shape) or renaming the component's type to differentiate it and documenting the expected image shape.
2 participants
No description provided.