-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT] Switch 컴포넌트, 스토리북 제작 #20
base: main
Are you sure you want to change the base?
Conversation
Walkthrough이 풀 리퀘스트는 새로운 Switch 컴포넌트를 도입합니다. Radix UI의 Switch 라이브러리를 사용하여 커스터마이징 가능한 스위치 UI 요소를 구현했습니다. 컴포넌트는 색상 prop을 통해 동적 스타일링을 지원하며, Storybook을 통해 컴포넌트의 문서화와 테스트를 용이하게 합니다. 또한 Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
Update: 2025년 01월 27일 14시 43분 15초 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/shared/ui/Switch/Switch.stories.tsx (1)
7-31
: 스토리북 구성이 잘 되어있습니다!스토리북 설정이 체계적으로 구성되어 있으며, 특히 color prop에 대한 문서화가 잘 되어 있습니다. 다만 다음과 같은 개선사항을 제안드립니다:
- 컴포넌트의 상태(checked/unchecked)에 대한 스토리도 추가하면 좋을 것 같습니다.
- 접근성 테스트를 위한 play 함수 추가를 고려해보세요.
src/shared/ui/Switch/Switch.tsx (1)
13-23
: 구현이 깔끔하며 접근성이 잘 고려되어 있습니다.Radix UI를 활용하여 접근성이 보장되어 있으며, CSS 변수를 통한 동적 스타일링이 효과적으로 구현되어 있습니다. 다만 몇 가지 제안사항이 있습니다:
- 스위치의 상태가 변경될 때 사용자에게 피드백을 제공하는 것이 좋을 것 같습니다.
- 테마 시스템과의 통합을 고려해보세요.
다음과 같은 개선사항을 제안드립니다:
export function Switch({ color = 'primary', ...props }: Props) { return ( <SwitchPrimitives.Root {...props} + aria-label={props['aria-label'] || '상태 전환'} className="group relative flex h-3.5 w-7 items-center rounded-full shadow-lg data-[state=checked]:bg-[var(--switch-color)] data-[state=unchecked]:bg-gray-500" style={color && ({ '--switch-color': COLORS[color] } as React.CSSProperties)} > <SwitchPrimitives.Thumb className="h-5 w-5 -translate-x-1.5 rounded-full bg-white shadow-inner duration-100 group-data-[state=checked]:translate-x-3.5" /> </SwitchPrimitives.Root> ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (6)
.gitignore
(1 hunks)package.json
(1 hunks)src/shared/lib/colors.ts
(1 hunks)src/shared/ui/Switch/Switch.stories.tsx
(1 hunks)src/shared/ui/Switch/Switch.tsx
(1 hunks)src/shared/ui/Switch/index.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- src/shared/ui/Switch/index.ts
🔇 Additional comments (3)
src/shared/ui/Switch/Switch.tsx (1)
5-11
: 타입 정의가 명확합니다!Props 타입 정의와 JSDoc 문서화가 잘 되어 있습니다.
src/shared/lib/colors.ts (1)
51-51
: 타입 정의가 일관성 있게 추가되었습니다.기존 색상 시스템과 잘 통합되어 있습니다.
package.json (1)
15-15
: Radix UI Switch 의존성이 적절하게 추가되었습니다.접근성과 사용성이 검증된 Radix UI 라이브러리를 선택한 것이 좋은 판단이었습니다.
src/shared/ui/Switch/Switch.tsx
Outdated
return ( | ||
<SwitchPrimitives.Root | ||
{...props} | ||
className="group relative flex h-3.5 w-7 items-center rounded-full shadow-lg data-[state=checked]:bg-[var(--switch-color)] data-[state=unchecked]:bg-gray-500" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relative
속성은 지정하신건지, 기존에 포함되어 있던 건지 궁금합니당
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4f49357 불필요한 속성이 포함되어져있었네요. 감사합니다 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후에 hover 애니메이션도 적용하면 좋을 것 같습니다
수고하셨습니다!
@@ -48,4 +48,5 @@ export const COLORS = { | |||
|
|||
export type Colors = keyof typeof colors; | |||
export type IconColor = keyof typeof COLORS; | |||
export type SwitchColor = keyof typeof COLORS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피그마 상에는 다른 색상은 사용되고 있지 않은 것으로 확인했는데, 다른 색상의 사용 활용에 대해 전달받으신 부분이 있는지 궁금합니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR에도 남긴 부분인데, 디자인팀과는 primary 단일 색상으로 의견을 맞췄어요.
하지만 다른 색상을 적용해야하는 경우가 존재할 수 있다고 판단했고, 이 경우에 대응이 쉽도록 color 속성 + defualt
처리했어요 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/shared/ui/Switch/Switch.tsx (3)
5-11
: Props 타입의 안전성 개선이 필요합니다.
color
prop의 기본값이 'primary'로 설정되어 있으므로, 타입 정의에서도 이를 반영하여 더 정확한 타입 안전성을 제공할 수 있습니다.다음과 같이 수정하는 것을 제안드립니다:
type Props = React.ComponentProps<typeof SwitchPrimitives.Root> & { /** * color of the switch. * @default 'primary' */ - color?: SwitchColor; + color: SwitchColor; };
18-18
: style prop 최적화가 필요합니다.
style
prop이 렌더링마다 새로운 객체를 생성하고 있습니다. 이는 불필요한 리렌더링을 유발할 수 있습니다.다음과 같이
useMemo
를 사용하여 최적화하는 것을 제안드립니다:+const getStyleForColor = (color: SwitchColor) => + ({ '--switch-color': COLORS[color] } as React.CSSProperties); export function Switch({ color = 'primary', ...props }: Props) { + const style = React.useMemo(() => getStyleForColor(color), [color]); return ( <SwitchPrimitives.Root {...props} className="..." - style={color && ({ '--switch-color': COLORS[color] } as React.CSSProperties)} + style={style} >
20-20
: 매직 넘버를 상수로 추출하는 것이 좋습니다.Tailwind 클래스에 하드코딩된 translate 값들을 상수로 추출하면 유지보수성이 향상됩니다.
컴포넌트 상단에 다음과 같이 상수를 정의하는 것을 제안드립니다:
const THUMB_POSITIONS = { unchecked: '-translate-x-1.5', checked: 'translate-x-3.5', } as const;그리고 className에서 다음과 같이 사용:
-className="h-5 w-5 -translate-x-1.5 rounded-full bg-white shadow-inner drop-shadow-md duration-100 group-data-[state=checked]:translate-x-3.5" +className={`h-5 w-5 ${THUMB_POSITIONS.unchecked} rounded-full bg-white shadow-inner drop-shadow-md duration-100 group-data-[state=checked]:${THUMB_POSITIONS.checked}`}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/shared/ui/Switch/Switch.tsx
(1 hunks)
🔇 Additional comments (1)
src/shared/ui/Switch/Switch.tsx (1)
13-23
: 컴포넌트 테스트 코드 작성이 필요합니다.Switch 컴포넌트의 동작을 검증하기 위한 테스트 코드가 없습니다. 주요 기능과 접근성을 검증하는 테스트를 추가해주세요.
테스트 코드 작성을 도와드릴까요? 다음과 같은 테스트 케이스들을 제안드립니다:
- 기본 렌더링 테스트
- 색상 변경 테스트
- 토글 동작 테스트
- 접근성 검증 테스트
return ( | ||
<SwitchPrimitives.Root | ||
{...props} | ||
className="group flex h-3.5 w-7 items-center rounded-full shadow-lg data-[state=checked]:bg-[var(--switch-color)] data-[state=unchecked]:bg-gray-500" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
접근성 개선이 필요합니다.
Switch 컴포넌트의 접근성을 높이기 위해 ARIA 레이블과 역할을 명시적으로 설정하는 것이 좋습니다.
다음과 같이 수정하는 것을 제안드립니다:
className="group flex h-3.5 w-7 items-center rounded-full shadow-lg data-[state=checked]:bg-[var(--switch-color)] data-[state=unchecked]:bg-gray-500"
+aria-label="Toggle switch"
+role="switch"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
className="group flex h-3.5 w-7 items-center rounded-full shadow-lg data-[state=checked]:bg-[var(--switch-color)] data-[state=unchecked]:bg-gray-500" | |
className="group flex h-3.5 w-7 items-center rounded-full shadow-lg data-[state=checked]:bg-[var(--switch-color)] data-[state=unchecked]:bg-gray-500" | |
aria-label="Toggle switch" | |
role="switch" |
🔥 연관 이슈
🚀 작업 내용
Summary by CodeRabbit
새로운 기능
종속성
@radix-ui/react-switch
라이브러리 추가기타