feat(checkbox): Checkbox 컴포넌트 구현 #106
Conversation
- Add checkbox dependencies - Fix checkbox element positioning
- Ensure compatibility with Radix UI's CheckedState type - Enhance type safety with exactOptionalPropertyTypes option
jiji-hoon96
requested review from
kimdaeyeobbb,
hy57in,
developerjhp,
bae-sh,
noahluftyang,
froggy1014,
heeji289,
synuns,
yeojini and
SEMIN-97
as code owners
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/checkbox/.storybook/preview.tsOops! Something went wrong! :( ESLint: 9.17.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by 개요Walkthrough이 풀 리퀘스트는 Changes
Poem
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
packages/checkbox/.storybook/preview.ts (1)
4-14: 접근성 매개변수 추가를 고려해보세요.현재 설정은 기본적인 기능을 잘 커버하고 있습니다만, 접근성 테스트를 위한 매개변수를 추가하면 좋을 것 같습니다.
다음과 같이 a11y 매개변수를 추가하는 것을 추천드립니다:
const preview: Preview = { parameters: { actions: { argTypesRegex: '^on[A-Z].*' }, controls: { matchers: { color: /(background|color)$/i, date: /Date$/i, }, }, + a11y: { + element: '#storybook-root', + config: { + rules: [ + { + id: 'color-contrast', + enabled: true + } + ] + } + } }, };packages/checkbox/src/Checkbox.stories.tsx (1)
87-102: CheckboxGroup 스토리의 레이아웃 개선이 필요합니다.현재 체크박스들이 수직으로 쌓이는 레이아웃이 없어 가독성이 떨어집니다.
다음과 같이 수정해보세요:
return ( - <CheckboxGroup value={selected} onChange={setSelected}> + <CheckboxGroup + value={selected} + onChange={setSelected} + style={{ display: 'flex', flexDirection: 'column', gap: '8px' }} + > <Checkbox {...args} value="apple" label="사과" /> <Checkbox {...args} value="banana" label="바나나" /> <Checkbox {...args} value="orange" label="오렌지" /> </CheckboxGroup> );packages/checkbox/src/Checkbox.test.tsx (3)
7-16: 체크박스 상태 변경 테스트 보완 제안테스트가 잘 구현되어 있지만, 다음과 같은 추가 검증을 고려해보세요:
data-state속성 검증- 접근성 속성(aria-checked) 검증
test('체크박스를 클릭하면 상태가 변경된다.', async () => { const user = userEvent.setup(); render(<Checkbox label="테스트" />); const checkbox = screen.getByRole('checkbox', { name: '테스트' }); expect(checkbox).not.toBeChecked(); + expect(checkbox).toHaveAttribute('data-state', 'unchecked'); + expect(checkbox).toHaveAttribute('aria-checked', 'false'); await user.click(checkbox); expect(checkbox).toBeChecked(); + expect(checkbox).toHaveAttribute('data-state', 'checked'); + expect(checkbox).toHaveAttribute('aria-checked', 'true'); });
18-27: 비활성화된 체크박스 테스트 케이스 보완 제안현재 테스트는 기본적인 동작을 검증하고 있지만, 다음과 같은 추가 테스트 케이스를 고려해보세요:
- 스타일 상태 검증 (opacity)
- 키보드 이벤트 처리 검증
test('disabled 상태의 체크박스는 클릭할 수 없다.', async () => { const user = userEvent.setup(); render(<Checkbox disabled label="테스트" />); const checkbox = screen.getByRole('checkbox', { name: '테스트' }); expect(checkbox).toBeDisabled(); + expect(checkbox).toHaveStyle({ opacity: '0.4' }); await user.click(checkbox); expect(checkbox).not.toBeChecked(); + + await user.keyboard('[Space]'); + expect(checkbox).not.toBeChecked(); });
47-77: 체크박스 그룹 에러 케이스 테스트 추가 제안현재 테스트는 정상적인 사용 사례만 다루고 있습니다. 다음과 같은 에러 케이스도 테스트하면 좋을 것 같습니다:
- 잘못된 value 처리
- 중복된 value 처리
- 빈 자식 요소 처리
다음과 같은 테스트 케이스를 추가해보세요:
test('체크박스 그룹에서 잘못된 value를 처리할 수 있다.', () => { render( <CheckboxGroup value={['invalid']}> <Checkbox value="1" label="항목 1" /> </CheckboxGroup> ); const checkbox = screen.getByRole('checkbox', { name: '항목 1' }); expect(checkbox).not.toBeChecked(); });packages/checkbox/src/Checkbox.module.css (1)
15-26: 호버 및 포커스 상태 스타일 개선 제안체크박스의 상호작용 상태에 대한 시각적 피드백을 개선하면 좋을 것 같습니다:
- 호버 상태 스타일
- 포커스 상태 스타일
- 키보드 포커스 링
.checkbox { all: unset; width: var(--size); height: var(--size); border-radius: 4px; border: 2px solid var(--border-color); background-color: var(--background-color); display: flex; align-items: center; justify-content: center; cursor: pointer; + transition: all 0.2s ease-in-out; +} + +.checkbox:hover { + border-color: var(--checked-border-color); +} + +.checkbox:focus-visible { + outline: 2px solid var(--checked-border-color); + outline-offset: 2px; }packages/checkbox/package.json (1)
13-22: 빌드 및 테스트 스크립트 개선 제안다음과 같은 스크립트 및 구성을 추가하면 좋을 것 같습니다:
- 테스트 커버리지 리포트
- 번들 크기 분석
- 성능 테스트
"scripts": { "build": "tsup", "build:storybook": "storybook build", "dev:storybook": "storybook dev -p 6006", "lint:biome": "pnpm exec biome lint", "lint:eslint": "pnpm exec eslint --flag unstable_ts_config", "test": "vitest", + "test:coverage": "vitest run --coverage", + "analyze": "tsup --analyze", + "perf": "vitest bench", "typecheck": "tsc", "prepack": "pnpm run build" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/checkbox/.storybook/main.ts(1 hunks)packages/checkbox/.storybook/preview.ts(1 hunks)packages/checkbox/global.d.ts(1 hunks)packages/checkbox/package.json(1 hunks)packages/checkbox/src/Checkbox.module.css(1 hunks)packages/checkbox/src/Checkbox.stories.tsx(1 hunks)packages/checkbox/src/Checkbox.test.tsx(1 hunks)packages/checkbox/src/Checkbox.tsx(1 hunks)packages/checkbox/src/index.ts(1 hunks)packages/checkbox/tsconfig.json(1 hunks)packages/checkbox/tsup.config.ts(1 hunks)packages/checkbox/vitest.config.ts(1 hunks)packages/checkbox/vitest.setup.ts(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- packages/checkbox/src/index.ts
- packages/checkbox/global.d.ts
- packages/checkbox/tsup.config.ts
- packages/checkbox/vitest.setup.ts
- packages/checkbox/tsconfig.json
🔇 Additional comments (2)
packages/checkbox/vitest.config.ts (1)
1-11: 테스트 설정이 적절히 구성되었습니다!기본 설정을 상속받아 프로젝트별 설정을 추가하는 방식으로 잘 구성되어 있습니다.
packages/checkbox/src/Checkbox.stories.tsx (1)
70-85: Controlled 컴포넌트의 타입 안전성 개선이 필요합니다.
checked상태의 타입이boolean | 'indeterminate'로 되어있는데, 이는 컴포넌트의 실제 타입과 일치하는지 확인이 필요합니다.다음 스크립트로 Checkbox 컴포넌트의 타입을 확인해보세요:
✅ Verification successful
타입 안전성이 올바르게 구현되어 있습니다.
checked상태의 타입boolean | 'indeterminate'는 Radix UI의CheckboxPrimitive.CheckedState타입과 일치하며, 컴포넌트가 이를 적절히 처리하고 있습니다.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checkbox 컴포넌트의 checked prop 타입 확인 ast-grep --pattern 'interface CheckboxProps { $$$ checked?: $type $$$ }'Length of output: 80
Script:
#!/bin/bash # Try different approaches to find the type definition # Search for type definitions in Checkbox component file echo "=== Searching in Checkbox component file ===" fd "Checkbox.tsx" --exec cat {} # Search for any type definitions containing 'Checkbox' echo -e "\n=== Searching for type files ===" fd -e ts -e tsx -g "*types*" --exec grep -l "Checkbox" {} # Search for checked prop usage echo -e "\n=== Searching for checked prop usage ===" rg "checked.*?:" -A 2 -g "*.{ts,tsx}"Length of output: 7277
| addons: [ | ||
| '@storybook/addon-onboarding', | ||
| '@storybook/addon-links', | ||
| '@storybook/addon-essentials', | ||
| '@storybook/addon-interactions', | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 애드온 추가가 필요합니다.
체크박스는 접근성이 매우 중요한 컴포넌트입니다. 접근성 테스트를 위한 애드온을 추가하면 좋을 것 같습니다.
다음과 같이 a11y 애드온을 추가해주세요:
addons: [
'@storybook/addon-onboarding',
'@storybook/addon-links',
'@storybook/addon-essentials',
'@storybook/addon-interactions',
+ '@storybook/addon-a11y',
],📝 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.
| addons: [ | |
| '@storybook/addon-onboarding', | |
| '@storybook/addon-links', | |
| '@storybook/addon-essentials', | |
| '@storybook/addon-interactions', | |
| ], | |
| addons: [ | |
| '@storybook/addon-onboarding', | |
| '@storybook/addon-links', | |
| '@storybook/addon-essentials', | |
| '@storybook/addon-interactions', | |
| '@storybook/addon-a11y', | |
| ], |
| export const States: Story = { | ||
| args: { | ||
| label: '체크박스', | ||
| }, | ||
| render: (args) => ( | ||
| <div style={{ display: 'flex', flexDirection: 'column', gap: '16px' }}> | ||
| <Checkbox {...args} label="Unchecked (Default)" /> | ||
| <Checkbox {...args} label="Checked" defaultChecked /> | ||
| <Checkbox {...args} label="Disabled" disabled /> | ||
| <Checkbox {...args} label="Disabled & Checked" disabled defaultChecked /> | ||
| </div> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
키보드 상호작용 예제가 필요합니다.
접근성을 위해 키보드 상호작용을 보여주는 스토리를 추가하면 좋을 것 같습니다.
다음과 같은 키보드 인터랙션 스토리를 추가해보세요:
export const KeyboardInteraction: Story = {
args: {
label: '키보드 상호작용',
},
parameters: {
docs: {
description: {
story: '스페이스바 또는 엔터 키를 사용하여 체크박스를 토글할 수 있습니다.',
},
},
},
play: async ({ canvasElement }) => {
const canvas = within(canvasElement);
const checkbox = canvas.getByRole('checkbox');
// 포커스
await userEvent.tab();
// 토글
await userEvent.keyboard('[Space]');
},
};| argTypes: { | ||
| size: { | ||
| description: '체크박스의 크기를 지정합니다', | ||
| options: ['sm', 'md', 'lg'], | ||
| control: { type: 'radio' }, | ||
| }, | ||
| disabled: { | ||
| description: '체크박스의 비활성화 상태를 지정합니다', | ||
| control: { type: 'boolean' }, | ||
| }, | ||
| checked: { | ||
| description: '체크박스의 선택 상태를 지정합니다', | ||
| control: { type: 'boolean' }, | ||
| }, | ||
| label: { | ||
| description: '체크박스의 레이블을 지정합니다', | ||
| control: 'text', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 속성에 대한 설명이 필요합니다.
argTypes에 aria-label과 같은 접근성 속성에 대한 설명이 누락되어 있습니다.
다음과 같이 접근성 관련 argTypes를 추가해주세요:
argTypes: {
+ 'aria-label': {
+ description: '스크린 리더를 위한 레이블을 지정합니다',
+ control: 'text',
+ },
+ 'aria-describedby': {
+ description: '스크린 리더를 위한 추가 설명을 지정합니다',
+ control: 'text',
+ },
size: {📝 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.
| argTypes: { | |
| size: { | |
| description: '체크박스의 크기를 지정합니다', | |
| options: ['sm', 'md', 'lg'], | |
| control: { type: 'radio' }, | |
| }, | |
| disabled: { | |
| description: '체크박스의 비활성화 상태를 지정합니다', | |
| control: { type: 'boolean' }, | |
| }, | |
| checked: { | |
| description: '체크박스의 선택 상태를 지정합니다', | |
| control: { type: 'boolean' }, | |
| }, | |
| label: { | |
| description: '체크박스의 레이블을 지정합니다', | |
| control: 'text', | |
| }, | |
| }, | |
| argTypes: { | |
| 'aria-label': { | |
| description: '스크린 리더를 위한 레이블을 지정합니다', | |
| control: 'text', | |
| }, | |
| 'aria-describedby': { | |
| description: '스크린 리더를 위한 추가 설명을 지정합니다', | |
| control: 'text', | |
| }, | |
| size: { | |
| description: '체크박스의 크기를 지정합니다', | |
| options: ['sm', 'md', 'lg'], | |
| control: { type: 'radio' }, | |
| }, | |
| disabled: { | |
| description: '체크박스의 비활성화 상태를 지정합니다', | |
| control: { type: 'boolean' }, | |
| }, | |
| checked: { | |
| description: '체크박스의 선택 상태를 지정합니다', | |
| control: { type: 'boolean' }, | |
| }, | |
| label: { | |
| description: '체크박스의 레이블을 지정합니다', | |
| control: 'text', | |
| }, | |
| }, |
packages/checkbox/src/Checkbox.tsx
Outdated
| export const Checkbox = forwardRef< | ||
| ElementRef<typeof CheckboxPrimitive.Root>, | ||
| CheckboxProps | ||
| >(({ className, size = 'md', label, style, id: providedId, ...props }, ref) => { | ||
| const generatedId = useId(); | ||
| const id = providedId || generatedId; | ||
| const { checkboxSize, indicatorSize, fontSize, padding } = getSizeStyles(size); | ||
|
|
||
| return ( | ||
| <div | ||
| className={styles.root} | ||
| style={ | ||
| { | ||
| '--margin': '4px', | ||
| '--padding': padding, | ||
| '--size': checkboxSize, | ||
| '--indicator-size': indicatorSize, | ||
| '--border-color': color.gray400, | ||
| '--background-color': color.white, | ||
| '--checked-border-color': color.cyan300, | ||
| '--checked-background-color': color.cyan300, | ||
| '--indicator-color': color.white, | ||
| ...style, | ||
| } as React.CSSProperties | ||
| } | ||
| > | ||
| <CheckboxPrimitive.Root | ||
| className={styles.checkbox} | ||
| ref={ref} | ||
| id={id} | ||
| {...props} | ||
| > | ||
| <CheckboxPrimitive.Indicator className={styles.indicator}> | ||
| <Check size={indicatorSize} /> | ||
| </CheckboxPrimitive.Indicator> | ||
| </CheckboxPrimitive.Root> | ||
| {label && ( | ||
| <Typography asChild size={fontSize}> | ||
| <label htmlFor={id} className={styles.label}> | ||
| {label} | ||
| </label> | ||
| </Typography> | ||
| )} | ||
| </div> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성 개선 제안
현재 구현에 다음과 같은 접근성 기능을 추가하면 좋을 것 같습니다:
- aria-describedby 지원
- aria-required 지원
- 키보드 포커스 스타일
export const Checkbox = forwardRef<
ElementRef<typeof CheckboxPrimitive.Root>,
CheckboxProps
->(({ className, size = 'md', label, style, id: providedId, ...props }, ref) => {
+>(({ className, size = 'md', label, style, id: providedId, required, description, ...props }, ref) => {
const generatedId = useId();
const id = providedId || generatedId;
+ const descriptionId = description ? `${id}-description` : undefined;
const { checkboxSize, indicatorSize, fontSize, padding } = getSizeStyles(size);
return (
<div
className={styles.root}
style={
{
'--margin': '4px',
'--padding': padding,
'--size': checkboxSize,
'--indicator-size': indicatorSize,
'--border-color': color.gray400,
'--background-color': color.white,
'--checked-border-color': color.cyan300,
'--checked-background-color': color.cyan300,
'--indicator-color': color.white,
...style,
} as React.CSSProperties
}
>
<CheckboxPrimitive.Root
className={styles.checkbox}
ref={ref}
id={id}
+ aria-describedby={descriptionId}
+ aria-required={required}
{...props}
>Committable suggestion skipped: line range outside the PR's diff.
packages/checkbox/src/Checkbox.tsx
Outdated
| export const CheckboxGroup = forwardRef<HTMLDivElement, CheckboxGroupProps>( | ||
| ({ children, name, onChange, value }, ref) => { | ||
| const handleCheckedChange = (checked: CheckboxPrimitive.CheckedState, itemValue: string | undefined) => { | ||
| if (!onChange || !itemValue || typeof checked !== 'boolean') return; | ||
|
|
||
| const newValues = checked | ||
| ? [...(value || []), itemValue] | ||
| : (value || []).filter((v) => v !== itemValue); | ||
|
|
||
| onChange(newValues); | ||
| }; | ||
|
|
||
| const mappedChildren = Children.map(children, (child) => { | ||
| if (!isValidElement<CheckboxProps>(child)) return child; | ||
|
|
||
| const checked = value?.includes(child.props.value || '') || false; | ||
|
|
||
| return cloneElement(child, { | ||
| checked, | ||
| name, | ||
| onCheckedChange: (state: CheckboxPrimitive.CheckedState) => { | ||
| handleCheckedChange(state, child.props.value); | ||
| child.props.onCheckedChange?.(state); | ||
| }, | ||
| } as Partial<CheckboxProps>); | ||
| }); | ||
|
|
||
| return ( | ||
| <div className={styles.group} ref={ref}> | ||
| {mappedChildren} | ||
| </div> | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
에러 처리 및 타입 안정성 개선 제안
CheckboxGroup 컴포넌트의 에러 처리와 타입 안정성을 개선하면 좋을 것 같습니다:
- 중복된 value 검증
- 자식 컴포넌트 타입 검증 강화
- 콜백 에러 처리
export const CheckboxGroup = forwardRef<HTMLDivElement, CheckboxGroupProps>(
({ children, name, onChange, value }, ref) => {
+ const valueSet = new Set(value);
+
+ if (valueSet.size !== value?.length) {
+ console.warn('CheckboxGroup: Duplicate values detected');
+ }
+
const handleCheckedChange = (checked: CheckboxPrimitive.CheckedState, itemValue: string | undefined) => {
if (!onChange || !itemValue || typeof checked !== 'boolean') return;
const newValues = checked
? [...(value || []), itemValue]
: (value || []).filter((v) => v !== itemValue);
- onChange(newValues);
+ try {
+ onChange(newValues);
+ } catch (error) {
+ console.error('CheckboxGroup: onChange handler error', error);
+ }
};📝 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 CheckboxGroup = forwardRef<HTMLDivElement, CheckboxGroupProps>( | |
| ({ children, name, onChange, value }, ref) => { | |
| const handleCheckedChange = (checked: CheckboxPrimitive.CheckedState, itemValue: string | undefined) => { | |
| if (!onChange || !itemValue || typeof checked !== 'boolean') return; | |
| const newValues = checked | |
| ? [...(value || []), itemValue] | |
| : (value || []).filter((v) => v !== itemValue); | |
| onChange(newValues); | |
| }; | |
| const mappedChildren = Children.map(children, (child) => { | |
| if (!isValidElement<CheckboxProps>(child)) return child; | |
| const checked = value?.includes(child.props.value || '') || false; | |
| return cloneElement(child, { | |
| checked, | |
| name, | |
| onCheckedChange: (state: CheckboxPrimitive.CheckedState) => { | |
| handleCheckedChange(state, child.props.value); | |
| child.props.onCheckedChange?.(state); | |
| }, | |
| } as Partial<CheckboxProps>); | |
| }); | |
| return ( | |
| <div className={styles.group} ref={ref}> | |
| {mappedChildren} | |
| </div> | |
| ); | |
| } | |
| ); | |
| export const CheckboxGroup = forwardRef<HTMLDivElement, CheckboxGroupProps>( | |
| ({ children, name, onChange, value }, ref) => { | |
| const valueSet = new Set(value); | |
| if (valueSet.size !== value?.length) { | |
| console.warn('CheckboxGroup: Duplicate values detected'); | |
| } | |
| const handleCheckedChange = (checked: CheckboxPrimitive.CheckedState, itemValue: string | undefined) => { | |
| if (!onChange || !itemValue || typeof checked !== 'boolean') return; | |
| const newValues = checked | |
| ? [...(value || []), itemValue] | |
| : (value || []).filter((v) => v !== itemValue); | |
| try { | |
| onChange(newValues); | |
| } catch (error) { | |
| console.error('CheckboxGroup: onChange handler error', error); | |
| } | |
| }; | |
| const mappedChildren = Children.map(children, (child) => { | |
| if (!isValidElement<CheckboxProps>(child)) return child; | |
| const checked = value?.includes(child.props.value || '') || false; | |
| return cloneElement(child, { | |
| checked, | |
| name, | |
| onCheckedChange: (state: CheckboxPrimitive.CheckedState) => { | |
| handleCheckedChange(state, child.props.value); | |
| child.props.onCheckedChange?.(state); | |
| }, | |
| } as Partial<CheckboxProps>); | |
| }); | |
| return ( | |
| <div className={styles.group} ref={ref}> | |
| {mappedChildren} | |
| </div> | |
| ); | |
| } | |
| ); |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/checkbox/src/Checkbox.tsx (1)
101-134: 🛠️ Refactor suggestionCheckboxGroup 컴포넌트의 안정성 개선이 필요합니다
이전 리뷰에서 언급된 중복 값 검증과 에러 처리 외에도, 다음 사항들을 개선하면 좋을 것 같습니다:
defaultValue와value동시 사용 시 경고 추가- 자식 컴포넌트가 없는 경우 처리
- 체크박스 그룹에 대한 접근성 속성 추가 (예: role="group", aria-label)
export const CheckboxGroup = forwardRef<HTMLDivElement, CheckboxGroupProps>( ({ children, name, onChange, value }, ref) => { + if (value !== undefined && defaultValue !== undefined) { + console.warn('CheckboxGroup: Both value and defaultValue props are provided. defaultValue will be ignored.'); + } + + if (!Children.count(children)) { + console.warn('CheckboxGroup: No children provided'); + } + // ... existing code ... return ( - <div className={styles.group} ref={ref}> + <div + className={styles.group} + ref={ref} + role="group" + aria-label={`${name || 'Checkbox'} group`} + > {mappedChildren} </div> ); } );
🧹 Nitpick comments (1)
packages/checkbox/src/Checkbox.tsx (1)
10-15: 타입 정의 개선이 필요합니다
CheckboxProps인터페이스에 다음과 같은 접근성 관련 속성들을 추가하면 좋을 것 같습니다:
aria-label: 레이블이 시각적으로 보이지 않을 때 사용aria-describedby: 추가 설명이 필요한 경우 사용required: 필수 입력 여부또한
value속성의 타입을string | undefined로 명시적으로 지정하는 것이 좋습니다.export interface CheckboxProps extends Omit<ComponentPropsWithoutRef<typeof CheckboxPrimitive.Root>, 'asChild'> { label?: string; size?: CheckboxSize; - value?: string; + value?: string | undefined; + 'aria-label'?: string; + 'aria-describedby'?: string; + required?: boolean; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/checkbox/package.json(1 hunks)packages/checkbox/src/Checkbox.module.css(1 hunks)packages/checkbox/src/Checkbox.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/checkbox/package.json
- packages/checkbox/src/Checkbox.module.css
🔇 Additional comments (2)
packages/checkbox/src/Checkbox.tsx (2)
25-49: 구현이 깔끔합니다!
getSizeStyles함수가 디자인 토큰을 잘 활용하고 있으며, 각 사이즈별로 일관된 스타일을 제공합니다.
78-87: 🛠️ Refactor suggestion키보드 접근성 개선이 필요합니다
키보드 사용자를 위한 포커스 관리가 필요합니다:
tabIndex속성 추가- 포커스 스타일에 대한 시각적 피드백 강화
<CheckboxPrimitive.Root className={styles.checkbox} ref={ref} id={id} + tabIndex={0} {...props} > <CheckboxPrimitive.Indicator className={styles.indicator}> <Check size={indicatorSize} /> </CheckboxPrimitive.Indicator> </CheckboxPrimitive.Root>Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/checkbox/src/Checkbox.tsx (3)
10-15: 접근성 관련 props 추가 필요접근성을 개선하기 위해 다음과 같은 props를 추가하는 것이 좋겠습니다:
aria-label: 스크린 리더를 위한 레이블aria-describedby: 추가 설명을 위한 요소 IDrequired: 필수 입력 여부export interface CheckboxProps extends Omit<ComponentPropsWithoutRef<typeof CheckboxPrimitive.Root>, 'asChild'> { label?: string; size?: CheckboxSize; value?: string; + 'aria-label'?: string; + 'aria-describedby'?: string; + required?: boolean; }
25-49: 타입 안전성 개선 필요
getSizeStyles함수의 반환 타입을 명시적으로 정의하여 타입 안전성을 개선하면 좋겠습니다.+interface SizeStyles { + checkboxSize: string; + indicatorSize: string; + fontSize: string; + padding: string; +} + -const getSizeStyles = (size: CheckboxSize) => { +const getSizeStyles = (size: CheckboxSize): SizeStyles => {
63-77: 호버 상태 스타일 추가 필요사용자 경험 향상을 위해 호버 상태의 스타일을 추가하면 좋겠습니다.
{ '--margin': '4px', '--border-radius': '4px', '--padding': padding, '--size': checkboxSize, '--indicator-size': indicatorSize, '--border-color': color.gray400, '--background-color': color.white, '--checked-border-color': color.cyan300, '--checked-background-color': color.cyan300, '--indicator-color': color.white, '--focus-border-color': color.cyan500, + '--hover-border-color': color.gray500, + '--hover-background-color': color.gray50, ...style, } as React.CSSProperties
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
packages/checkbox/src/Checkbox.module.css(1 hunks)packages/checkbox/src/Checkbox.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/checkbox/src/Checkbox.module.css
🔇 Additional comments (2)
packages/checkbox/src/Checkbox.tsx (2)
102-112: 중복 값 검증 및 에러 처리 필요CheckboxGroup에서 중복된 값 검증과 에러 처리를 추가하면 좋겠습니다.
export const CheckboxGroup = forwardRef<HTMLDivElement, CheckboxGroupProps>( ({ children, name, onChange, value }, ref) => { + // 중복 값 검증 + const valueSet = new Set(value); + if (valueSet.size !== value?.length) { + console.warn('CheckboxGroup: 중복된 값이 감지되었습니다'); + } + const handleCheckedChange = (checked: CheckboxPrimitive.CheckedState, itemValue: string | undefined) => { if (!onChange || !itemValue || typeof checked !== 'boolean') return; const newValues = checked ? [...(value || []), itemValue] : (value || []).filter((v) => v !== itemValue); - onChange(newValues); + try { + onChange(newValues); + } catch (error) { + console.error('CheckboxGroup: onChange 핸들러 에러', error); + } };
130-132: 키보드 포커스 스타일 추가 필요키보드 접근성 향상을 위해 포커스 스타일을 추가하면 좋겠습니다.
- <div className={styles.group} ref={ref}> + <div + className={styles.group} + ref={ref} + style={{ + '--focus-outline-color': color.cyan500, + '--focus-outline-width': '2px', + } as React.CSSProperties} + >
- move non-variable elements to module.css for better separation
- Replace logical operators with ternary operators for clarity - Clean up type imports and React imports for better organization
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/checkbox/src/Checkbox.tsx (2)
79-83: 🛠️ Refactor suggestion키보드 포커스 스타일 추가 필요
키보드 사용자를 위한 포커스 스타일이 누락되어 있습니다. CSS 모듈에
:focus-visible스타일을 추가하는 것이 좋겠습니다.
97-112: 🛠️ Refactor suggestionCheckboxGroup 에러 처리 개선 필요
다음과 같은 에러 처리 개선이 필요합니다:
- 중복된 value 검증
- onChange 콜백의 에러 처리
- 타입 안정성 강화
export const CheckboxGroup = forwardRef<HTMLDivElement, CheckboxGroupProps>( ({ children, name, onChange, value }, ref) => { + // 중복 값 검증 + const valueSet = new Set(value); + if (valueSet.size !== value?.length) { + console.warn('CheckboxGroup: 중복된 값이 감지되었습니다'); + } const handleCheckedChange = ( checked: CheckboxPrimitive.CheckedState, itemValue: string | undefined, ) => { if (!onChange || !itemValue || typeof checked !== 'boolean') { return; } const newValues = checked ? [...(value ?? []), itemValue] : (value ?? []).filter((v) => v !== itemValue); - onChange(newValues); + try { + onChange(newValues); + } catch (error) { + console.error('CheckboxGroup: onChange 핸들러 에러', error); + } };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 104-105: packages/checkbox/src/Checkbox.tsx#L104-L105
Added lines #L104 - L105 were not covered by tests
🧹 Nitpick comments (1)
packages/checkbox/src/Checkbox.tsx (1)
61-64: 컴포넌트 문서화 개선 필요
Checkbox와CheckboxGroup컴포넌트에 JSDoc 주석을 추가하여 사용 방법과 props에 대한 설명을 추가하면 좋겠습니다.예시:
/** * Checkbox 컴포넌트는 사용자가 여러 옵션 중에서 선택할 수 있게 해주는 입력 요소입니다. * * @param {CheckboxProps} props - 체크박스 속성 * @param {string} [props.label] - 체크박스 레이블 * @param {CheckboxSize} [props.size='md'] - 체크박스 크기 * @param {string} [props.value] - 체크박스 값 */Also applies to: 97-98
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/checkbox/.storybook/preview.ts(1 hunks)packages/checkbox/src/Checkbox.module.css(1 hunks)packages/checkbox/src/Checkbox.test.tsx(1 hunks)packages/checkbox/src/Checkbox.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/checkbox/.storybook/preview.ts
- packages/checkbox/src/Checkbox.test.tsx
- packages/checkbox/src/Checkbox.module.css
🧰 Additional context used
🪛 GitHub Check: codecov/patch
packages/checkbox/src/Checkbox.tsx
[warning] 90-90: packages/checkbox/src/Checkbox.tsx#L90
Added line #L90 was not covered by tests
[warning] 104-105: packages/checkbox/src/Checkbox.tsx#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 116-117: packages/checkbox/src/Checkbox.tsx#L116-L117
Added lines #L116 - L117 were not covered by tests
🔇 Additional comments (3)
packages/checkbox/src/Checkbox.tsx (3)
20-25: Props 인터페이스에 description 속성 추가 제안접근성 향상을 위해
CheckboxProps인터페이스에description속성을 추가하면 좋을 것 같습니다. 이는 스크린 리더 사용자에게 추가적인 컨텍스트를 제공할 수 있습니다.export interface CheckboxProps extends Omit<ComponentPropsWithoutRef<typeof CheckboxPrimitive.Root>, 'asChild'> { label?: string; size?: CheckboxSize; value?: string; + description?: string; }
35-59: 스타일 로직 CSS 모듈로 이동 제안현재
getSizeStyles함수에서 관리되는 고정된 스타일 값들을 CSS 모듈로 이동하면 좋을 것 같습니다. CSS 변수를 활용하면 더 나은 유지보수성과 성능을 얻을 수 있습니다.
90-90: 테스트 커버리지 개선 필요다음 코드 경로들에 대한 테스트가 누락되어 있습니다:
- label이 없는 경우 (L90)
- onChange 콜백이 없는 경우 (L104-105)
- 유효하지 않은 자식 요소가 있는 경우 (L116-117)
이러한 엣지 케이스들에 대한 테스트를 추가하면 좋겠습니다.
Also applies to: 104-105, 116-117
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 90-90: packages/checkbox/src/Checkbox.tsx#L90
Added line #L90 was not covered by tests
변경사항
@sipe-team/checkbox 패키지 추가
체크박스 컴포넌트 구현 [#6]
CheckboxGroup 컴포넌트 구현
Storybook을 통한 컴포넌트 문서화
시각자료
2025-01-21.4.05.29.mov
체크리스트
추가 논의사항
질문, 변경사항 관련해서 코멘트 달아주시면 감사하겠습니다!
Summary by CodeRabbit
릴리즈 노트
새로운 기능
개선 사항
개발 환경