#21 refactor: Button 컴포넌트 리팩터링 (tailwind-variants 적용) #22
Conversation
📝 WalkthroughWalkthroughThis PR refactors the Button component from a theme-based prop system ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/components/admin/assignments/AssignmentFormLayout.tsx (1)
27-32: Consider specifying the content variant for consistency.These Button components don't specify the
contentprop, relying on default behavior. For consistency with other Button usages in the codebase (e.g.,AssignmentCreatePage.tsxline 66), consider explicitly settingcontent='text'since these buttons contain only text children.🔎 Suggested enhancement for consistency
- <Button color='outlinePurple' onClick={onCancel}> + <Button color='outlinePurple' content='text' onClick={onCancel}> 취소 </Button> - <Button color='primary' onClick={onConfirm}> + <Button color='primary' content='text' onClick={onConfirm}> 저장 </Button>src/components/admin/assignments/AssignmentPageLayout.tsx (1)
41-42: Consider specifying size and content variants for consistency.Similar to other files, these Buttons omit the
sizeandcontentprops. Consider addingcontent='text'for consistency with the variant-based API.🔎 Suggested enhancement
- <Button color='outlinePurple'>취소</Button> - <Button color='primary'>등록</Button> + <Button color='outlinePurple' content='text'>취소</Button> + <Button color='primary' content='text'>등록</Button>src/components/common/CourseOverview/CourseActionsBar.tsx (1)
6-7: Consider adding content variant for consistency.These text-only Buttons should explicitly specify
content='text'to align with the variant-driven API used throughout the codebase.🔎 Suggested enhancement
- <Button color='outlineWhite'>학생 목록</Button> - <Button color='outlinePurple'>단원 추가</Button> + <Button color='outlineWhite' content='text'>학생 목록</Button> + <Button color='outlinePurple' content='text'>단원 추가</Button>src/pages/common/LandingPage.tsx (1)
93-98: Consider using the Button component for consistency.The Google login button uses a native
<button>with custom styling. For consistency across the codebase, consider using the refactoredButtoncomponent with appropriate variants (e.g.,color='ghost',size='none') and passing the flex/gap styles viaclassName.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/components/admin/assignments/AssignmentFormLayout.tsxsrc/components/admin/assignments/AssignmentPageLayout.tsxsrc/components/common/Button.tsxsrc/components/common/CourseOverview/CourseActionsBar.tsxsrc/components/common/Dashboard/CourseList.tsxsrc/layout/Layout.tsxsrc/pages/admin/assignments/AssignmentCreatePage.tsxsrc/pages/common/LandingPage.tsxsrc/pages/common/UserIdInputPage.tsx
🔇 Additional comments (8)
src/components/admin/assignments/AssignmentPageLayout.tsx (1)
35-38: LGTM - Proper usage of mixed content variant.The Button correctly specifies
content='mixed'for icon + text combination, which aligns with the new variant API.src/pages/common/UserIdInputPage.tsx (1)
125-132: LGTM - Proper disabled state handling.The Button correctly uses the
disabledprop with appropriate className for cursor styling. Thecolor='secondary'andsize='wide'variants are clearly specified.src/layout/Layout.tsx (1)
45-51: LGTM - Excellent accessibility practice.The Button correctly specifies
content='icon'andsize='icon'for icon-only buttons, and properly maintainsaria-labelfor accessibility. The icon is correctly passed as a child.src/pages/admin/assignments/AssignmentCreatePage.tsx (1)
63-70: All specified Button variants are properly defined in the component. Thecolor='tonal',size='compact', andcontent='mixed'variants are present in Button.tsx, and the usage in AssignmentCreatePage.tsx is correct.package.json (1)
18-18: tailwind-variants ^3.2.2 is compatible and secure.Verified: v3.2.2 is built for Tailwind CSS v4.x and compatible with v4.1.11. No security advisories found. This version is the latest stable release.
src/components/common/Button.tsx (1)
3-35: LGTM!The variant configuration is well-structured with clear separation of concerns across color, size, and content axes. Default variants are appropriately set.
src/pages/common/LandingPage.tsx (2)
73-88: Hover handlers won't work until the Button component is fixed.The
onMouseEnterandonMouseLeavehandlers passed here are not applied to the underlying<button>element due to the bug inButton.tsx. Once that fix is applied, this code will function correctly.
42-48: LGTM!The ghost button usage is appropriate for this navigation action. The
classNameoverride for custom leading/text size will work once the Button component properly applies the className prop.
| const Button = ({ | ||
| children, | ||
| onClick, | ||
| type = 'button', | ||
| disabled = false, | ||
| ...props | ||
| }: ButtonProps) => { | ||
| return ( | ||
| <button | ||
| className={`py-1.5 px-3 rounded-[10px] cursor-pointer ${buttonTheme[theme]}`}> | ||
| {icon} | ||
| <span className='text-btn'>{text}</span> | ||
| <button onClick={onClick} disabled={disabled} className={button(props)}> | ||
| {children} | ||
| </button> | ||
| ); |
There was a problem hiding this comment.
Missing type, onMouseEnter, and onMouseLeave attributes on the button element.
The type prop is destructured but never applied to the <button>. Additionally, onMouseEnter and onMouseLeave are accepted as props but remain in the ...props spread—they're passed to the button() tv function (which ignores them) instead of being wired to the actual element. This breaks hover interactions in consumers like LandingPage.
🔎 Proposed fix
const Button = ({
children,
onClick,
type = 'button',
disabled = false,
+ onMouseEnter,
+ onMouseLeave,
+ className,
...props
}: ButtonProps) => {
return (
- <button onClick={onClick} disabled={disabled} className={button(props)}>
+ <button
+ type={type}
+ onClick={onClick}
+ disabled={disabled}
+ onMouseEnter={onMouseEnter}
+ onMouseLeave={onMouseLeave}
+ className={button({...props, className})}>
{children}
</button>
);
};📝 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.
| const Button = ({ | |
| children, | |
| onClick, | |
| type = 'button', | |
| disabled = false, | |
| ...props | |
| }: ButtonProps) => { | |
| return ( | |
| <button | |
| className={`py-1.5 px-3 rounded-[10px] cursor-pointer ${buttonTheme[theme]}`}> | |
| {icon} | |
| <span className='text-btn'>{text}</span> | |
| <button onClick={onClick} disabled={disabled} className={button(props)}> | |
| {children} | |
| </button> | |
| ); | |
| const Button = ({ | |
| children, | |
| onClick, | |
| type = 'button', | |
| disabled = false, | |
| onMouseEnter, | |
| onMouseLeave, | |
| className, | |
| ...props | |
| }: ButtonProps) => { | |
| return ( | |
| <button | |
| type={type} | |
| onClick={onClick} | |
| disabled={disabled} | |
| onMouseEnter={onMouseEnter} | |
| onMouseLeave={onMouseLeave} | |
| className={button({...props, className})}> | |
| {children} | |
| </button> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
In src/components/common/Button.tsx around lines 49 to 60, the destructured type
prop is not applied to the <button> and the onMouseEnter/onMouseLeave props are
swallowed by the props spread (passed only to the style function), so hover
callbacks never reach the DOM element; fix by adding type={type} on the
<button>, explicitly pass onMouseEnter={onMouseEnter} and
onMouseLeave={onMouseLeave} to the element (keep the remaining ...props spread
for other attributes), and ensure the button() tv function only receives the
props it needs (or pass a copy that excludes event handlers).
| <Button | ||
| color='ghostWhite' | ||
| size='compact' | ||
| className='hover:opacity-70'> | ||
| <AddIcon width={12} height={12} /> | ||
| <span>추가</span> | ||
| </button> | ||
| 추가 | ||
| </Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add content='mixed' variant for icon + text combination.
This Button contains both an icon and text but doesn't specify content='mixed'. This is inconsistent with similar usage in AssignmentCreatePage.tsx (line 66) and AssignmentPageLayout.tsx (line 35).
🔎 Proposed fix
<Button
color='ghostWhite'
size='compact'
+ content='mixed'
className='hover:opacity-70'>
<AddIcon width={12} height={12} />
추가
</Button>📝 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.
| <Button | |
| color='ghostWhite' | |
| size='compact' | |
| className='hover:opacity-70'> | |
| <AddIcon width={12} height={12} /> | |
| <span>추가</span> | |
| </button> | |
| 추가 | |
| </Button> | |
| <Button | |
| color='ghostWhite' | |
| size='compact' | |
| content='mixed' | |
| className='hover:opacity-70'> | |
| <AddIcon width={12} height={12} /> | |
| 추가 | |
| </Button> |
🤖 Prompt for AI Agents
In src/components/common/Dashboard/CourseList.tsx around lines 49 to 55, the
Button contains both an icon and text but is missing the content='mixed' prop;
update the Button declaration to include content='mixed' so it matches similar
usages (e.g., AssignmentCreatePage.tsx line 66 and AssignmentPageLayout.tsx line
35) to ensure consistent styling/behavior for icon+text buttons.
suminb99
changed the title
⚙️ Related ISSUE Number
Related #21
📄 Work Description
tailwind-variants를 활용하여 공통Button컴포넌트를 리펙토링 했습니다!작업한 브랜치 기준
ActionsButton,IconButton와 기존Button컴포넌트를 사용하던 부분에 리펙토링 된Button컴포넌트 모두 적용했습니다.버튼의 역할과 구성을 명확히 하기 위해 Variant를
color,size,content3가지 기준으로 분리했습니다.color- 버튼 색상 / 스타일size- 버튼 크기 및 형태content- 버튼 내부 구성 방식contentvariant는 버튼의children구성에 따라서 적용되는flex,gap,text관련 스타일이 달라지기 때문에base속성에 모든 스타일을 넣는 것을 피하기 위해 추가했습니다!default variants는
color: 'primary',size: 'default',content: 'text'로 설정했고className을 통해 추가 스타일 확장 가능해요1. 텍스트 버튼
2. 아이콘 + 텍스트 버튼
3. 아이콘 버튼
📷 Screenshot
💬 To Reviewers
🔗 Reference
https://www.tailwind-variants.org/docs/introduction
Summary by CodeRabbit
Release Notes
Style
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.