-
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] Badge 컴포넌트, 스토리북 #31
base: main
Are you sure you want to change the base?
Conversation
워크스루이 풀 리퀘스트는 새로운 Badge 컴포넌트를 구현하고 있습니다. Badge 컴포넌트는 다양한 상태를 시각적으로 표현할 수 있으며, Storybook을 통해 컴포넌트의 다양한 상태와 속성을 확인할 수 있도록 설정되었습니다. 변경 사항
이슈 대상 평가
제안된 리뷰어
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년 02월 09일 13시 07분 23초 |
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: 3
🧹 Nitpick comments (2)
src/shared/ui/Badge/Badge.tsx (2)
47-77
: 상태 로직을 단순화할 수 있습니다.현재 조건문이 복잡하게 중첩되어 있습니다. 상태 매핑 객체를 사용하여 로직을 단순화할 수 있습니다.
- if (isRecriting !== undefined) { - badgeColor = isRecriting ? 'bg-green-100 text-green-300' : 'bg-gray-100 text-gray-400'; - badgeText = isRecriting ? '모집 중' : '모집 마감'; - } else if (isProgress !== undefined) { - if (isProgress === null) { - badgeColor = 'bg-gray-100 text-gray-400'; - badgeText = '진행 전'; - } else { - badgeColor = isProgress ? 'bg-green-100 text-green-300' : 'bg-red-100 text-red-300'; - badgeText = isProgress ? '진행 중' : '마감'; - } - } // ... 나머지 조건문 + const stateConfig = { + recruiting: { + true: { color: 'bg-green-100 text-green-300', text: '모집 중' }, + false: { color: 'bg-gray-100 text-gray-400', text: '모집 마감' }, + }, + progress: { + true: { color: 'bg-green-100 text-green-300', text: '진행 중' }, + false: { color: 'bg-red-100 text-red-300', text: '마감' }, + null: { color: 'bg-gray-100 text-gray-400', text: '진행 전' }, + }, + // ... 다른 상태들도 유사하게 정의 + }; + + if (isRecruiting !== undefined) { + const state = stateConfig.recruiting[String(isRecruiting)]; + badgeColor = state.color; + badgeText = state.text; + } + // ... 다른 상태들도 유사하게 처리
79-83
: 스타일 클래스를 상수로 분리하는 것이 좋습니다.Tailwind CSS 클래스들을 상수로 분리하면 재사용성과 유지보수성이 향상됩니다.
+const BASE_BADGE_CLASSES = 'w-min whitespace-nowrap rounded-xl p-1 px-2 font-semibold'; return ( - <div className={`w-min whitespace-nowrap rounded-xl p-1 px-2 font-semibold ${badgeColor}`}> + <div className={`${BASE_BADGE_CLASSES} ${badgeColor}`}>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/shared/ui/Badge/Badge.stories.tsx
(1 hunks)src/shared/ui/Badge/Badge.tsx
(1 hunks)src/shared/ui/Badge/index.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/shared/ui/Badge/index.ts
🔇 Additional comments (1)
src/shared/ui/Badge/Badge.stories.tsx (1)
17-25
: 기본값이 적절하게 설정되어 있습니다.InteractiveBadge 스토리의 기본값 설정이 잘 되어 있습니다.
argTypes: { | ||
isSubmited: { control: 'radio', options: [true, false, null] }, | ||
isProgress: { control: 'radio', options: [true, false, null] }, | ||
isRecriting: { control: 'boolean' }, |
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
isRecriting
속성의 제어 타입이 일관성이 없습니다.
다른 상태 속성들은 radio
타입으로 null
옵션을 포함하고 있는데 반해, isRecriting
속성만 boolean
타입으로 되어있습니다. 일관성을 위해 다음과 같이 수정하는 것이 좋습니다.
- isRecriting: { control: 'boolean' },
+ isRecriting: { control: 'radio', options: [true, false, null] },
📝 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.
isRecriting: { control: 'boolean' }, | |
isRecriting: { control: 'radio', options: [true, false, null] }, |
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.
수고 많으셨어요 👍
기존에는 아래와 같은 설계를 예상했어요 !
// badge component (공통 view component)
- color와 text 속성으로 주입받음
// badge component를 사용하는 component (도메인)
- 각 상태에 따른 color, text 처리 진행
추상화를 너무 많이하면 이후 다른 도메인에서 사용할 때 다른 속성을 또 추가해야 해야하잖아요.
그래서 공통 컴포넌트로서 잘활용되려면, 도메인적인 성격을 띄지 않아야한다고 생각해요. 유진이의 의견은 어떤가요?
사용이 빈번하지 않은 컴포넌트라 우선순위가 떨어진다 생각해 당장 변경을 요청드리는 부분은 아니고,
추후 속성 추가 등 변경이 요구될 때 고려해주셔도 좋을 듯 싶어요.
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 (4)
src/shared/ui/Badge/Badge.tsx (4)
3-20
: Props 타입을 HTMLDivElement 속성으로 확장하세요.컴포넌트의 유연성을 높이기 위해 div 엘리먼트의 기본 HTML 속성을 상속받는 것이 좋습니다.
다음과 같이 수정하세요:
-type Props = { +type Props = { color?: 'red' | 'green' | 'gray' | null; text?: string; -}; +} & React.HTMLAttributes<HTMLDivElement>;
12-12
: 색상 값을 상수로 정의하세요.색상 값이 여러 곳에서 반복되어 사용되고 있습니다. 유지보수성을 높이기 위해 이를 상수로 분리하는 것이 좋습니다.
다음과 같이 수정하세요:
+const BADGE_COLORS = { + RED: 'red', + GREEN: 'green', + GRAY: 'gray', +} as const; + +const BADGE_STYLES = { + [BADGE_COLORS.RED]: 'bg-red-100 text-red-300', + [BADGE_COLORS.GREEN]: 'bg-green-100 text-green-300', + [BADGE_COLORS.GRAY]: 'bg-gray-100 text-gray-400', +} as const; + - color?: 'red' | 'green' | 'gray' | null; + color?: typeof BADGE_COLORS[keyof typeof BADGE_COLORS] | null;
26-34
: 배지 색상 로직을 단순화하세요.현재 if-else 구문으로 구현된 색상 로직을 더 간단하게 만들 수 있습니다.
상수를 사용하여 다음과 같이 수정하세요:
- let badgeColor = 'bg-gray-100 text-gray-400'; - - if (color === 'green') { - badgeColor = 'bg-green-100 text-green-300'; - } else if (color === 'red') { - badgeColor = 'bg-red-100 text-red-300'; - } else if (color === 'gray') { - badgeColor = 'bg-gray-100 text-gray-400'; - } + const badgeColor = color ? BADGE_STYLES[color] : BADGE_STYLES[BADGE_COLORS.GRAY];
39-39
: className 문자열을 더 관리하기 쉽게 만드세요.긴 className 문자열은 가독성이 떨어지고 관리하기 어려울 수 있습니다. 이를 개선하기 위해 clsx나 tailwind-merge와 같은 유틸리티를 사용하는 것이 좋습니다.
다음과 같이 수정하세요:
+import { clsx } from 'clsx'; + - <div className={`w-min whitespace-nowrap rounded-xl p-1 px-2 font-semibold ${badgeColor}`}> + <div + className={clsx( + 'w-min whitespace-nowrap rounded-xl p-1 px-2 font-semibold', + badgeColor + )} + >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/shared/ui/Badge/Badge.stories.tsx
(1 hunks)src/shared/ui/Badge/Badge.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/shared/ui/Badge/Badge.stories.tsx
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/Badge/Badge.tsx (2)
3-20
: Props 타입 정의를 개선하세요.다음과 같은 개선사항을 제안합니다:
- 색상 값을 상수로 관리하여 타입 안정성을 높이고 재사용성을 개선하세요.
- 접근성을 위해 aria-label 속성을 추가하세요.
+const BADGE_COLORS = { + RED: 'red', + GREEN: 'green', + GRAY: 'gray', +} as const; type Props = { /** * - `'green'`: 초록색 (`bg-green-100 text-green-300`) * - `'red'`: 빨간색 (`bg-red-100 text-red-300`) * - `'gray'`: 회색 (`bg-gray-100 text-gray-400`) * - `null`: 기본값 (회색) * * @default null */ - color?: 'red' | 'green' | 'gray' | null; + color?: typeof BADGE_COLORS[keyof typeof BADGE_COLORS] | null; /** * 특정 상태가 없을 경우 * * @default '상태 없음' */ text?: string; + + /** + * 접근성을 위한 레이블 + * @default text prop value + */ + 'aria-label'?: string; } & React.HTMLAttributes<HTMLDivElement>;
25-46
: 컴포넌트 구현을 개선하세요.다음과 같은 개선사항을 제안합니다:
- 색상 매핑을 상수 객체로 분리하여 관리하세요.
- 접근성을 위한 aria-label을 추가하세요.
- 스타일링 관리를 위해 CSS 모듈이나 styled-components 사용을 고려하세요.
+const BADGE_STYLES = { + green: 'bg-green-100 text-green-300', + red: 'bg-red-100 text-red-300', + gray: 'bg-gray-100 text-gray-400', +} as const; -export function Badge({ color, text, ...props }: Props) { +export function Badge({ color, text, 'aria-label': ariaLabel, ...props }: Props) { - let badgeColor = 'bg-gray-100 text-gray-400'; - - if (color === 'green') { - badgeColor = 'bg-green-100 text-green-300'; - } else if (color === 'red') { - badgeColor = 'bg-red-100 text-red-300'; - } else if (color === 'gray') { - badgeColor = 'bg-gray-100 text-gray-400'; - } + const badgeColor = color ? BADGE_STYLES[color] : BADGE_STYLES.gray; const badgeText = text ?? '상태 없음'; return ( <div className={`w-min whitespace-nowrap rounded-xl p-1 px-2 font-semibold ${badgeColor}`} + aria-label={ariaLabel ?? badgeText} {...props} > {badgeText} </div> ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/shared/ui/Badge/Badge.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: preview-storybook
🔇 Additional comments (1)
src/shared/ui/Badge/Badge.tsx (1)
1-46
: 테스트 코드를 추가하세요.컴포넌트의 안정성을 위해 다음 사항들에 대한 테스트 코드 작성을 권장합니다:
- 각 색상 변형에 대한 테스트
- 기본 텍스트 동작 테스트
- 접근성 속성 테스트
- props 전달 테스트
테스트 파일 작성이 필요하시다면 제가 도움을 드릴 수 있습니다. 테스트 코드를 생성해드릴까요?
🔥 연관 이슈
🚀 작업 내용
🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
Summary by CodeRabbit
새로운 기능
문서화