Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough새로운 재사용 버튼 컴포넌트 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 사용자
participant Page as CourseSettingPreview
participant Button as CommonButton
User->>Page: 페이지 열람
Page->>Button: 9개 버튼 렌더링 (label, variant, onClick)
note right of Page #E8F8F5: 로컬 상태 active: null
User->>Button: 버튼 클릭(id)
Button->>Page: onClick -> toggleActive(id)
alt id !== active
Page->>Page: setActive(id)
Page->>Button: 해당 버튼 variant="active"
else id === active
Page->>Page: setActive(null)
Page->>Button: 모든 버튼 variant="default"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🏷️ Labeler has automatically applied labels based on your PR title, branch name, or commit message. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/map/index.tsx(1 hunks)src/shared/components/button/CommonButton.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/shared/components/button/CommonButton.tsx (1)
src/shared/lib/utils.ts (1)
cn(71-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (3)
src/shared/components/button/CommonButton.tsx (1)
10-28: 버튼 스타일 정의 잘 구현되었습니다
cva를 사용한 variant 기반 스타일 정의가 깔끔하고 확장 가능하게 구현되었습니다.default와activevariant가 명확하게 구분되어 있습니다.src/pages/map/index.tsx (2)
4-17: 상태 관리와 버튼 구성이 잘 구현되었습니다
useState를 활용한 active 상태 관리가 적절하고, 버튼 배열의 구조(id,label)가 명확합니다. 한글 레이블도 사용자 친화적입니다.
24-31: 토글 로직이 정확하게 구현되었습니다Line 29의
onClick핸들러에서 이미 활성화된 버튼을 다시 클릭하면null로 설정하고, 그렇지 않으면 해당id로 설정하는 토글 로직이 정확합니다.variantprop도 상태에 따라 올바르게 전환됩니다.
KongMezu
left a comment
KongMezu
left a comment
There was a problem hiding this comment.
새벽까지 수고하셨습니다! 간단한 리뷰 달아드렸으니 수정해보심 좋을거 같습니다!
|
|
||
| const buttonStyle = cva( | ||
| ` | ||
| text-center rounded-[2rem] text-title-md tracking-[0.015em] |
There was a problem hiding this comment.
Tracking 부분도
다 폰트 유틸리티로 넣어놔서 다시 안넣어도 될 거 같습니다!
jjangminii
left a comment
jjangminii
left a comment
There was a problem hiding this comment.
버튼 이름 오타만 해결해주시고 코멘트 하나 남겼는데 어떻게 생각하시는지 궁금합니다-!
| interface CummonButtonProps | ||
| extends React.ButtonHTMLAttributes<HTMLButtonElement>, | ||
| VariantProps<typeof buttonStyle> { | ||
| label: string; | ||
| } |
There was a problem hiding this comment.
이게 나중에 그룹으로 묶여서 radio 버튼처럼 작동할것 같은데 따로 그룹 속성은 없어도 괜찮을까요? 따로 Optional로 추가해도 좋을 것 같아요
There was a problem hiding this comment.
Optional로 확장성을 고려하는 건 좋은 방법인 것 같아요!
다만, 저는 CommonButton에서 버튼 자체는 표시나 클릭 이벤트 역할에 집중하고,
이지선다 정도가 아니라 옵션이 많다보니, 선택 상태 관리 같은 로직은 별도의 그룹 컴포넌트에서 담당하는 게
역할 분리가 더 명확하고 유지보수 면에서도 좋을 것 같습니다..!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pages/map/index.tsx (2)
7-17: 버튼 데이터 구조화 제안현재 버튼 데이터가 컴포넌트 내부에 하드코딩되어 있습니다. 재사용성과 유지보수성을 위해 다음을 고려해보세요:
- 상수 파일로 분리 (예:
src/constants/courseCategories.ts)- 명시적 TypeScript 인터페이스 정의
예시:
// src/constants/courseCategories.ts export interface CourseCategory { id: string; label: string; } export const COURSE_CATEGORIES: CourseCategory[] = [ { id: 'family', label: '가족여행' }, { id: 'friends', label: '우정여행' }, // ... ];+import { COURSE_CATEGORIES } from '@/constants/courseCategories'; import CommonButton from '@/shared/components/button/CommonButton'; import { useState } from 'react'; export default function CourseSettingPreview() { const [active, setActive] = useState<string | null>(null); - const buttons = [ - { id: 'family', label: '가족여행' }, - { id: 'friends', label: '우정여행' }, - ... - ]; return ( // ... <div className="flex flex-wrap justify-center gap-4 max-w-[400px]"> - {buttons.map(({ id, label }) => ( + {COURSE_CATEGORIES.map(({ id, label }) => ( <CommonButton
20-20: 임시 주석 제거 필요머지 전에 임시 주석을 제거해주세요.
- // 스타일은 다 수정할 거에요 버튼만 봐주세요~ // CommonButton.tsx 저것만 확인해주세요 <div className="flex flex-col items-center gap-6 py-10">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/map/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (3)
src/pages/map/index.tsx (3)
1-1: 이전 리뷰의 오타가 수정되었습니다!이전 리뷰에서 지적된
CummonButton→CommonButton오타가 올바르게 수정되었습니다.
4-5: 컴포넌트 역할 및 명명 확인 필요PR 목적과 이슈 #30에서는 "CourseButton" 공통 컴포넌트 생성을 언급하고 있으나, 이 파일의 컴포넌트명은
CourseSettingPreview입니다.이 파일은
CommonButton컴포넌트를 사용하는 프리뷰/데모 페이지로 보이며, 실제 공통 컴포넌트는src/shared/components/button/CommonButton.tsx에 있는 것으로 추정됩니다.PR 설명에 이 구조를 명확히 하시면 리뷰어가 이해하기 쉬울 것 같습니다.
25-30: 컴포넌트 사용 로직이 올바르게 수정되었습니다
CommonButton컴포넌트 사용이 올바르게 수정되었고, 토글 로직도 적절합니다. 클릭 시 활성 상태를 토글하는 동작이 정확하게 구현되었습니다.
| // 스타일은 다 수정할 거에요 버튼만 봐주세요~ // CommonButton.tsx 저것만 확인해주세요 | ||
| <div className="flex flex-col items-center gap-6 py-10"> | ||
| <h1 className="text-title-lg text-gray-700 mb-4">코스 버튼 임시 화면</h1> | ||
| <div className="flex flex-wrap justify-center gap-4 max-w-[400px]"> |
There was a problem hiding this comment.
화면 전체 너비 요구사항과 불일치
이슈 #30의 요구사항에는 "CourseButton은 화면에 꽉 차게 표시되되 스크롤 가능하도록 동작해야 함"이라고 명시되어 있으나, 현재 max-w-[400px]로 제한되어 있습니다.
요구사항에 맞게 전체 너비를 사용하도록 수정이 필요합니다.
- <div className="flex flex-wrap justify-center gap-4 max-w-[400px]">
+ <div className="flex flex-wrap justify-center gap-4 w-full px-4"> 또는 화면을 꽉 채우고 스크롤이 필요한 경우:
- <div className="flex flex-wrap justify-center gap-4 max-w-[400px]">
+ <div className="flex flex-wrap justify-center gap-4 w-full max-h-screen overflow-y-auto px-4"> 📝 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.
| <div className="flex flex-wrap justify-center gap-4 max-w-[400px]"> | |
| <div className="flex flex-wrap justify-center gap-4 w-full px-4"> |
🤖 Prompt for AI Agents
In src/pages/map/index.tsx around line 23, the container currently constrains
CourseButton width with max-w-[400px], which violates issue #30 requiring
CourseButton to fill the viewport width and be scrollable; remove the max-w
restriction and make the container use full width (e.g., w-full) and enable
scrolling as needed (horizontal or vertical) by adding an appropriate overflow
utility (e.g., overflow-auto or overflow-x-auto) so the buttons stretch to the
screen width while still allowing scroll when content exceeds viewport.
| interface CummonButtonProps | ||
| extends React.ButtonHTMLAttributes<HTMLButtonElement>, | ||
| VariantProps<typeof buttonStyle> { | ||
| label: string; | ||
| } |
There was a problem hiding this comment.
Optional로 확장성을 고려하는 건 좋은 방법인 것 같아요!
다만, 저는 CommonButton에서 버튼 자체는 표시나 클릭 이벤트 역할에 집중하고,
이지선다 정도가 아니라 옵션이 많다보니, 선택 상태 관리 같은 로직은 별도의 그룹 컴포넌트에서 담당하는 게
역할 분리가 더 명확하고 유지보수 면에서도 좋을 것 같습니다..!
KongMezu
left a comment
KongMezu
left a comment
There was a problem hiding this comment.
피드백 반영 확인했습니다! 수고하셨습니다!
3 participants
🔥 작업 내용
🤔 추후 작업 사항
🔗 이슈
PR Point (To Reviewer)
📸 피그마 스크린샷 or 기능 GIF
(작업 내역 스크린샷)
Summary by CodeRabbit