[feat/carousel] - 캐러셀 컴포넌트 구현 #22
Conversation
Summary of ChangesHello @ramong26, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 새로운 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements a new Carousel component with auto-play functionality and interactive slide features. The component allows users to display a series of images with titles and text, supporting click handlers for each slide and customizable image styling.
- Adds a complete Carousel component with autoplay, customizable intervals, and click handling for individual slides
- Refactors the Select component to move background styling from inline styles to CSS classes for better maintainability
- Includes comprehensive test coverage and Storybook documentation for the new Carousel component
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/Carousel/Carousel.tsx | Main carousel component implementation with autoplay and click functionality |
| src/components/Carousel/Carousel.test.tsx | Unit tests covering carousel interactions and autoplay behavior |
| src/components/Carousel/Carousel.stories.tsx | Storybook stories demonstrating carousel usage |
| src/components/Carousel/Carousel.scss | Comprehensive styling with responsive design and paper texture theme |
| src/components/Select/Select.tsx | Removes inline background image styles in favor of CSS classes |
| src/components/Select/Select.scss | Moves background styling to CSS and removes width constraint |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/components/Carousel/Carousel.tsx
Outdated
| setCurrentIndex((currentIndex + 1) % items.length); | ||
| }, interval); | ||
| return () => clearInterval(timer); | ||
| }, [autoPlay, interval, currentIndex, items.length]); |
There was a problem hiding this comment.
The useEffect dependency array includes currentIndex, causing the timer to reset on every slide change. This creates a new timer each time instead of maintaining a consistent interval. Remove currentIndex from the dependency array and use a functional update instead: setCurrentIndex(prev => (prev + 1) % items.length).
| setCurrentIndex((currentIndex + 1) % items.length); | |
| }, interval); | |
| return () => clearInterval(timer); | |
| }, [autoPlay, interval, currentIndex, items.length]); | |
| setCurrentIndex(prev => (prev + 1) % items.length); | |
| }, interval); | |
| return () => clearInterval(timer); | |
| }, [autoPlay, interval, items.length]); |
| items, | ||
| autoPlay = false, | ||
| interval = 3000, | ||
| ...rest |
There was a problem hiding this comment.
The ...rest props are spread on the container div but the component doesn't extend any HTML element interface. This could lead to type errors or unexpected behavior. Consider either extending React.HTMLAttributes<HTMLDivElement> in CarouselProps or removing the rest spread if not needed.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
캐러셀 컴포넌트 구현을 잘 마치셨네요! 자동 재생, 커스텀 이미지 스타일, 슬라이드별 onClick 핸들러 등 요구사항이 잘 반영되었습니다. 코드 구조도 BEM을 사용한 SCSS와 함께 깔끔하게 작성되었습니다. 다만, 코드의 안정성과 접근성, 테스트 완성도를 높이기 위해 몇 가지 개선점을 제안합니다. useEffect 내 setInterval 사용 방식, 접근성 관련 속성 추가, 그리고 테스트 코드의 논리 오류에 대한 피드백을 포함했습니다. 또한 SCSS에 정의된 이전/다음 버튼을 실제 컴포넌트에 구현하면 더 완성도 높은 컴포넌트가 될 것입니다.
src/components/Carousel/Carousel.tsx
Outdated
| useEffect(() => { | ||
| if (!autoPlay) return; | ||
| const timer = setInterval(() => { | ||
| setCurrentIndex((currentIndex + 1) % items.length); | ||
| }, interval); | ||
| return () => clearInterval(timer); | ||
| }, [autoPlay, interval, currentIndex, items.length]); |
There was a problem hiding this comment.
자동 슬라이드 기능을 구현한 useEffect의 의존성 배열에 currentIndex가 포함되어 있습니다. 이렇게 하면 슬라이드가 변경될 때마다 setInterval이 해제되고 다시 생성됩니다. 이는 불필요한 재실행을 유발하고 잠재적으로 타이머의 정확성에 영향을 줄 수 있습니다.
useState의 함수형 업데이트를 사용하면 currentIndex에 대한 의존성을 제거할 수 있습니다. 이렇게 수정하면 useEffect는 컴포넌트가 마운트될 때와 autoPlay, interval, items.length가 변경될 때만 실행되어 더 효율적입니다.
| useEffect(() => { | |
| if (!autoPlay) return; | |
| const timer = setInterval(() => { | |
| setCurrentIndex((currentIndex + 1) % items.length); | |
| }, interval); | |
| return () => clearInterval(timer); | |
| }, [autoPlay, interval, currentIndex, items.length]); | |
| useEffect(() => { | |
| if (!autoPlay) return; | |
| const timer = setInterval(() => { | |
| setCurrentIndex((prevIndex) => (prevIndex + 1) % items.length); | |
| }, interval); | |
| return () => clearInterval(timer); | |
| }, [autoPlay, interval, items.length]); |
| expect(screen.getByRole('heading', { name: /Slide 1/i })).toBeInTheDocument(); | ||
| vi.advanceTimersByTime(1000); | ||
| expect(screen.getByRole('heading', { name: /Slide 2/i })).toBeInTheDocument(); | ||
| vi.advanceTimersByTime(1000); | ||
| expect(screen.getByRole('heading', { name: /Slide 3/i })).toBeInTheDocument(); | ||
| vi.advanceTimersByTime(1000); | ||
| expect(screen.getByRole('heading', { name: /Slide 1/i })).toBeInTheDocument(); |
There was a problem hiding this comment.
자동 슬라이드 테스트에서 vi.advanceTimersByTime을 호출한 후 다음 슬라이드의 제목(heading)이 문서에 있는지 확인하고 있습니다. 하지만 캐러셀 구현상 모든 슬라이드는 초기에 DOM에 렌더링되므로, 이 테스트는 실제로 슬라이드가 전환되었는지를 검증하지 못합니다. expect(...).toBeInTheDocument()는 항상 통과하게 됩니다.
슬라이드 전환을 올바르게 테스트하려면, 현재 활성화된 인디케이터가 변경되었는지 확인하는 것이 더 확실한 방법입니다. 이를 위해서는 다른 리뷰 댓글에서 제안한 것처럼 인디케이터 버튼에 aria-label을 추가하여 테스트에서 각 버튼을 특정할 수 있도록 하는 것이 좋습니다.
아래와 같이 활성화된 인디케이터를 확인하도록 테스트를 수정하는 것을 권장합니다.
const indicators = screen.getAllByRole('button');
expect(indicators[0]).toHaveClass('active');
vi.advanceTimersByTime(1000);
expect(indicators[0]).not.toHaveClass('active');
expect(indicators[1]).toHaveClass('active');
vi.advanceTimersByTime(1000);
expect(indicators[1]).not.toHaveClass('active');
expect(indicators[2]).toHaveClass('active');
vi.advanceTimersByTime(1000);
expect(indicators[2]).not.toHaveClass('active');
expect(indicators[0]).toHaveClass('active');
| }, [autoPlay, interval, currentIndex, items.length]); | ||
|
|
||
| return ( | ||
| <div className="carousel"> |
There was a problem hiding this comment.
Carousel.scss 파일에는 이전/다음 버튼(.carousel__button--prev, .carousel__button--next)에 대한 스타일이 정의되어 있지만, 실제 Carousel.tsx 컴포넌트에는 이 버튼들이 구현되어 있지 않습니다. 사용자가 인디케이터 외에도 직접 슬라이드를 넘길 수 있도록 이전/다음 버튼을 추가하면 사용성이 크게 향상될 것입니다.
아래와 같이 핸들러 함수와 버튼을 추가할 수 있습니다.
// 핸들러 함수 추가
const goToNext = () => {
setCurrentIndex((prevIndex) => (prevIndex + 1) % items.length);
};
const goToPrev = () => {
setCurrentIndex((prevIndex) =>
prevIndex === 0 ? items.length - 1 : prevIndex - 1
);
};
// JSX에 버튼 추가
return (
<div className="carousel">
<button onClick={goToPrev} className="carousel__button carousel__button--prev"><</button>
<button onClick={goToNext} className="carousel__button carousel__button--next">></button>
{/* ... aunchor ... */}
</div>
);
src/components/Carousel/Carousel.tsx
Outdated
| style={{ transform: `translateX(-${currentIndex * 100}%)` }} | ||
| > | ||
| {items.map((item, index) => ( | ||
| <li key={index} className="carousel__item" onClick={item.onClick}> |
There was a problem hiding this comment.
이 <li> 요소와 관련하여 두 가지 개선점을 제안합니다.
keyprop: 현재key로 배열의index를 사용하고 있습니다. 아이템 순서가 변경되거나 중간에 추가/삭제될 경우 예기치 않은 동작이 발생할 수 있습니다. 각 아이템에 고유id를 부여하고key로 사용하는 것이 좋습니다.- 접근성: 현재 모든 슬라이드가 DOM에 렌더링되어 스크린 리더가 보이지 않는 슬라이드까지 읽을 수 있습니다.
aria-hidden={index !== currentIndex}속성을 추가하여 현재 보이는 슬라이드만 스크린 리더가 읽도록 할 수 있습니다.
아래와 같이 aria-hidden을 추가하고, key는 추후 고유 ID로 변경하는 것을 고려해보세요.
| <li key={index} className="carousel__item" onClick={item.onClick}> | |
| <li key={index} className="carousel__item" onClick={item.onClick} aria-hidden={index !== currentIndex}> |
src/components/Carousel/Carousel.tsx
Outdated
| {item.image && ( | ||
| <img | ||
| src={item.image} | ||
| alt={item.title} |
There was a problem hiding this comment.
이미지의 alt 속성으로 item.title을 사용하고 있습니다. CarouselItem 인터페이스에서 title은 선택적이므로, title이 없는 경우 alt 속성이 누락되어 웹 접근성 가이드라인을 위반하게 됩니다.
이미지에 대한 설명이 없는 경우, alt 속성을 빈 문자열("")로 설정하여 해당 이미지가 장식용임을 명시하는 것이 좋습니다.
| alt={item.title} | |
| alt={item.title ?? ''} |
| <button | ||
| key={index} | ||
| className={`carousel__indicator ${ | ||
| index === currentIndex ? 'active' : '' | ||
| }`} | ||
| onClick={() => setCurrentIndex(index)} | ||
| /> |
There was a problem hiding this comment.
인디케이터 버튼에 텍스트나 aria-label이 없어 스크린 리더 사용자가 각 버튼의 역할을 이해하기 어렵습니다.
각 버튼에 aria-label을 추가하여 "1번 슬라이드로 이동"과 같이 명확한 설명을 제공하는 것이 접근성 향상에 도움이 됩니다.
<button
key={index}
className={`carousel__indicator ${
index === currentIndex ? 'active' : ''
}`}
onClick={() => setCurrentIndex(index)}
aria-label={`Go to slide ${index + 1}`}
/>
📝 PR 제목
캐러셀 컴포넌트 구현
🛠 변경 내용
🎯 목적 / 이유
✅ 확인 사항