-
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] Flex 컴포넌트, 스토리북 #27
base: main
Are you sure you want to change the base?
Conversation
워크스루이 풀 리퀘스트는 변경 사항
연결된 이슈 평가
관련 가능성 있는 PR
제안된 라벨
제안된 리뷰어
📜 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: 2025년 02월 01일 02시 39분 12초 |
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 (4)
src/shared/ui/Flex/Flex.stories.tsx (2)
11-14
: 컴포넌트 설명을 더 자세히 보완해주세요.현재 설명은 기본적인 내용만 포함하고 있습니다. 사용 사례나 주의사항 등을 추가하면 더 유용할 것 같습니다.
예시:
- 'Flex 컴포넌트는 display: flex를 적용해 정렬과 배치를 간편하게 조정할 수 있는 컨테이너입니다.', + 'Flex 컴포넌트는 display: flex를 적용해 정렬과 배치를 간편하게 조정할 수 있는 컨테이너입니다. dir, items, justify, wrap 속성을 통해 자식 요소들의 배치를 제어할 수 있으며, className을 통해 추가적인 스타일링이 가능합니다. 반응형 레이아웃 구성에 특히 유용합니다.'
57-63
: Basic 스토리의 예제를 더 다양하게 보완해주세요.현재 예제는 기본적인 사용법만 보여주고 있습니다. 다양한 속성 조합의 예시를 추가하면 컴포넌트의 활용도를 더 잘 이해할 수 있을 것 같습니다.
예시:
render: (args) => ( - <Flex {...args} className="w-full"> - <div className="size-10 bg-red-300">1</div> - <div className="size-10 bg-green-300">2</div> - <div className="size-10 bg-primary-300">3</div> - </Flex> + <div className="space-y-4"> + <Flex {...args} className="w-full gap-2"> + <div className="size-10 bg-red-300">1</div> + <div className="size-10 bg-green-300">2</div> + <div className="size-10 bg-primary-300">3</div> + </Flex> + <Flex {...args} className="w-full h-32 border-2 border-dashed border-gray-300"> + <div className="p-4 bg-blue-300">반응형</div> + <div className="p-4 bg-yellow-300">레이아웃</div> + </Flex> + </div> + ),src/shared/ui/Flex/Flex.tsx (2)
36-66
: gap 속성 지원을 고려해보세요.현재 gap은 className을 통해 적용해야 하지만, 자주 사용되는 속성이므로 전용 prop으로 제공하면 사용성이 개선될 것 같습니다.
예시:
type Props = { + /** + * Defines the gap between flex items. + * @default 0 + */ + gap?: 0 | 1 | 2 | 3 | 4 | 5 | 6 | 8 | 10 | 12 | 16; // ... 기존 props };
78-88
: 접근성을 위한 role 속성 추가를 고려해보세요.시맨틱한 의미를 부여하기 위해 role 속성을 추가하면 좋을 것 같습니다.
예시:
<div className={cn( 'flex', DIR[dir], ALIGN_ITEMS[items], JUSTIFY_CONTENT[justify], WRAP[wrap], className )} + role="group" {...props} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/shared/ui/Flex/Flex.stories.tsx
(1 hunks)src/shared/ui/Flex/Flex.tsx
(1 hunks)src/shared/ui/Flex/index.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/shared/ui/Flex/index.ts
export function Flex({ | ||
dir = 'row', | ||
items = 'stretch', | ||
justify = 'start', | ||
wrap = 'nowrap', | ||
className = '', | ||
children, | ||
...props | ||
}: Props) { |
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
...props의 타입 안전성을 개선해주세요.
현재 ...props의 타입이 명시되어 있지 않아 의도하지 않은 속성이 전달될 수 있습니다.
예시:
- ...props
+ ...props
}: Props & Omit<React.HTMLAttributes<HTMLDivElement>, keyof Props>) {
📝 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 function Flex({ | |
dir = 'row', | |
items = 'stretch', | |
justify = 'start', | |
wrap = 'nowrap', | |
className = '', | |
children, | |
...props | |
}: Props) { | |
export function Flex({ | |
dir = 'row', | |
items = 'stretch', | |
justify = 'start', | |
wrap = 'nowrap', | |
className = '', | |
children, | |
...props | |
}: Props & Omit<React.HTMLAttributes<HTMLDivElement>, keyof Props>) { |
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.
수고많으셨어요 👍
Props 키 값으로 alignItems와 items 중 고민했는만, 사용 할 경우 더 짧게 더 낫지 않을까 싶어서 items를 선택했습니다. 다만, 직관성이 다소 떨어질 수 있어 어떤 방향이 더 좋을지 의견 주시면 감사하겠습니다!
items로도 유추에 어려움이 없을 것으로 예상해요. 선택해주신 방향대로 진행해도 좋습니다
🔥 연관 이슈
🚀 작업 내용
🤔 고민했던 내용
gap
,width
,height
를Props
에 포함시킬까 하다가 tailwind에서 사용하는 style 체계가 다음 사진과 같고 동적 지원이 어려워서 일부 값은className
을 통해 적용해야 할 것 같습니다.Props
키 값으로alignItems
와items
중 고민했는만, 사용 할 경우 더 짧게 더 낫지 않을까 싶어서items
를 선택했습니다. 다만, 직관성이 다소 떨어질 수 있어 어떤 방향이 더 좋을지 의견 주시면 감사하겠습니다!Summary by CodeRabbit
새로운 기능
Flex
컴포넌트 추가문서화
Flex
컴포넌트 문서 및 기본 스토리 추가개선 사항
수출 추가
Flex
컴포넌트를 중앙 위치에서 접근 가능하도록 수출 추가