[FEAT] Chip 컴포넌트,스토리북 구현 #52
Conversation
Walkthrough이 PR은 Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@keemsebin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2024년 11월 17일 23시 52분 44초 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/components/common/Chip/Chip.types.ts (1)
1-6: Chip 컴포넌트의 확장성 개선을 위한 제안현재 구현은 기본적인 기능을 잘 수행하고 있지만, 다음과 같은 선택적 props를 추가하면 더 유연한 컴포넌트가 될 것 같습니다:
export type Props = { /** * content to be displayed within the Chip component. */ children: string; + /** + * 칩의 변형 스타일을 지정합니다. + */ + variant?: 'filled' | 'outlined'; + /** + * 칩의 크기를 지정합니다. + */ + size?: 'small' | 'medium' | 'large'; + /** + * 칩 클릭 시 실행될 콜백 함수 + */ + onClick?: () => void; + /** + * 칩을 비활성화 상태로 설정합니다. + */ + disabled?: boolean; };src/components/common/Chip/Chip.stories.tsx (2)
1-4: ESLint 규칙 비활성화에 대한 설명이 필요합니다ESLint 규칙을 비활성화한 이유를 주석으로 설명해주시면 좋겠습니다. 향후 유지보수를 위해 이 결정의 배경을 이해하는 것이 중요합니다.
-/* eslint-disable no-restricted-exports */ +/* eslint-disable no-restricted-exports */ +// 스토리북 메타데이터를 default export로 내보내야 하기 때문에 규칙을 비활성화합니다
15-27: 스토리 구현을 더 풍부하게 만들어보세요기본적인 구현은 잘 되어있지만, 다음과 같은 개선사항을 고려해보시면 좋겠습니다:
- 다양한 사용 사례를 보여주는 추가 스토리 구현 (예: 다른 스타일, 크기 등)
- 컴포넌트 사용법에 대한 설명 추가
- 접근성(a11y) 관련 정보 포함
예시 구현:
export const WithCustomStyle: Story = { args: { children: '인기', }, parameters: { docs: { description: { story: '커스텀 스타일이 적용된 Chip 컴포넌트입니다.', }, }, }, // ... 추가 구현 };tailwind.config.ts (2)
10-12: 폰트 패밀리 설정의 중복성 개선 필요
sans와pretendard가 동일한 폰트를 사용하고 있어 중복이 발생합니다. 하나의 키로 통일하는 것이 유지보수에 더 효율적일 것 같습니다.다음과 같이 수정하는 것을 제안드립니다:
fontFamily: { - sans: ['Pretendard'], pretendard: ['Pretendard'], },
Line range hint
27-29: z-index 확장성 고려 필요모달의 z-index를 100으로 설정하셨는데, 향후 다른 오버레이 요소들과의 관계를 고려하여 더 체계적인 z-index 전략이 필요할 수 있습니다.
다음과 같은 계층 구조를 제안드립니다:
zIndex: { dropdown: '10', sticky: '20', fixed: '30', drawer: '40', modal: '50', popover: '60', tooltip: '70', toast: '80', }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/components/common/Chip/Chip.stories.tsx(1 hunks)src/components/common/Chip/Chip.types.ts(1 hunks)src/components/common/Chip/index.tsx(1 hunks)src/pages/Link.tsx(1 hunks)tailwind.config.ts(1 hunks)
🔇 Additional comments (7)
src/components/common/Chip/Chip.types.ts (1)
1-6: 기본 구현이 깔끔합니다!
타입 정의가 명확하고 JSDoc 문서화가 잘 되어있습니다.
src/components/common/Chip/index.tsx (1)
1-2: Props 타입 정의 검증 필요
Props 타입이 별도의 파일에서 정의되어 있습니다. children 외에 추가로 필요한 속성이 있는지 검토해 주세요:
- variant/size 등 스타일 변형을 위한 속성
- onClick 등 이벤트 핸들러
- disabled 상태 처리
Props 타입 정의를 확인하기 위해 다음 스크립트를 실행하세요:
✅ Verification successful
Let me check the component implementation to provide a complete verification. I'll run another script to see how the Chip component is using these props.
Props 타입 정의가 적절합니다
현재 Chip 컴포넌트의 구현과 Props 타입 정의를 검토한 결과:
- children prop이 string 타입으로 제한되어 있어 적절합니다
- ...props를 통해 필요한 HTML span 속성들을 전달할 수 있습니다
- 고정된 스타일 클래스를 사용하는 단순한 디자인 시스템 컴포넌트로서 추가 Props가 필요하지 않습니다
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Props 타입 정의 확인
ast-grep --pattern 'type Props = {
$$$
}'
Length of output: 3889
Script:
#!/bin/bash
# Check Chip component implementation
cat src/components/common/Chip/index.tsx
Length of output: 314
src/components/common/Chip/Chip.stories.tsx (2)
6-12: 메타 구성이 잘 되어있습니다
타입 안전성이 보장되어 있고, autodocs 태그가 포함되어 자동 문서화가 가능하도록 구성되어 있습니다.
13-13: 타입 정의가 명확합니다
Story 타입이 Chip 컴포넌트에 맞게 잘 정의되어 있습니다.
tailwind.config.ts (1)
Line range hint 14-23: 타이포그래피 시스템이 적절히 구성되었습니다
타이틀과 본문 텍스트의 크기가 계층적으로 잘 정의되어 있습니다. 디자인 시스템의 일관성을 잘 유지할 수 있을 것 같습니다.
src/pages/Link.tsx (2)
Line range hint 1-65: 전반적인 구현이 잘 되어있습니다! 👍
- React 훅들의 적절한 사용
- 명확한 컴포넌트 구조
- 적절한 에러 처리
46-46: 액션 타입 변경에 대한 검증이 필요합니다.
'link'에서 'SET_LINK'로의 액션 타입 변경이 확인되었습니다. 이 변경이 리듀서와 다른 컴포넌트들에서 일관되게 적용되었는지 확인이 필요합니다.
액션 타입의 일관성을 위해 상수로 분리하는 것을 고려해보세요. 예시:
// youtubeActions.ts
export const YOUTUBE_ACTIONS = {
SET_LINK: 'SET_LINK',
} as const;✅ Verification successful
Let me try a different approach to verify the action types in the codebase.
Let me try one more search with simpler patterns:
Let me check the context implementation:
액션 타입 변경이 올바르게 구현되었습니다.
useYoutubeContext.tsx의 리듀서 구현을 확인한 결과, 'SET_LINK' 액션 타입이 정확하게 정의되어 있으며 Link.tsx 컴포넌트에서의 사용도 일관성 있게 구현되어 있습니다. 추가적인 변경이 필요하지 않습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of the action type change across the codebase
# Search for any remaining occurrences of the old action type
echo "Checking for old action type 'link'..."
rg "type:\s*'link'" -t tsx -t ts
# Search for the new action type to ensure it's properly implemented in the reducer
echo "Checking for new action type 'SET_LINK'..."
rg "type:\s*'SET_LINK'" -t tsx -t ts
# Look for the reducer implementation
echo "Checking reducer implementation..."
ast-grep --pattern "case 'SET_LINK':"
Length of output: 460
Script:
#!/bin/bash
# Search for any occurrences of 'link' in dispatch calls
echo "Checking for 'link' action type..."
rg "dispatch.*link" --type=typescript --type=javascriptreact --type=typescriptreact
# Search for SET_LINK in dispatch calls and reducer
echo "Checking for 'SET_LINK' action type..."
rg "SET_LINK" --type=typescript --type=javascriptreact --type=typescriptreact
# Look for reducer files
echo "Looking for reducer files..."
fd "reducer" --type=f --extension ts --extension tsx --extension js --extension jsx
Length of output: 562
Script:
#!/bin/bash
# Search for any files with 'link' or 'SET_LINK'
echo "Searching for relevant files..."
rg -l "link|SET_LINK"
# Look specifically in src directory
echo "Contents of src directory..."
ls -R src/
# Try to find reducer related files
echo "Looking for reducer or context files..."
rg -l "reducer|context"
Length of output: 3858
Script:
#!/bin/bash
# Check the YouTube context implementation
echo "Checking YouTube context implementation..."
cat src/hooks/common/useYoutubeContext.tsx
# Check the Link component implementation
echo "Checking Link component implementation..."
cat src/pages/Link.tsx
Length of output: 2772
src/components/common/Chip/index.tsx
Outdated
| export const Chip = ({ children, ...props }: Props) => { | ||
| return ( | ||
| <span | ||
| className="h-7 w-7 bg-primary rounded-md text-body4 text-white cursor-default px-3 py-1" | ||
| {...props} | ||
| > | ||
| {children} | ||
| </span> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
컴포넌트 구현 개선 필요
현재 구현에서 다음 사항들의 개선이 필요합니다:
- 접근성 개선
- 반응형 디자인 고려
- 인터랙션 상태 추가
- 크기 설정 유연성
다음과 같은 개선사항을 제안드립니다:
-export const Chip = ({ children, ...props }: Props) => {
+export const Chip = ({
+ children,
+ className,
+ size = 'medium',
+ ...props
+}: Props) => {
return (
<span
- className="h-7 w-7 bg-primary rounded-md text-body4 text-white cursor-default px-3 py-1"
+ className={clsx(
+ 'bg-primary rounded-md text-white cursor-default transition-colors',
+ 'hover:bg-primary-dark focus:ring-2 focus:ring-primary-light',
+ {
+ 'px-2 py-0.5 text-body5': size === 'small',
+ 'px-3 py-1 text-body4': size === 'medium',
+ 'px-4 py-2 text-body3': size === 'large',
+ },
+ className
+ )}
+ role="status"
{...props}
>
{children}
</span>
);
};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
src/components/common/Chip/Chip.types.ts (1)
1-6: Props 타입에 선택적 속성 추가를 고려해보세요.현재 모든 속성이 필수로 지정되어 있어 컴포넌트의 유연성이 제한될 수 있습니다. 다음과 같은 선택적 속성 추가를 제안드립니다:
className?: string- 커스텀 스타일링을 위한 속성onClick?: () => void- 클릭 이벤트 핸들러export type Props = { /** * The variant of the Chip. * @default 'large' */ variant: 'small' | 'medium' | 'large'; + /** + * Additional CSS classes to apply to the chip. + */ + className?: string; + /** + * Optional click handler for the chip. + */ + onClick?: () => void; /**src/components/common/Chip/Chip.variants.ts (1)
3-3: 커서 스타일 개선 제안현재
cursor-default로 설정되어 있는데, 칩이 클릭 가능하거나 상호작용이 가능한 경우를 대비하여 이를 variant로 분리하는 것이 좋을 것 같습니다.다음과 같은 구현을 제안합니다:
- export const ChipVariants = cva(`bg-primary rounded-md text-white cursor-default px-3 py-1`, { + export const ChipVariants = cva(`bg-primary rounded-md text-white px-3 py-1`, { variants: { + interactive: { + true: 'cursor-pointer hover:bg-primary-dark', + false: 'cursor-default' + }, variant: { // ... existing variants } }, defaultVariants: { variant: 'medium', + interactive: false } });src/components/common/Chip/Chip.stories.tsx (2)
1-5: ESLint 규칙 비활성화에 대한 설명이 필요합니다ESLint 규칙을 비활성화하는 이유를 주석으로 명시하면 다른 개발자들이 이해하는데 도움이 될 것 같습니다.
다음과 같이 수정하는 것을 제안합니다:
-/* eslint-disable no-restricted-exports */ +/* eslint-disable no-restricted-exports -- Storybook 설정에 필요한 default export를 허용하기 위해 비활성화 */
15-32: 스토리 구현에 대한 개선 제안스토리 구현이 기본적인 기능은 잘 보여주고 있지만, 다음과 같은 개선사항들을 제안드립니다:
- variant 옵션을 컴포넌트의 타입에서 가져오는 것이 좋을 것 같습니다.
- 다양한 사용 사례를 보여주는 추가 스토리들이 있으면 좋을 것 같습니다.
variant 옵션을 타입에서 가져오도록 수정하는 예시입니다:
+import { ChipVariant } from './Chip.types'; export const Basic: Story = { args: { variant: 'medium', children: '카페', }, argTypes: { variant: { control: 'inline-radio', - options: ['small', 'medium', 'large'], + options: Object.values(ChipVariant), }, children: { control: 'text', }, },추가로 다음과 같은 스토리들을 구현하는 것을 추천드립니다:
export const SmallChip: Story = { args: { variant: 'small', children: '작은 칩', }, }; export const LargeChip: Story = { args: { variant: 'large', children: '큰 칩', }, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/components/common/Button/Button.variants.ts(0 hunks)src/components/common/Chip/Chip.stories.tsx(1 hunks)src/components/common/Chip/Chip.types.ts(1 hunks)src/components/common/Chip/Chip.variants.ts(1 hunks)src/components/common/Chip/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/common/Button/Button.variants.ts
🔇 Additional comments (5)
src/components/common/Chip/Chip.types.ts (1)
1-11: 전반적인 타입 정의가 잘 되어있습니다!
타입 정의와 JSDoc 문서화가 명확하게 작성되어 있습니다.
src/components/common/Chip/index.tsx (2)
1-2: 코드 구조가 잘 모듈화되어 있습니다!
타입과 스타일 변형을 별도의 파일로 분리한 것이 좋은 구조입니다.
4-10: 이전 리뷰의 개선 제안사항이 여전히 유효합니다
이전 리뷰에서 제안된 다음 개선사항들을 반영하면 좋겠습니다:
- 반응형 디자인 고려
- 호버, 포커스 등의 인터랙션 상태 추가
- className prop을 통한 스타일 확장성 제공
src/components/common/Chip/Chip.stories.tsx (2)
6-12: 메타 설정이 잘 구성되어 있습니다
컴포넌트 메타데이터가 타입 안전성을 보장하면서 깔끔하게 구성되어 있습니다. autodocs 태그를 포함하여 자동 문서화도 잘 설정되어 있습니다.
13-13: 타입 정의가 명확합니다
Story 타입이 Chip 컴포넌트에 맞게 잘 정의되어 있습니다.
| /** | ||
| * content to be displayed within the Chip component. | ||
| */ | ||
| children: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 개선을 위한 aria-label 속성 추가를 권장드립니다.
스크린 리더 사용자를 위해 Chip의 목적을 설명하는 aria-label을 추가하면 좋을 것 같습니다.
/**
* content to be displayed within the Chip component.
*/
children: string;
+ /**
+ * Accessible label for screen readers.
+ * @example '카테고리: JavaScript'
+ */
+ 'aria-label'?: string;📝 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.
| /** | |
| * content to be displayed within the Chip component. | |
| */ | |
| children: string; | |
| /** | |
| * content to be displayed within the Chip component. | |
| */ | |
| children: string; | |
| /** | |
| * Accessible label for screen readers. | |
| * @example '카테고리: JavaScript' | |
| */ | |
| 'aria-label'?: string; |
| export const Chip = ({ variant, children, ...props }: Props) => { | ||
| return ( | ||
| <span className={ChipVariants({ variant })} {...props}> | ||
| {children} | ||
| </span> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 개선이 필요합니다
다음과 같은 접근성 관련 속성들을 추가하는 것이 좋습니다:
-export const Chip = ({ variant, children, ...props }: Props) => {
+export const Chip = ({ variant, children, role = "status", ...props }: Props) => {
return (
<span
+ role={role}
+ aria-label={typeof children === 'string' ? children : undefined}
className={ChipVariants({ variant })}
{...props}
>
{children}
</span>
);
};📝 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.
| export const Chip = ({ variant, children, ...props }: Props) => { | |
| return ( | |
| <span className={ChipVariants({ variant })} {...props}> | |
| {children} | |
| </span> | |
| ); | |
| }; | |
| export const Chip = ({ variant, children, role = "status", ...props }: Props) => { | |
| return ( | |
| <span | |
| role={role} | |
| aria-label={typeof children === 'string' ? children : undefined} | |
| className={ChipVariants({ variant })} | |
| {...props} | |
| > | |
| {children} | |
| </span> | |
| ); | |
| }; |
| small: 'h-6 w-6 text-caption', | ||
| medium: 'h-7 w-7 text-body4', | ||
| large: 'h-8 w-8 text-body3', |
There was a problem hiding this comment.
고정된 너비 값으로 인한 텍스트 잘림 문제
현재 각 variant에 고정된 너비 값(w-6, w-7, w-8)이 설정되어 있어, 칩 내부의 텍스트가 잘릴 수 있습니다. 칩은 내부 콘텐츠의 길이에 따라 유동적으로 너비가 조절되어야 합니다.
다음과 같이 수정하는 것을 제안합니다:
variant: {
- small: 'h-6 w-6 text-caption',
- medium: 'h-7 w-7 text-body4',
- large: 'h-8 w-8 text-body3',
+ small: 'h-6 min-w-6 text-caption',
+ medium: 'h-7 min-w-7 text-body4',
+ large: 'h-8 min-w-8 text-body3',
},📝 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.
| small: 'h-6 w-6 text-caption', | |
| medium: 'h-7 w-7 text-body4', | |
| large: 'h-8 w-8 text-body3', | |
| small: 'h-6 min-w-6 text-caption', | |
| medium: 'h-7 min-w-7 text-body4', | |
| large: 'h-8 min-w-8 text-body3', |
관련 이슈
close #27
📑 작업 내용
💬 리뷰 중점 사항/기타 참고 사항
Summary by CodeRabbit
새로운 기능
Chip컴포넌트의 새로운 Storybook 스토리 추가Chip컴포넌트 생성 및 자식 요소 렌더링 기능 추가Chip컴포넌트를 위한 CSS 클래스 변형 정의 추가수정 사항
Link컴포넌트의 상태 업데이트 액션 타입 변경 (링크에서SET_LINK로)타입 정의
Chip컴포넌트의 속성을 정의하는 새로운 타입Props추가