feat: JDS Radio 컴포넌트 신규 스펙 반영 및 구조 개선#490
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughRadio의 공개 prop과 컨텍스트가 size/variant 중심으로 바뀌고, Item/Basic/Label/Helper 렌더링과 스타일이 새 스펙에 맞게 재구성되었습니다. 스토리와 SelectRadio 연동이 갱신됐고, 기존 RadioContent 프리셋은 제거되었습니다. ChangesRadio 신규 스펙 반영
Sequence Diagram(s)sequenceDiagram
participant "Radio.Root" as RadioRoot
participant "RadioConfigProvider" as RadioConfigProvider
participant "Radio.Item" as RadioItem
participant "RadioItemProvider" as RadioItemProvider
participant "Radio.Basic" as RadioBasic
participant "RadioControl" as RadioControl
participant "Radio.Helper" as RadioHelper
RadioRoot->>RadioConfigProvider: provide size, variant, disabled, state
RadioItem->>RadioItemProvider: provide labelId and helperId
RadioBasic->>RadioConfigProvider: read size and variant
RadioBasic->>RadioControl: render input and visual span
RadioHelper->>RadioItemProvider: signal helper mount change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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: 3
🤖 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 @.changeset/radio-new-spec.md:
- Around line 1-3: The changeset version bump for `@jects/jds` should remain minor
because the package is still on 0.x and Breaking Change handling follows 0.x
semver conventions; keep the frontmatter in the changeset file as minor and do
not change it to patch or major.
In `@packages/jds/src/components/Radio/Radio.tsx`:
- Line 181: The Radio input is currently allowing external props to override its
semantic type, which can break it by rendering as something other than a radio.
Update RadioBasicProps to omit type as well, and in the Radio component’s
mergeProps/inputProps composition ensure the internal type: "radio" is applied
last so grouped and standalone paths both cannot be overridden by restProps or
inputProps.
- Around line 125-130: `Radio`의 input props 병합에서 `aria-labelledby`와
`aria-describedby`를 무조건 덮어쓰는 문제가 있습니다. `Radio.Item` 내부의 `Radio.Label` 존재 여부를
추적하는 상태(예: `hasLabel`)를 `Radio.Label`/`hasHelper`와 같은 방식으로 관리해 `labelId`는 실제 라벨이
mount된 경우에만 넣고, `inputProps`에 이미 들어온 ARIA id는 유지하면서 생성한 id와 병합하도록 `Radio` 컴포넌트의
input 렌더링 로직을 수정하세요.
🪄 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: 335a8ed8-b96c-4bf1-8753-48e3dde4b08a
📒 Files selected for processing (11)
.changeset/radio-new-spec.mdpackages/jds/src/components/Radio/Radio.stories.tsxpackages/jds/src/components/Radio/Radio.tsxpackages/jds/src/components/Radio/RadioContext.tsxpackages/jds/src/components/Radio/index.tspackages/jds/src/components/Radio/preset/RadioContent.stories.tsxpackages/jds/src/components/Radio/preset/RadioContent.tsxpackages/jds/src/components/Radio/preset/radioContent.types.tspackages/jds/src/components/Radio/radio.css.tspackages/jds/src/components/Radio/radio.types.tspackages/jds/src/components/Select/SelectRadio.tsx
💤 Files with no reviewable changes (3)
- packages/jds/src/components/Radio/preset/RadioContent.tsx
- packages/jds/src/components/Radio/preset/radioContent.types.ts
- packages/jds/src/components/Radio/preset/RadioContent.stories.tsx
| --- | ||
| "@jects/jds": minor | ||
| --- |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: `@jects/jds` 현재 버전과 changeset 정책 근거를 확인합니다.
fd -a 'package.json' . | xargs -I{} sh -c 'printf "\n--- %s ---\n" "$1"; jq -r ".name, .version" "$1" 2>/dev/null' sh {}
rg -n -C2 '"`@jects/jds`"|changeset|semver|major|minor|breaking' README.md .changeset package.json .github 2>/dev/nullRepository: JECT-Study/JECT-Official-WebSite-Client
Length of output: 6525
Breaking Change 에 대해 minor 로 설정하신 것이 적절합니다.
현재 @jects/jds 는 0.4.0 버전 (0.x 대) 이며, 시맨틱 버저닝 표준에 따라 0.x 버전에서는 Breaking Change 를 minor 증가로 처리하는 것이 일반적입니다.
---
"`@jects/jds`": minor
---🤖 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 @.changeset/radio-new-spec.md around lines 1 - 3, The changeset version bump
for `@jects/jds` should remain minor because the package is still on 0.x and
Breaking Change handling follows 0.x semver conventions; keep the frontmatter in
the changeset file as minor and do not change it to patch or major.
| <input | ||
| {...inputProps} | ||
| ref={inputRef} | ||
| aria-labelledby={isWithinItem ? labelId : undefined} | ||
| aria-describedby={describedById} | ||
| className={radioInput} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
aria-labelledby를 무조건 덮어쓰지 마세요.
Radio.Item 안에 Radio.Label이 없으면 Line 128이 존재하지 않는 id를 참조하고, wrapping <label>의 native label 계산도 aria-labelledby에 의해 가려질 수 있습니다. 소비자가 넘긴 aria-describedby도 Line 129에서 덮어써집니다. Label mount 여부를 추적하거나, 기존 ARIA id와 생성 id를 병합해 주세요.
수정 방향 예시
- aria-labelledby={isWithinItem ? labelId : undefined}
- aria-describedby={describedById}
+ aria-labelledby={
+ [inputProps["aria-labelledby"], isWithinItem ? labelId : undefined]
+ .filter(Boolean)
+ .join(" ") || undefined
+ }
+ aria-describedby={
+ [inputProps["aria-describedby"], describedById].filter(Boolean).join(" ") ||
+ undefined
+ }labelId는 실제 Radio.Label이 mount된 경우에만 전달되도록 hasLabel도 hasHelper와 같은 방식으로 관리하는 편이 안전합니다.
📝 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.
| <input | |
| {...inputProps} | |
| ref={inputRef} | |
| aria-labelledby={isWithinItem ? labelId : undefined} | |
| aria-describedby={describedById} | |
| className={radioInput} | |
| <input | |
| {...inputProps} | |
| ref={inputRef} | |
| aria-labelledby={ | |
| [inputProps["aria-labelledby"], isWithinItem ? labelId : undefined] | |
| .filter(Boolean) | |
| .join(" ") || undefined | |
| } | |
| aria-describedby={ | |
| [inputProps["aria-describedby"], describedById].filter(Boolean).join(" ") || | |
| undefined | |
| } | |
| className={radioInput} |
🤖 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/Radio/Radio.tsx` around lines 125 - 130, `Radio`의
input props 병합에서 `aria-labelledby`와 `aria-describedby`를 무조건 덮어쓰는 문제가 있습니다.
`Radio.Item` 내부의 `Radio.Label` 존재 여부를 추적하는 상태(예: `hasLabel`)를
`Radio.Label`/`hasHelper`와 같은 방식으로 관리해 `labelId`는 실제 라벨이 mount된 경우에만 넣고,
`inputProps`에 이미 들어온 ARIA id는 유지하면서 생성한 id와 병합하도록 `Radio` 컴포넌트의 input 렌더링 로직을
수정하세요.
| size={size} | ||
| interaction={interaction} | ||
| inputRef={ref} | ||
| inputProps={mergeProps(inputProps, restProps, { onChange })} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
외부 props가 radio input의 type을 바꾸지 못하게 막아 주세요.
RadioBasicProps가 type을 허용하고, grouped/standalone 병합에서 외부 props가 내부 radio 속성보다 뒤에 적용될 수 있어 type='checkbox' 같은 값이 실제 input semantics를 깨뜨릴 수 있습니다. 타입에서 type을 제외하고 렌더링에서도 type: "radio"가 마지막에 보장되게 해 주세요.
수정 방향 예시
- inputProps={mergeProps(inputProps, restProps, { onChange })}
+ inputProps={mergeProps(restProps, inputProps, { onChange, type: "radio" })}- inputProps={{
- type: "radio",
+ inputProps={{
+ ...restProps,
value,
checked,
disabled: isDisabled,
onChange,
name,
- ...restProps,
+ type: "radio",
}}그리고 RadioBasicProps에서도 type을 Omit 대상에 추가해 주세요.
Also applies to: 224-231
🤖 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/Radio/Radio.tsx` at line 181, The Radio input is
currently allowing external props to override its semantic type, which can break
it by rendering as something other than a radio. Update RadioBasicProps to omit
type as well, and in the Radio component’s mergeProps/inputProps composition
ensure the internal type: "radio" is applied last so grouped and standalone
paths both cannot be overridden by restProps or inputProps.
Zero-1016
left a comment
Zero-1016
left a comment
There was a problem hiding this comment.
큰 변경사항은 없는 것을 확인하였습니다. Checkbox와 동일한 구조로 변했네요.
코멘트 하나만 확인 부탁드립니다. 🙏
| export interface RadioBasicProps extends Omit<ComponentPropsWithoutRef<"input">, "value"> { | ||
| radioSize?: RadioSize; | ||
| export interface RadioBasicProps | ||
| extends Omit<ComponentPropsWithoutRef<"input">, "value" | "size"> { |
There was a problem hiding this comment.
Radio와 Checkbox 둘 다, type으로 override가 가능한 것을 확인하였어요. 해당 props를 받지 않도록 해야할 것 같아요~
<Radio.Basic value='lg' size='lg' type='radio' />There was a problem hiding this comment.
확인했습니다~ 두 컴포넌트 모두 수정해야 하는 사항이라 머지된 후에 묶어서 별도 작업으로 진행하겠습니다 🙂
3 participants
💡 작업 내용
Radio.Basicref 미연결 수정💡 자세한 설명
1. 개요
Radio는 이미 vanilla-extract 기반으로 마이그레이션되어 있습니다. 이번 PR에서는 신규 디자인 스펙을 반영하고,Checkbox에서 정립한 패턴에 맞춰 API와 내부 구조를 정비했습니다.구조, 접근성, 슬롯 배치 등의 설계 배경은
CheckboxPR(#462)과 동일합니다.2. API 변경 사항
prop 이름/값, public 타입, 보조 텍스트 컴포넌트가 변경됩니다.
radioSize/radioStylesize/variantradioStyle="empty" | "outline"variant = "hollow" | "outlined"radioAlign/RadioAlignRadio.SubLabelRadio.HelperRadioStyle/RADIO_STYLE_OPTIONSRadioVariant/RADIO_VARIANT_OPTIONSRadioSubLabelPropsRadioHelperProps또한 패키지 외부로 노출되지 않던
RadioContent프리셋을 제거했습니다.3. 구조 개선
다음 사항들에 대해
Checkbox와 동일한 구조를 적용했습니다.Radio.Item을<label>로 전환하여 인디케이터뿐 아니라 라벨/헬퍼 텍스트를 클릭해도 선택할 수 있도록 했습니다.aria-labelledby, 헬퍼 텍스트는aria-describedby로 input과 연결했습니다.interactionvariant로 분리해 컨테이너가 인터랙션을 담당하도록 정리했습니다. 이에 따라.visual리터럴 의존성과 focus 억제를 위한globalStyle을 제거했습니다.globalStyle(:nth-child)대신 역할 기반 슬롯 클래스로 관리하도록 변경했습니다.getLabelClassName으로 통일했습니다.Radio.Label,Radio.Helper를<span>으로 변경하고, 컨텍스트를config와item으로 분리했습니다.4. 신규 디자인 스펙 반영
not-allowed커서를 적용했습니다.5. 버그 수정
그룹 모드에서
Radio.Basic이useObjectRef로 생성한 ref를 실제<input>에 연결하지 않던 문제를 수정했습니다. 이제forwardRef로 전달한 ref가 input DOM 요소를 정상적으로 참조합니다.📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
✅ 셀프 체크리스트
closes #489
Summary by CodeRabbit
새 기능
버그 수정
문서/예시