refactor: migrate and refactor checkbox component#152
refactor: migrate and refactor checkbox component#152froggy1014 merged 20 commits intorelease/v1from
Conversation
* fix(avatar): apply vanilla-extract * fix(avatar): type error
* refactor(badge): migrate module css to vanilla extract * chore: regenerate lock file * style(badge): apply color tokens * test(badge): color token update on test
* refactor(checkbox): apply vanilla-extract for styling * refactor(checkbox): update Checkbox component structure to use compound components * chore(checkbox): remove unused styles and constants * test(checkbox): update tests to reflect new component structure and props
froggy1014
requested review from
SEMIN-97,
bae-sh,
developerjhp,
heeji289,
hy57in,
jiji-hoon96,
kimdaeyeob,
noahluftyang,
synuns and
yeojini
as code owners
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Walkthrough이 변경 사항은 주요 UI 컴포넌트(Avatar, Badge, Card, Checkbox 등)에 대해 기존 CSS 모듈 기반 스타일링을 제거하고, vanilla-extract를 활용한 타입 안전 CSS-in-TypeScript 스타일링으로 전환합니다. 각 컴포넌트의 스타일 파일이 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CheckboxRoot
participant CheckboxInput
participant CheckboxLabel
participant CheckboxContext
User->>CheckboxRoot: 렌더링(checked, indeterminate 등 props)
CheckboxRoot->>CheckboxContext: 컨텍스트 값 제공
CheckboxRoot->>CheckboxInput: 자식으로 전달
CheckboxInput->>CheckboxContext: 컨텍스트 값 소비
CheckboxInput->>CheckboxInput: 상태/props 병합, 스타일 적용
CheckboxInput->>User: onChange 발생 시 콜백 호출
CheckboxRoot->>CheckboxLabel: 자식으로 전달
CheckboxLabel->>CheckboxContext: 컨텍스트 값 소비
CheckboxLabel->>CheckboxInput: label 연결(for/id)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
package.json (1)
1-78:⚠️ Potential issue락파일 업데이트 필요
파이프라인 실패 로그를 보면 packages/input/package.json과 pnpm-lock.yaml 간의 불일치가 있습니다. 이로 인해 CI가 실패하고 있습니다.
다음을 실행하여 락파일을 업데이트해야 합니다:
- 현재 상태: 락파일이 packages/input/package.json과 동기화되지 않음 + 실행 필요: pnpm install🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: pnpm install failed due to outdated lockfile. The pnpm-lock.yaml is not up to date with packages/input/package.json. Specifiers in the lockfile do not match the package.json dependencies.
🪛 GitHub Actions: Run tests and upload coverage
[error] 1-1: pnpm install failed due to outdated lockfile. The pnpm-lock.yaml is not up to date with packages/input/package.json. Specifiers in the lockfile do not match the package.json dependencies.
🧹 Nitpick comments (10)
packages/badge/src/Badge.test.tsx (1)
38-43: 하드코딩된 색상 값 남아있음weak variant 테스트에서는 여전히 하드코딩된 색상 값('#e4e4e7')을 사용하고 있습니다. 다른 테스트와의 일관성을 위해 이 값도 토큰 시스템을 활용하여 변경하는 것이 좋겠습니다.
다음과 같이 변경을 제안합니다:
-test('variant가 weak인 경우 배경색 gray200로 형태를 적용한다.', () => { +test(`variant가 weak인 경우 배경색 gray200(${colorToken.gray200})로 형태를 적용한다.`, () => { render(<Badge variant="weak">테스트</Badge>); expect(screen.getByRole('status')).toHaveStyle({ - backgroundColor: '#e4e4e7', + backgroundColor: colorToken.gray200, }); });packages/avatar/src/Avatar.css.ts (2)
12-33: 크기 변형이 체계적으로 정의되었습니다.styleVariants를 사용하여 각 크기별 스타일을 체계적으로 정의한 점이 좋습니다. 열거형 키를 동적으로 활용하여 타입 안전성과 유지보수성을 높인 접근법이 효과적입니다.
단, 픽셀 단위 대신 rem 또는 em 단위를 사용하면 접근성과 반응형 디자인 측면에서 더 좋을 수 있습니다. 향후 버전에서 고려해보세요.
53-57: 폴백 스타일이 적절하게 정의되었습니다.이미지가 없을 때 표시되는 텍스트를 위한 폴백 스타일이 잘 정의되어 있습니다. 텍스트 색상, 크기, 정렬이 적절하게 지정되었습니다.
단, 폰트 크기를 고정 값(0.8rem) 대신 크기에 따라 다르게 적용하는 방식을 고려해보면 더 나은 사용자 경험을 제공할 수 있을 것입니다.
packages/badge/src/Badge.tsx (2)
14-22: forwardRef 결과에displayName을 지정하면 디버깅이 쉬워집니다개발자 도구에서 컴포넌트 이름이
ForwardRef로만 표시되어 추적이 어렵습니다. 컴포넌트 아래에 한 줄 추가해 주세요.export const Badge = forwardRef(function Badge( { className, children, size = 'medium', variant = 'filled', ...props }: BadgeProps, ref: ForwardedRef<HTMLDivElement>, ) { ... }); + +Badge.displayName = 'Badge';
32-41:getTypographySize하드코딩 대신 토큰/상수화 권장폰트 크기를 숫자 리터럴로 직접 반환하면 다른 디자인 시스템 값 변경 시 동기화가 어려워집니다.
@sipe-team/tokens에 typography 크기 토큰이 있다면 이를 참조하도록 리팩터링을 고려해 주세요.
예시:import { fontSize } from '@sipe-team/tokens'; function getTypographySize(size: BadgeSize): keyof typeof fontSize { switch (size) { case 'small': return fontSize.xs; ...packages/checkbox/src/Checkbox.test.tsx (2)
7-14: 도우미 컴포넌트를 파일 최상단에서 한 번만 선언해 재정의 비용을 줄이세요
RenderBasicCheckbox가 각 테스트 실행 시 새로 선언되면 테스트 러너가 불필요한 함수 객체를 계속 생성합니다.
파일 최상단(모듈 스코프)에 선언해 한 번만 생성하도록 이동하면 미세하지만 성능과 가독성이 향상됩니다.
149-178: 폼 제출 테스트에서fireEvent대신userEvent사용을 고려해 보세요
userEvent는 실제 브라우저 상호작용에 더 가깝게 이벤트 시퀀스를 발생시켜 신뢰도가 높습니다. 이미 의존성을 추가하셨으므로 버튼 클릭도 아래처럼 통일하는 편이 좋습니다.-const submitButton = screen.getByText('Submit'); -fireEvent.click(submitButton); +const user = userEvent.setup(); +await user.click(screen.getByText('Submit'));packages/checkbox/src/Checkbox.css.ts (1)
74-77: disabled 변형이 실제 효과가 없음
container레시피의disabled변형이 비어 있어 UI 변화를 주지 않습니다. 최소한 포인터 이벤트 차단 또는 불투명도 감소 정도를 넣어두면 시각적으로 비활성 상태임을 알릴 수 있습니다.packages/checkbox/src/Checkbox.tsx (2)
128-129: ref 병합 로직이 한쪽 ref를 덮어쓸 수 있습니다
ref || contextRef방식은 두 ref 가 동시에 전달되면 사용자 ref 가 무시될 수 있습니다. 위 diff 처럼 콜백을 이용한 병합을 권장합니다.
117-121: 오류 처리 방식 개선 제안
handleChange내부에서console.error만 호출하면 테스트 환경이나 상위 에러 바운더리에서 잡히지 않습니다. 예외를 다시 throw 하거나, 로그 라이브러리로 남기는 쪽이 좋습니다.
🛑 Comments failed to post (4)
packages/card/src/Card.css.ts (1)
20-28: 🛠️ Refactor suggestion
box-sizing 설정이 빠져 있어 레이아웃이 의도치 않게 흐트러질 수 있습니다
padding과border를 가진 컴포넌트는 컨테이너 폭 계산 시box-sizing: border-box가 설정돼 있지 않으면 부모 레이아웃이 틀어질 가능성이 있습니다. 기본 스타일에 아래 속성을 추가하는 것을 권장합니다.base: { display: 'flex', justifyContent: 'center', alignItems: 'center', borderRadius: '12px', padding: '20px', + boxSizing: 'border-box', },packages/checkbox/src/Checkbox.css.ts (1)
88-97: 🛠️ Refactor suggestion
포커스 스타일과 Safari 대응이 누락되었습니다
appearance: 'none'으로 기본 브라우저 스타일을 제거했지만, 포커스 링을 별도로 넣지 않아 키보드 접근성이 사라집니다. 또한 Safari에서는-webkit-appearance: none을 같이 지정하지 않으면 원형 체크박스가 남습니다.base: { appearance: 'none', + /* Safari */ + WebkitAppearance: 'none', + /* 접근성: focus-visible 처리 */ + selectors: { + '&:focus-visible': { + outline: '2px solid #2563eb', // 또는 토큰 사용 + outlineOffset: '2px', + }, + }, border: `${CHECKBOX_STYLE.borderWidth}px solid ${CHECKBOX_STYLE.borderColor}`,📝 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.base: { appearance: 'none', /* Safari */ WebkitAppearance: 'none', /* 접근성: focus-visible 처리 */ selectors: { '&:focus-visible': { outline: '2px solid #2563eb', // 또는 토큰 사용 outlineOffset: '2px', }, }, border: `${CHECKBOX_STYLE.borderWidth}px solid ${CHECKBOX_STYLE.borderColor}`, backgroundColor: CHECKBOX_STYLE.backgroundColor, backgroundSize: CHECKBOX_STYLE.backgroundSize, backgroundPosition: CHECKBOX_STYLE.backgroundPosition, backgroundRepeat: CHECKBOX_STYLE.backgroundRepeat, transition: CHECKBOX_STYLE.transition, cursor: 'pointer', }, variants: {packages/checkbox/src/Checkbox.tsx (1)
124-137: 🛠️ Refactor suggestion
indeterminate DOM 프로퍼티 동기화 필요
ARIA 속성만으로 시멘틱 정보는 전달되지만, 실제 DOMHTMLInputElement.indeterminate프로퍼티를 설정하지 않아 브라우저 기본 동작(예: 폼 리셋 시 상태 보존)이 보장되지 않습니다.+ const inputRef = useRef<HTMLInputElement>(null); + return ( <input {...rootProps} {...props} id={id} - ref={ref || contextRef} + ref={(node) => { + inputRef.current = node; + if (typeof (ref || contextRef) === 'function') (ref || contextRef)?.(node); + else if (ref || contextRef) (ref || contextRef).current = node; + }} type="checkbox" checked={checked} onChange={handleChange} disabled={disabled} aria-checked={indeterminate ? 'mixed' : checked} className={clsx(input({ size, checked, disabled, indeterminate }), className)} name={name || contextName} value={value || contextValue || 'on'} /> );그리고
useEffect로inputRef.current!.indeterminate = indeterminate를 동기화해 주세요.📝 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.import { useRef, useEffect } from 'react'; // … other imports function Checkbox({ /* props */ }) { // … other hooks/logic const inputRef = useRef<HTMLInputElement>(null); useEffect(() => { if (inputRef.current) { inputRef.current.indeterminate = indeterminate; } }, [indeterminate]); return ( <input {...rootProps} {...props} id={id} ref={(node) => { inputRef.current = node; if (typeof (ref || contextRef) === 'function') { (ref || contextRef)(node); } else if (ref || contextRef) { (ref || contextRef).current = node; } }} type="checkbox" checked={checked} onChange={handleChange} disabled={disabled} aria-checked={indeterminate ? 'mixed' : checked} className={clsx(input({ size, checked, disabled, indeterminate }), className)} name={name || contextName} value={value || contextValue || 'on'} /> ); } export default Checkbox;packages/checkbox/src/Checkbox.stories.tsx (1)
138-146:
⚠️ Potential issue
key충돌로 React 경고가 발생할 수 있습니다
{checkedItems.map((item, index) => ( ... key={${item}-newDate} ... ))}
item값이 중복(boolean)이라 키가 동일해집니다. 인덱스나 고유 ID 를 사용하세요.- key={`${item}-newDate`} + key={`option-${index}`}
| size: CheckboxSize; | ||
| onCheckChange?: (checked: boolean) => void; | ||
| indeterminate?: boolean; | ||
| ref?: Ref<HTMLInputElement> | undefined; |
There was a problem hiding this comment.
이 prop은 CheckBoxRootBaseProps에 포함되어있지 않아?
There was a problem hiding this comment.
CheckBoxRootBaseProps 에는 포함되어 있어 있진 않지만 넣는게 좋은거 같아 넣었습니다 ㅎㅎㅎ.
packages/checkbox/src/Checkbox.tsx
Outdated
| ); | ||
|
|
||
| interface CheckboxLabelProps extends LabelHTMLAttributes<HTMLLabelElement> { | ||
| children: ReactNode; |
There was a problem hiding this comment.
children도 LabelHTMLAttributes에 포함되어있을거야~
There was a problem hiding this comment.
HTMLAttributes에는 포함되어 있지는 않더군요. ComponentProp으로 다 대체했습니다.
2 participants
Changes
기존 CSS 모듈 기반 Checkbox 컴포넌트를 vanilla-extract로 마이그레이션하고 컴파운드 컴포넌트 패턴을 적용하여 리팩토링했습니다.
기존에는 각 요소마다 스타일 변화가 힘들었고
레이아웃 변경 및 요소 추가 또한 불가능했습니다.
Visuals
Checklist
Additional Discussion Points
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation