Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 170 additions & 0 deletions src/components/Carousel/Carousel.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
@use '../../styles/index' as s;
@use 'sass:map';

.carousel {
position: relative;
overflow: hidden;
border: 2px solid s.color(black);
background-image: url('../../assets/paper.png');
background-repeat: repeat;
background-size: auto;
font-family: 'Times New Roman', serif;

&__container {
width: 100%;
overflow: hidden;
}

&__list {
display: flex;
transition: transform 0.4s ease-in-out;
}

&__item {
@include s.flex-box(space-around, center, row);
cursor: pointer;
flex: 0 0 100%;
padding: 20px;
text-align: center;
background-size: cover;

&:last-child {
border-right: none;
}
}

&__image {
max-width: 100%;
height: auto;
background: s.color(white) url('../../assets/paper.png');
margin-bottom: 12px;
}

&__title {
@include s.text-style-extended('2xl', 800, red-900);
margin-bottom: 6px;
text-transform: uppercase;
}

&__text {
@include s.text-style-extended('lg', 400, gray-700);
}

&__button {
position: absolute;
top: 50%;
transform: translateY(-50%);
padding: 6px 12px;
@include s.text-style-extended('lg', 400, gray-700);
background: s.color(white) url('../../assets/paper.png');
background-size: cover;
border: 2px solid s.color(black);
cursor: pointer;
transition: all 0.2s ease-in-out;

&:hover {
background: s.color(yellow-200) url('../../assets/paper.png');
transform: translateY(-50%) scale(1.05);
}

&--prev {
left: 10px;
}
&--next {
right: 10px;
}
}

&__indicators {
@include s.flex-box(center, center, row);
gap: 8px;
margin: 12px;
}

&__indicator {
width: 16px;
height: 16px;
border: 2px solid s.color(black);
background: s.color(white) url('../../assets/paper.png');
background-size: cover;
cursor: pointer;
transition: all 0.2s ease-in-out;

&.active {
background: s.color(yellow-600) url('../../assets/paper.png');
transform: scale(1.1);
}
}

// sm 이상
@include s.mq(sm) {
&__title {
font-size: 20px;
}
&__text {
font-size: 14px;
}
&__button {
padding: 4px 10px;
font-size: 14px;
}
&__indicator {
width: 12px;
height: 12px;
}
}

// md 이상
@include s.mq(md) {
&__title {
font-size: 22px;
}
&__text {
font-size: 15px;
}
&__button {
padding: 6px 12px;
font-size: 15px;
}
&__indicator {
width: 14px;
height: 14px;
}
}

// lg 이상
@include s.mq(lg) {
&__title {
font-size: 24px;
}
&__text {
font-size: 16px;
}
&__button {
padding: 8px 14px;
font-size: 16px;
}
&__indicator {
width: 16px;
height: 16px;
}
}

// xl 이상
@include s.mq(xl) {
&__title {
font-size: 28px;
}
&__text {
font-size: 18px;
}
&__button {
padding: 10px 16px;
font-size: 18px;
}
&__indicator {
width: 18px;
height: 18px;
}
}
}
41 changes: 41 additions & 0 deletions src/components/Carousel/Carousel.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import type { Meta, StoryObj } from '@storybook/react-vite';
import Carousel from './Carousel';

const meta: Meta<typeof Carousel> = {
title: 'Components/Carousel',
component: Carousel,
tags: ['autodocs']
};

export default meta;

type Story = StoryObj<typeof Carousel>;

export const Default: Story = {
args: {
items: [
{
image: 'https://wikidocs.net/images/page/279778/boardwalk.jpg',
title: 'Slide 1',
text: 'This is the first slide.',
onClick: () => alert('Slide 1 clicked!'),
imageStyle: { width: '500px', height: 'auto' }
},
{
image: 'https://wikidocs.net/images/page/279778/boardwalk.jpg',
title: 'Slide 2',
text: 'This is the second slide.',
onClick: () => alert('Slide 2 clicked!'),
imageStyle: { width: '500px', height: 'auto' }
},
{
image: 'https://wikidocs.net/images/page/279778/boardwalk.jpg',
title: 'Slide 3',
text: 'This is the third slide.',
onClick: () => alert('Slide 3 clicked!'),
imageStyle: { width: '500px', height: 'auto' }
}
],
autoPlay: true
}
};
55 changes: 55 additions & 0 deletions src/components/Carousel/Carousel.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { test, expect, vi } from 'vitest';

import Carousel from './Carousel';

const items = [
{
image: 'https://wikidocs.net/images/page/279778/boardwalk.jpg',
title: 'Slide 1',
text: 'This is the first slide.',
onClick: vi.fn(),
imageStyle: { width: '500px', height: 'auto' }
},
{
image: 'https://wikidocs.net/images/page/279778/boardwalk.jpg',
title: 'Slide 2',
text: 'This is the second slide.',
onClick: vi.fn(),
imageStyle: { width: '500px', height: 'auto' }
},
{
image: 'https://wikidocs.net/images/page/279778/boardwalk.jpg',
title: 'Slide 3',
text: 'This is the third slide.',
onClick: vi.fn(),
imageStyle: { width: '500px', height: 'auto' }
}
];

test('renders Carousel and 클릭', async () => {
render(<Carousel items={items} autoPlay={false} />);
const slide1 = screen.getByRole('heading', { name: /Slide 1/i });
const slide2 = screen.getByRole('heading', { name: /Slide 2/i });
const slide3 = screen.getByRole('heading', { name: /Slide 3/i });
await userEvent.click(slide1);
expect(items[0].onClick).toHaveBeenCalled();
await userEvent.click(slide2);
expect(items[1].onClick).toHaveBeenCalled();
await userEvent.click(slide3);
expect(items[2].onClick).toHaveBeenCalled();
});

test('자동 슬라이드 동작 (타이머 기반)', () => {
vi.useFakeTimers();
render(<Carousel items={items} autoPlay={true} interval={1000} />);
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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

자동 슬라이드 테스트에서 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');

vi.useRealTimers();
});
77 changes: 77 additions & 0 deletions src/components/Carousel/Carousel.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import React, { useState, useEffect } from 'react';
import './Carousel.scss';

export interface CarouselItem {
title?: string;
text?: string;
image?: string;
imageStyle?: React.CSSProperties;
onClick?: () => void;
}
export interface CarouselProps {
items: CarouselItem[];
autoPlay?: boolean;
interval?: number;
}

const Carousel: React.FC<CarouselProps> = ({
items,
autoPlay = false,
interval = 3000,
...rest
Copy link

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}) => {
const [currentIndex, setCurrentIndex] = useState(0);

// 자동 슬라이드 기능
useEffect(() => {
if (!autoPlay) return;
const timer = setInterval(() => {
setCurrentIndex((currentIndex + 1) % items.length);
}, interval);
return () => clearInterval(timer);
}, [autoPlay, interval, currentIndex, items.length]);
Copy link

Copilot AI Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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]);

Copilot uses AI. Check for mistakes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

자동 슬라이드 기능을 구현한 useEffect의 의존성 배열에 currentIndex가 포함되어 있습니다. 이렇게 하면 슬라이드가 변경될 때마다 setInterval이 해제되고 다시 생성됩니다. 이는 불필요한 재실행을 유발하고 잠재적으로 타이머의 정확성에 영향을 줄 수 있습니다.

useState의 함수형 업데이트를 사용하면 currentIndex에 대한 의존성을 제거할 수 있습니다. 이렇게 수정하면 useEffect는 컴포넌트가 마운트될 때와 autoPlay, interval, items.length가 변경될 때만 실행되어 더 효율적입니다.

Suggested change
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]);


return (
<div className="carousel">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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">&lt;</button>
    <button onClick={goToNext} className="carousel__button carousel__button--next">&gt;</button>
    {/* ... aunchor ... */}
  </div>
);

<div className="carousel__container" {...rest}>
<ul
className="carousel__list"
style={{ transform: `translateX(-${currentIndex * 100}%)` }}
>
{items.map((item, index) => (
<li key={index} className="carousel__item" onClick={item.onClick}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

<li> 요소와 관련하여 두 가지 개선점을 제안합니다.

  1. key prop: 현재 key로 배열의 index를 사용하고 있습니다. 아이템 순서가 변경되거나 중간에 추가/삭제될 경우 예기치 않은 동작이 발생할 수 있습니다. 각 아이템에 고유 id를 부여하고 key로 사용하는 것이 좋습니다.
  2. 접근성: 현재 모든 슬라이드가 DOM에 렌더링되어 스크린 리더가 보이지 않는 슬라이드까지 읽을 수 있습니다. aria-hidden={index !== currentIndex} 속성을 추가하여 현재 보이는 슬라이드만 스크린 리더가 읽도록 할 수 있습니다.

아래와 같이 aria-hidden을 추가하고, key는 추후 고유 ID로 변경하는 것을 고려해보세요.

Suggested change
<li key={index} className="carousel__item" onClick={item.onClick}>
<li key={index} className="carousel__item" onClick={item.onClick} aria-hidden={index !== currentIndex}>

{item.image && (
<img
src={item.image}
alt={item.title}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

이미지의 alt 속성으로 item.title을 사용하고 있습니다. CarouselItem 인터페이스에서 title은 선택적이므로, title이 없는 경우 alt 속성이 누락되어 웹 접근성 가이드라인을 위반하게 됩니다.

이미지에 대한 설명이 없는 경우, alt 속성을 빈 문자열("")로 설정하여 해당 이미지가 장식용임을 명시하는 것이 좋습니다.

Suggested change
alt={item.title}
alt={item.title ?? ''}

className="carousel__image"
style={item.imageStyle}
/>
)}
<div className="carousel__content">
{item.title && (
<h2 className="carousel__title">{item.title}</h2>
)}
{item.text && <p className="carousel__text">{item.text}</p>}
</div>
</li>
))}
</ul>
</div>

<div className="carousel__indicators">
{items.map((_, index) => (
<button
key={index}
className={`carousel__indicator ${
index === currentIndex ? 'active' : ''
}`}
onClick={() => setCurrentIndex(index)}
/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

인디케이터 버튼에 텍스트나 aria-label이 없어 스크린 리더 사용자가 각 버튼의 역할을 이해하기 어렵습니다.

각 버튼에 aria-label을 추가하여 "1번 슬라이드로 이동"과 같이 명확한 설명을 제공하는 것이 접근성 향상에 도움이 됩니다.

          <button
            key={index}
            className={`carousel__indicator ${
              index === currentIndex ? 'active' : ''
            }`}
            onClick={() => setCurrentIndex(index)}
            aria-label={`Go to slide ${index + 1}`}
          />

))}
</div>
</div>
);
};

export default Carousel;
10 changes: 6 additions & 4 deletions src/components/Select/Select.scss
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
.select {
position: relative;
cursor: pointer;
width: 100%;
font-family: 'Times New Roman', serif;
}

Expand All @@ -15,7 +14,9 @@
box-sizing: border-box;
padding: 6px 12px;
background: s.color(paper-background);
background-size: cover;
background-image: url('../../assets/paper.png');
background-repeat: repeat;
background-size: auto;
font-family: 'Times New Roman', serif;
border: 2px solid s.color(black);
@include s.text-style-extended(sm, 800, gray-900);
Expand Down Expand Up @@ -65,7 +66,9 @@
left: 0;
width: 100%;
background: s.color(paper-background);
background-size: cover;
background-image: url('../../assets/paper.png');
background-repeat: repeat;
background-size: auto;
font-family: 'Times New Roman', serif;
border: 2px solid s.color(black);
margin: 4px 0 0 0;
Expand Down Expand Up @@ -109,7 +112,6 @@
&:focus {
@include s.text-style-extended(sm, 700, red-900);
text-shadow: 1px 1px s.color(white);
transition: all 0.15s ease-in-out;
}

// sm 이상
Expand Down
Loading