refactor: JDS Checkbox 컴포넌트 vanilla-extract 마이그레이션#462
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesCheckbox 마이그레이션 (vanilla-extract + react-aria)
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant CheckboxRoot
participant CheckboxProvider
participant CheckboxItem
participant CheckboxBasic
Consumer->>CheckboxRoot: value / onChange / size / variant 전달
CheckboxRoot->>CheckboxRoot: useCheckboxGroupState() 호출
CheckboxRoot->>CheckboxProvider: size, variant, disabled, isInvalid, state 제공
CheckboxProvider->>CheckboxItem: 컨텍스트 전파
CheckboxItem->>CheckboxItem: childChecked로 isInvalid && !childChecked 계산
CheckboxItem->>CheckboxProvider: onChildCheckedChange 콜백 등록
CheckboxItem->>CheckboxBasic: 그룹 state 존재 여부 판단
alt 그룹 내 아이템 (value 필수)
CheckboxBasic->>CheckboxBasic: useCheckboxGroupItem() 호출
CheckboxBasic->>CheckboxItem: onChildCheckedChange(isChecked) 동기화
else 단독 아이템
CheckboxBasic->>CheckboxBasic: useToggleState() 호출
CheckboxBasic->>CheckboxBasic: ref.current.indeterminate 직접 세팅
CheckboxBasic->>CheckboxItem: onChildCheckedChange(isChecked) 동기화
end
CheckboxBasic-->>Consumer: input + checkboxVisual 렌더링
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60분 Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
packages/jds/src/components/Checkbox/migration/checkbox.css.ts (1)
30-31:.visual문자열에 대한 취약한 스타일 계약을 제거하세요.checkbox.css.ts의 30-31줄과 163-165줄처럼 전역 선택자가 하드코딩된
"visual"클래스에 의존하면, Checkbox.tsx에서 호출부를 리팩터링할 때 포커스 링 동작이 조용히 깨질 수 있습니다. 슬롯 전용 클래스를 checkbox.css.ts에서 export하고, Checkbox.tsx에서 그 클래스를 함께 주입하도록 변경하면 이러한 위험을 제거할 수 있습니다.변경 예시
-// 호출자는 className에 "visual" 리터럴을 반드시 함께 부여해야 한다. checkboxItem globalStyle이 이 클래스명에 의존한다. +export const checkboxVisualSlot = style({}); -globalStyle(`${checkboxItemGrid} input[type="checkbox"]:focus-visible + .visual`, { +globalStyle(`${checkboxItemGrid} input[type="checkbox"]:focus-visible + .${checkboxVisualSlot}`, { boxShadow: "none !important", });-<span className={clsx(checkboxVisual({ size }), "visual")} aria-hidden='true'> +<span className={clsx(checkboxVisual({ size }), checkboxVisualSlot)} aria-hidden='true'>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jds/src/components/Checkbox/migration/checkbox.css.ts` around lines 30 - 31, Remove the brittle hardcoded dependency on the "visual" class string in checkbox.css.ts. Instead of relying on callers to manually inject the "visual" className, export a named class constant from checkbox.css.ts that represents the visual slot (this will replace the reliance on the global selector and the comments at lines 30-31 and 163-165 about the hardcoded "visual" string). Then update Checkbox.tsx to import this exported class and inject it alongside other classes when rendering the checkbox element, eliminating the fragile string contract that could silently break during refactoring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/jds/src/components/Checkbox/migration/checkbox.css.ts`:
- Line 9: The checkboxGroupWrapper style uses display: contents which removes
the element from the layout tree and breaks accessibility semantics for the
checkbox group, causing some browser and screen reader combinations to
incorrectly interpret the accessibility attributes from react-aria's
useCheckboxGroup(). Replace the display: contents value in the
checkboxGroupWrapper style definition with either display: block or display:
inline-flex to preserve the semantic meaning of the group wrapper while
maintaining proper accessibility support.
In `@packages/jds/src/components/Checkbox/migration/Checkbox.stories.tsx`:
- Around line 23-26: The Checkbox.Basic components in the stories lack
accessible names, causing accessibility warnings for unlabeled controls. Add
aria-label or aria-labelledby attributes to each Checkbox.Basic instance to
provide accessible names. This issue affects multiple locations in the file
packages/jds/src/components/Checkbox/migration/Checkbox.stories.tsx: at lines
23-26 (the anchor location showing the basic size variants), at lines 35-45, and
at lines 285-295. For each location, add appropriate aria-label attributes to
the Checkbox.Basic components to describe their purpose or distinguishing
characteristics.
In `@packages/jds/src/components/Checkbox/migration/Checkbox.tsx`:
- Around line 145-166: The forwardRef is created using
useObjectRef(forwardedRef) but is not being connected to the actual input
element, causing the ref contract to break in group mode where ref.current
remains empty. Add the ref attribute to the input element (the one with
mergeProps(inputProps, restProps)) by passing the ref variable that was created,
so the forwarded reference actually points to the underlying input DOM element.
- Around line 260-271: In group mode (when context?.state exists), the
CheckboxBasicGrouped component receives value={value ?? ""} which defaults
missing values to empty strings, causing multiple items without explicit values
to be treated identically and breaking group selection logic. Remove the
fallback to empty string and instead require value to be present in group mode -
either validate that value exists before rendering CheckboxBasicGrouped or throw
an error when value is missing, ensuring each checkbox item in the group has a
unique identifier.
---
Nitpick comments:
In `@packages/jds/src/components/Checkbox/migration/checkbox.css.ts`:
- Around line 30-31: Remove the brittle hardcoded dependency on the "visual"
class string in checkbox.css.ts. Instead of relying on callers to manually
inject the "visual" className, export a named class constant from
checkbox.css.ts that represents the visual slot (this will replace the reliance
on the global selector and the comments at lines 30-31 and 163-165 about the
hardcoded "visual" string). Then update Checkbox.tsx to import this exported
class and inject it alongside other classes when rendering the checkbox element,
eliminating the fragile string contract that could silently break during
refactoring.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1aab636a-10b7-426c-9a4d-ee90a629b8d9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
packages/jds/package.jsonpackages/jds/src/components/Checkbox/migration/Checkbox.stories.tsxpackages/jds/src/components/Checkbox/migration/Checkbox.tsxpackages/jds/src/components/Checkbox/migration/CheckboxContext.tsxpackages/jds/src/components/Checkbox/migration/checkbox.css.tspackages/jds/src/components/Checkbox/migration/checkbox.types.tspackages/jds/src/components/Checkbox/migration/checkbox.variants.tspackages/jds/src/utils/index.tspackages/jds/src/utils/labelTypographyVars.css.ts
| import { CHECKBOX_SIZE_OPTIONS, type CheckboxSize } from "./checkbox.types"; | ||
| import { checkboxSizeMap } from "./checkbox.variants"; | ||
|
|
||
| export const checkboxGroupWrapper = style({ display: "contents" }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# checkboxGroupWrapper가 실제 groupProps가 붙는 노드에 적용되는지 확인
rg -n -C3 'checkboxGroupWrapper|useCheckboxGroup|groupProps' packages/jds/src/components/Checkbox/migration/Checkbox.tsx
rg -n -C2 'display:\s*"contents"' packages/jds/src/components/Checkbox/migration/checkbox.css.tsRepository: JECT-Study/JECT-Official-WebSite-Client
Length of output: 1716
display: contents가 그룹 접근성 시맨틱을 깨뜨릴 수 있습니다.
Line 60에서 checkboxGroupWrapper가 groupProps를 받는 div 요소에 적용됩니다. display: contents는 요소를 레이아웃 트리에서 제거하면서, react-aria의 useCheckboxGroup()에서 제공하는 접근성 속성을 일부 브라우저/스크린리더 조합에서 올바르게 해석하지 못할 수 있습니다. 그룹 래퍼는 시맨틱 보존이 가능한 display: block 또는 display: inline-flex 계열로 변경하는 것이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jds/src/components/Checkbox/migration/checkbox.css.ts` at line 9,
The checkboxGroupWrapper style uses display: contents which removes the element
from the layout tree and breaks accessibility semantics for the checkbox group,
causing some browser and screen reader combinations to incorrectly interpret the
accessibility attributes from react-aria's useCheckboxGroup(). Replace the
display: contents value in the checkboxGroupWrapper style definition with either
display: block or display: inline-flex to preserve the semantic meaning of the
group wrapper while maintaining proper accessibility support.
| <Checkbox.Basic size='lg' /> | ||
| <Checkbox.Basic size='md' /> | ||
| <Checkbox.Basic size='sm' /> | ||
| <Checkbox.Basic size='xs' /> |
There was a problem hiding this comment.
Checkbox.Basic 단독 예시에 접근 가능한 이름을 추가해 주세요.
현재 여러 스토리에서 라벨 없이 Checkbox.Basic만 렌더링되어 접근성 검사에서 unlabeled control 경고가 날 수 있습니다. 각 항목에 aria-label(또는 aria-labelledby)을 넣어 주세요.
예시 수정
- <Checkbox.Basic size='lg' />
+ <Checkbox.Basic size='lg' aria-label='Large checkbox' />
- <Checkbox.Basic checked={false} onCheckedChange={() => {}} />
+ <Checkbox.Basic
+ checked={false}
+ aria-label='Unchecked checkbox'
+ onCheckedChange={() => {}}
+ />Also applies to: 35-45, 285-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jds/src/components/Checkbox/migration/Checkbox.stories.tsx` around
lines 23 - 26, The Checkbox.Basic components in the stories lack accessible
names, causing accessibility warnings for unlabeled controls. Add aria-label or
aria-labelledby attributes to each Checkbox.Basic instance to provide accessible
names. This issue affects multiple locations in the file
packages/jds/src/components/Checkbox/migration/Checkbox.stories.tsx: at lines
23-26 (the anchor location showing the basic size variants), at lines 35-45, and
at lines 285-295. For each location, add appropriate aria-label attributes to
the Checkbox.Basic components to describe their purpose or distinguishing
characteristics.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/jds/src/components/Checkbox/migration/checkbox.variants.ts (1)
6-14: 💤 Low value타입 추론 강화를 위해
as const단언 추가를 고려하세요 (선택 사항).현재 구현은 정상 작동하지만,
as const단언을 추가하면 각 속성 값이 리터럴 타입으로 좁혀져 더 엄격한 타입 안전성을 제공합니다.♻️ 제안하는 개선 사항
export const checkboxSizeMap: Record< CheckboxSize, { visual: string; icon: IconSize; label: LabelSize; helper: LabelSize } -> = { +> = { lg: { visual: pxToRem(20), icon: "md", label: "lg", helper: "sm" }, md: { visual: pxToRem(18), icon: "sm", label: "md", helper: "sm" }, sm: { visual: pxToRem(16), icon: "xs", label: "sm", helper: "xs" }, xs: { visual: pxToRem(14), icon: "2xs", label: "xs", helper: "xs" }, -}; +} as const;이렇게 하면 각 사이즈별 값이 더 구체적인 리터럴 타입으로 추론되어, 런타임 오류를 컴파일 타임에 잡을 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/jds/src/components/Checkbox/migration/checkbox.variants.ts` around lines 6 - 14, The checkboxSizeMap object in checkbox.variants.ts would benefit from stricter type inference. Add an `as const` assertion at the end of the checkboxSizeMap object definition to narrow each property value to its literal type. This will enable the TypeScript compiler to catch type mismatches at compile time rather than runtime by ensuring that icon size values like "md", "sm", "xs", "2xs" and label sizes are inferred as exact literal types rather than general string types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/jds/src/components/Checkbox/migration/checkbox.variants.ts`:
- Around line 6-14: The checkboxSizeMap object in checkbox.variants.ts would
benefit from stricter type inference. Add an `as const` assertion at the end of
the checkboxSizeMap object definition to narrow each property value to its
literal type. This will enable the TypeScript compiler to catch type mismatches
at compile time rather than runtime by ensuring that icon size values like "md",
"sm", "xs", "2xs" and label sizes are inferred as exact literal types rather
than general string types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e21a2aac-f377-446e-8240-128d602c46e0
📒 Files selected for processing (7)
.changeset/checkbox-migration.mdpackages/jds/src/components/Checkbox/migration/Checkbox.stories.tsxpackages/jds/src/components/Checkbox/migration/Checkbox.tsxpackages/jds/src/components/Checkbox/migration/CheckboxContext.tsxpackages/jds/src/components/Checkbox/migration/checkbox.css.tspackages/jds/src/components/Checkbox/migration/checkbox.types.tspackages/jds/src/components/Checkbox/migration/checkbox.variants.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/checkbox-migration.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jds/src/components/Checkbox/migration/CheckboxContext.tsx
- packages/jds/src/components/Checkbox/migration/checkbox.types.ts
- packages/jds/src/components/Checkbox/migration/Checkbox.stories.tsx
- packages/jds/src/components/Checkbox/migration/checkbox.css.ts
- packages/jds/src/components/Checkbox/migration/Checkbox.tsx
Zero-1016
left a comment
Zero-1016
left a comment
There was a problem hiding this comment.
확인했습니다. 추가로 몇가지 코멘트 남겼으니 확인해주세요!
areumH
left a comment
areumH
left a comment
There was a problem hiding this comment.
수고하셨습니다!! 간단히 코멘트 남깁니다
|
|
||
| CheckboxBasic.displayName = "Checkbox.Basic"; | ||
|
|
||
| const CheckboxLabel = forwardRef<HTMLDivElement, CheckboxLabelProps>(({ children }, ref) => { |
There was a problem hiding this comment.
Checkbox.Label이 실제 checkbox input과 연결되어 있지 않아서 라벨 텍스트를 클릭해도 체크박스가 토글되지 않을 것 같아요.
피그마의 하이퍼링크 사용 - 이 때 checkbox 인터렉션에 가려져 하이퍼링크에 접근할 수 없는 경우를 방지해야 합니다. 문구에 따르면 레이블 텍스트나 그룹 컴포넌트를 클릭했을 때 체크박스 토글로 이어져야할 것 같아 코멘트 남겨요!
There was a problem hiding this comment.
기존 구현에서도 누락되어 있던 부분이었네요! 789e523 에서 반영했습니다. 하이퍼링크 사용 관련해서는 Checkbox에서 서브 컴포넌트로 제공하는 것과 소비자가 직접 조합하는 것 중 어떤 방식이 더 적절할지 조금 더 검토해봐야 할 것 같아요
| 'input[type="checkbox"]:not(:disabled) + &:hover::after': { | ||
| opacity: overlayOpacityMap.normal.hover, | ||
| }, | ||
| 'input[type="checkbox"]:not(:disabled) + &:active::after': { | ||
| opacity: overlayOpacityMap.normal.pressed, |
There was a problem hiding this comment.
Checkbox.Item 안에서 Checkbox.Basic을 사용할 때 item overlay와 visual overlay가 동시에 켜질 수 있을 것 같아요. 체크박스 visual 영역에 hover될 경우 해당 영역만 색이 더 진해질 수 있어 보이는데, 의도된 스타일인지 궁금합니다!
There was a problem hiding this comment.
기존에는 visual 영역만 클릭 가능해서 별도 인터랙션 레이어를 두었는데 label과 연결된 이후에는 중복인 것 같아 제거해 두었습니다.
다만 지금은 label 영역에만 id를 연결해서 토글 범위가 제한되어 있는데, 다시 생각해 보니 Checkbox.Item이 hover/press 인터랙션을 위임받는 만큼 토글도 같이 책임지는 편이 더 자연스러울 것 같네요 😅 이 부분은 구조 변경이 꽤 있을 것 같아 별도 PR로 반영해 두겠습니다~
Zero-1016
left a comment
Zero-1016
left a comment
There was a problem hiding this comment.
conflict 나는 부분 확인해주세요.
Zero-1016
left a comment
Zero-1016
left a comment
There was a problem hiding this comment.
다음 코멘트들 확인 부탁드려요. 놓친 부분들 다시 코멘트 드립니다.
There was a problem hiding this comment.
해당 파일을 제거하고 Checkbox.tsx / checkbox.css.ts에서 직접 처리하는 게 가독성(응집도) 면에서 좋아 보여요.
checkboxSizeMap의 필드를 보면 소비처가 완전히 갈립니다:
visual→checkbox.css.ts에서만 사용 (recipe의 width/height)iconlabelhelper→Checkbox.tsx에서만 사용 (<Icon size>,getLabelClassName)
지금은 한 테이블에 모여 있지만 정작 한 파일이 네 필드를 다 쓰는 곳은 없어요. 그래서 사이즈를 수정하거나 추가할 때 variants.ts로 한 번 더 들어가야 해서, 오히려 depth만 길어지고 불편한 것 같아요.
소비처 기준으로 나누면:
// checkbox.css.ts — visual만
const checkboxSizeMap = {
lg: { visual: pxToRem(20) },
md: { visual: pxToRem(18) },
sm: { visual: pxToRem(16) },
xs: { visual: pxToRem(14) },
} satisfies Record<CheckboxSize, { visual: string }>;// Checkbox.tsx — icon / label / helper
const checkboxSizeMap = {
lg: { icon: "md", label: "lg", helper: "sm" },
md: { icon: "sm", label: "md", helper: "sm" },
sm: { icon: "xs", label: "sm", helper: "xs" },
xs: { icon: "2xs", label: "xs", helper: "xs" },
} satisfies Record<CheckboxSize, { icon: IconSize; label: LabelSize; helper: LabelSize }>;이러면 각 파일이 자기가 쓰는 토큰만 갖고, pxToRem / IconSize·LabelSize import도 각 소비처로 따라가 정리됩니다.
"사이즈 토큰이 두 군데로 흩어지지 않나"라는 우려는 있을 수 있는데, visual과 나머지는 소비처가 갈려서 실제로 같이 수정할 일이 적습니다. 한 테이블로 묶어둔 응집보다, 각 파일이 쓰는 값만 곁에 두는 쪽이 이 경우엔 더 자연스러워 보여요.
There was a problem hiding this comment.
원래는 사이즈 관련 값을 한 객체에 모아 컴포넌트의 사이즈 설정 값을 SSOT로 관리하려는 의도였는데요, 각 서브 컴포넌트가 모두 동일한 size variant에 따라 함께 결정되는 값이라고 생각해서 사이즈를 추가하거나 수정할 때 한 곳만 확인하면 된다고 생각했어요.
다만 말씀해주신 대로 실제 사용 위치가 분리되어 있어서 SSOT의 이점은 살리지 못하고 불필요한 depth만 늘어나는 것처럼 보이기도 하네요 😓
|
|
||
| export const CheckboxProvider = CheckboxContext.Provider; | ||
|
|
||
| export const useCheckboxContext = () => useContext(CheckboxContext); |
There was a problem hiding this comment.
지금 useContext를 바로 반환하고 있어, null 타입이 추론되기 때문에 코드상에서 null 검사를 해주고 있는 것을 확인했어요.
useContext를 사용하는 부분에서 검사를 진행한다면 다음처럼 사용할 수 있어요.
- const context = useCheckboxContext();
- const size = context?.size ?? "md";
+ const { size } = useCheckboxContext('Checkbox.Label');| export const useCheckboxContext = () => useContext(CheckboxContext); | |
| export const useCheckboxContext = (componentName: string) => { | |
| const context = useContext(CheckboxContext); | |
| if (!context) { | |
| throw new Error(`${componentName} 컴포넌트는 Checkbox.Root 내부에서 사용해야 합니다`); | |
| } | |
| return context; | |
| }; |
There was a problem hiding this comment.
컨텍스트를 nullable로 두고 코드에서 null을 처리하는 건 의도된 부분이었어요. Checkbox.Basic은 컨트롤 단독 사용(문서상으로도 권장되지는 않지만 지원하는 케이스)을 허용하고 있어서, Root/Item 없이 사용되면 컨텍스트가 null이 됩니다.
그리고 컨텍스트 분리 이후 useCheckboxItem()의 반환값이 null인지 여부로 Item 내부에서 사용 중인지도 함께 판별하고 있어서 컨텍스트가 null인 상태가 유효해요. "md" 폴백도 Provider 없이 동작할 수 있도록 둔 기본값이었어요.
다만 Label/Helper는 Item 외부에서 사용되면 의미가 없어서, 이 둘에만 제약을 둘 수 있을 것 같습니다. 공유 훅은 nullable 상태로 유지하고, Item 컨텍스트가 없으면 throw하는 훅을 별도로 만들어 Label/Helper에만 적용하는 방식을 생각하고 있는데, 혹시 더 좋은 방향이 있을까요?
There was a problem hiding this comment.
내부에 명시적인 플래그/분기 로직을 두고 싶지 않아서, 컨텍스트 존재 여부로 갈라낼 수 있을지 생각해봤어요. 그런데 Checkbox.Basic이 단독·그룹 양쪽에서 공용으로 쓰이다 보니(혼자 쓰거나, Root 안에서 그룹으로 쓰거나) 이 방식으로는 깔끔하게 분리하기 어려워 보여요. useCheckboxItem Provider도 Basic 최상단에서 씌워지는 구조라, "컨텍스트가 있으면 Item 안"이라는 판별이 성립하지 않더라고요.
그래서 Radio와 동일한 컴파운드 패턴을 유지하는 선에서는, 지금처럼 분기 로직을 두는 게 최선이라고 생각합니다. 컨텍스트 존재 여부로 우아하게 처리하려면 결국 Basic을 그룹용/단독용 별도 컴포넌트로 쪼개는 수밖에 없고, 안 쪼갤 거면 지금 방식을 유지할 수밖에 없어 보여요.
다만 만약 쪼갠다면 아래처럼 단독용과 그룹용 진입점을 나누는 형태가 될 것 같아요. 단독은 Checkbox가 직접 상태를 갖고, 그룹은 CheckboxGroup.Item이 그룹 상태를 받는 식이라 각 경로의 Basic이 자기 역할을 위치로 되물을 필요가 없어져요.
// 단독
<Checkbox.Root>
<Checkbox.Label />
<Checkbox.Helper />
</Checkbox.Root>// 그룹
<CheckboxGroup.Root>
<CheckboxGroup.Item>
<CheckboxGroup.Label />
<CheckboxGroup.Helper />
</CheckboxGroup.Item>
<CheckboxGroup.Item>
<CheckboxGroup.Label />
<CheckboxGroup.Helper />
</CheckboxGroup.Item>
</CheckboxGroup.Root>물론 이 방향은 Radio와 맞춰둔 현재 패턴을 벗어나는 거라, 이 PR 범위에서 당장 적용하기보다는 별도로 논의해보면 좋을 것 같아요.
| interface CheckboxBasicGroupedProps { | ||
| size: CheckboxSize; | ||
| value: string; | ||
| isDisabled: boolean; | ||
| isInvalid: boolean; | ||
| controlId: string; | ||
| interaction: "on" | "off"; | ||
| state: CheckboxGroupState; | ||
| onChildCheckedChange?: (checked: CheckedState) => void; | ||
| forwardedRef: ForwardedRef<HTMLInputElement>; | ||
| restProps: InputHTMLAttributes<HTMLInputElement>; | ||
| } |
There was a problem hiding this comment.
Props를 선언하는 위치에 규칙이 있을까요? 기존에는 *.types.ts에 선언했던거 같아서요.
There was a problem hiding this comment.
외부에 공개되는 컴포넌트의 props만 checkbox.types.ts에서 관리하고, CheckboxControl, CheckboxBasicGrouped, CheckboxBasicStandalone처럼 export되지 않는 내부 헬퍼 컴포넌트의 props는 사용처 바로 위에 인라인으로 선언했습니다. (Radio, Card와 동일) 다만 문서화된 규칙은 아니라서 공통 컨벤션을 정해두어도 좋을 것 같아요
There was a problem hiding this comment.
2팀분들과 함께, JSDoc 수준 논의와 함께 공유되면 좋을 것 같아요.
# Conflicts: # packages/jds/src/utils/typography.css.ts
* refactor: Checkbox.Item을 단일 label로 전환해 아이템 전체 토글 통일 * feat: Checkbox.Helper를 aria-describedby로 input에 연결 * fix: CheckboxItemProps 기반 타입을 label로 수정 * fix: Checkbox.Helper를 span으로 변경
itwillbeoptimal
force-pushed
the
refactor/451-jds-checkbox-migration
branch
from
1333cf0 to
dab2c6a
Compare
|
사용처( 특히 |
github-project-automation
Bot
moved this from In Progress
to Done
in JECT Official WebSite
* chore: @react-aria/checkbox 패키지 의존성 추가 * feat: Checkbox vanilla-extract 마이그레이션 타입 정의 * feat: Checkbox Context 정의 * fix: Checkbox 타입 최신 DS 기준으로 업데이트 * feat: labelTypographyVars CSS vars 유틸리티 추가 * fix: CheckboxContext에서 align 속성 제거 * feat: CheckboxContext에 onChildCheckedChange 콜백 추가 * feat: Checkbox vanilla-extract 스타일 작성 * refactor: checkboxSizeMap을 checkbox.variants로 분리 * refactor: Checkbox prop/타입에서 컴포넌트명 prefix 제거 * fix: Checkbox visual disabled 상태에서 인터랙션 오버레이 제거 * feat: Checkbox vanilla-extract 마이그레이션 구현체 작성 * refactor: Checkbox 스타일 satisfies/어노테이션 표기 통일 * docs: Checkbox 마이그레이션 Storybook 문서 작성 * fix: 그룹 input 요소에 ref 연결 * fix: 그룹 내 Checkbox.Basic의 value 누락 검증 추가 * refactor: Checkbox 타이포그래피를 getLabelClassName으로 통일 * refactor: Checkbox 사이즈 맵을 단일 테이블로 통합 * fix: 체크 상태 타입명을 CheckedState로 복원 * docs: Checkbox 마이그레이션 changeset 추가 * docs: changeset 제목 수정 * refactor: focus ring 억제 로직을 globalStyle에서 variant로 전환 * fix: 라벨 색상을 labelColorVar 주입으로 변경해 cascade 순서 의존 제거 # Conflicts: # packages/jds/src/utils/typography.css.ts * style: ::before/::after 동일 shape를 쉼표 셀렉터로 통합 * feat: Label 클릭 시 체크박스가 토글되도록 controlId 연결 * fix: Item 내부에서 Checkbox visual 오버레이 이중 적용 제거 * fix: Checkbox.Item 클릭 영역 불일치 및 구조 수정 (#471) * refactor: Checkbox.Item을 단일 label로 전환해 아이템 전체 토글 통일 * feat: Checkbox.Helper를 aria-describedby로 input에 연결 * fix: CheckboxItemProps 기반 타입을 label로 수정 * fix: Checkbox.Helper를 span으로 변경 * refactor: checkbox.variants.ts 제거 및 사이즈맵을 소비처로 분리 * refactor: 클래스명을 역할에 맞게 변경 * refactor: 컨텍스트를 공통 설정과 Item 내부 상태 공유 용도로 분리 * refactor: parentContext를 parentConfig로 수정 * refactor: 마이그레이션 구현체 교체 및 소비처 정리
4 participants
💡 작업 내용
@react-aria/checkbox의존성 추가CheckboxContext작성checkbox.css.tsVE 스타일 작성migration/Checkbox.tsxVE 기반 구현체 작성migration/Checkbox.stories.tsx스토리 작성Checkbox구현 제거💡 자세한 설명
1. 개요
Checkbox의 스타일링 레이어를 Emotion에서 vanilla-extract로 마이그레이션합니다.@react-aria/checkbox를 도입하여 접근성 및 그룹 상태 관리를 위임하고, 전체 구조는Radio마이그레이션과 동일한 패턴을 따릅니다.기존
Checkbox는 Emotion 기반으로 구현되어 있으며 그룹 사용을 지원하지 않습니다. 신규 구현은 우선migration/디렉토리에 작성한 뒤, 본 PR 승인 후 기존 구현을 제거하고migration/의 코드로 완전히 대체할 예정입니다.Checkbox/ ├── Checkbox.tsx ← 기존 유지, 승인 후 제거 예정 ├── checkbox.styles.ts ← 기존 Emotion, 승인 후 제거 예정 ├── checkbox.types.ts ← 기존 타입, 승인 후 제거 예정 ├── Checkbox.stories.tsx ← 기존, 승인 후 제거 예정 ├── index.ts ← 승인 후 migration 기준으로 교체 └── migration/ ├── checkbox.types.ts ← 완료 ├── checkbox.variants.ts ← 완료 (사이즈 맵 SSOT) ├── checkbox.css.ts ← 완료 ├── CheckboxContext.tsx ← 완료 ├── Checkbox.tsx ← 완료 └── Checkbox.stories.tsx ← 완료2. 컴포넌트 구조
신규
Checkbox는Radio와 동일한 컴파운드 컴포넌트 구조를 따릅니다.Checkbox.RootCheckbox.ItemCheckbox.BasicCheckbox.LabelCheckbox.Helper기존
Checkbox.Content는label,subLabel을 props로 받아 렌더링하는 프리셋 컴포넌트였습니다. 신규 구조에서는Checkbox.Item,Checkbox.Basic,Checkbox.Label,Checkbox.Helper를 조합하는 방식으로 동일한 UI를 구성할 수 있습니다.3.
Checkbox.Root도입 및 네이밍기존
Checkbox구현에는 그룹 컨테이너 개념이 없었습니다. 신규 구현에서는@react-aria/checkbox의useCheckboxGroup과useCheckboxGroupState를 활용하여Checkbox.Root를 추가합니다.react-aria에서는 해당 개념을
CheckboxGroup으로 명명하고 있지만, 디자인 시스템 내부에서Radio.Root와 동일한 패턴을 유지하기 위해Checkbox.Root네이밍을 사용합니다.Radio는 항상 그룹 단위로 동작하지만, 체크박스는 단독으로 사용할 수 있으므로Checkbox.Root는 선택적으로 사용할 수 있습니다.Checkbox.Basic은 그룹 컨텍스트 존재 여부에 따라 내부 동작을 분기합니다.4.
Checkbox.Basic제어/비제어 패턴Radio.Basic은 그룹이 항상 선택 상태를 관리하므로 별도의 제어/비제어 분기가 존재하지 않습니다. 반면Checkbox.Basic은 단독 사용 시 자체적으로 상태를 관리해야 합니다.indeterminate상태는 비제어 모드에서 지원하지 않습니다.indeterminate는 HTML attribute가 아닌 DOM 프로퍼티(input.indeterminate)를 통해서만 설정할 수 있어 비제어 컴포넌트의 초기값으로 표현하기 어렵기 때문입니다.Checkbox.Root내부에서 사용되는 경우에는 그룹 상태가 선택 여부를 관리하므로checked와defaultChecked는 사용되지 않습니다. 이때value: string은 그룹 내 각 체크박스를 식별하기 위한 키 역할을 합니다.5.
isInvalid처리isInvalid는Radio에는 없는 개념입니다.Checkbox.Root와Checkbox.Basic모두에서 사용할 수 있으며, 그룹 전체 또는 개별 체크박스 단위로 유효성 검사 상태를 표현합니다.@react-aria/checkbox의CheckboxGroupState는isInvalid상태를 기본적으로 지원하므로, 그룹 수준의 invalid 상태를useCheckboxGroupState와 자연스럽게 연동할 수 있습니다.스타일 레이어에서는
data-invalid속성을 기반으로 처리하며,Radio에서data-disabled를 전파하는 방식과 동일한 패턴입니다.디자인 시스템 스펙상 invalid는 선택되지 않은 상태에 대한 유효성 검사 결과를 의미합니다. 따라서 checked 또는 indeterminate 상태에서는 invalid가 적용되지 않습니다.
이를 CSS 레이어에서
:not(:has(:checked))같은 셀렉터로 처리하는 대신 컴포넌트 레벨에서 invalid 상태를 필터링하여data-invalid를 부여하도록 구현했습니다.이렇게 계산된
isEffectiveInvalid를 기준으로 접근성 속성과 스타일 속성을 모두 제어합니다.Checkbox.Basicdata-invalid,aria-invalid,useCheckbox에 전달하는isInvalid모두isEffectiveInvalid를 사용합니다.Checkbox.ItemCheckbox.Basic으로부터 현재 checked 상태를 전달받아 invalid 여부를 판단합니다.onChildCheckedChange콜백을 통해 이루어지며,useLayoutEffect에서 페인트 이전에 반영합니다.스타일 레이어의
:has()기반 unchecked 가드를 사용하지 않고data-invalid속성 자체가 항상 unchecked + invalid 상태를 의미하도록 보장할 수 있습니다.6. 컨텍스트 구조
컨텍스트 구조는
Radio와 동일하게 단일 컨텍스트를 사용합니다.Checkbox.Root는 공통 설정과 그룹 상태를 제공하고,Checkbox.Item은 부모 컨텍스트를 읽어 자신의 props와 병합한 뒤 하위 컴포넌트에 전달합니다.onChildCheckedChange는 invalid 필터링을 위해 자식의 checked 상태를 상위로 전달하는 콜백입니다.checked 상태는
Checkbox.Basic이 소유하지만,data-invalid부여 여부는Checkbox.Item에서 결정해야 합니다. 이를 위해Item이 콜백을 제공하고,Basic이 상태 변경 시 현재 checked 값을 상위로 전달합니다.Checkbox.Basic을 단독으로 사용하는 경우에는 해당 콜백이 존재하지 않으므로 별도의 동작 없이 무시됩니다.7. 최신 DS 스펙 반영
마이그레이션 과정에서 확인된 최신 JDS 스펙을 반영했습니다.
variant값"empty""hollow""left" | "right"Checkbox.SubLabelCheckbox.Helper8. 그리드 슬롯 배치 개선
처음 구현은
Radio에서 사용했던 위치 기반 배치 방식을 그대로 가져왔습니다.이 방식에서는 다음 문제들이 있었습니다.
Item > Basic + Helper처럼 특정 슬롯이 생략되면 의도하지 않은 위치에 배치될 수 있습니다.:nth-child()는 자식 요소의 역할을 구분하지 않습니다. 예상하지 못한 요소가 추가될 경우 레이아웃이 깨질 수 있습니다.globalStyle에 의존합니다.이번 구현에서 역할 기반 클래스를 사용하여 위치 의존성을 제거했습니다.
각 슬롯 클래스는 서브 컴포넌트에 직접 부여되며, DOM 순서나 자식 개수와 관계없이 역할에 맞는 위치에 배치됩니다.
처음에는
gridTemplateAreas를 사용했으나 Helper가 없어도 두 번째 행이 생성되어row-gap만큼의 하단 여백이 발생했습니다. 이를 방지하기 위해gridRow기반으로 수정했습니다.📸 스크린샷
📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
✅ 셀프 체크리스트
closes #451
Summary by CodeRabbit
릴리스 노트
Root / Item / Basic / Label / Helper) 추가: 단일/그룹 선택, Disabled/Invalid 표시,indeterminate표현 지원.empty→hollow,align제거) 반영.