refactor: JDS Radio 컴포넌트 vanilla-extract 마이그레이션#437
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
f3850e5 to
0ef9a70
Compare
WonJuneKim
left a comment
There was a problem hiding this comment.
수정 사항 확인 했습니다! useContainerPresable의 경우 Checkbox 작업 시에 활용할 수 있을 거 같네요. 간단한 코멘트 한번 확인 부탁드립니다!
| overlay, | ||
| focusRing, | ||
| { | ||
| position: "relative", | ||
| vars: { [overlayColor]: vars.color.semantic.interaction.normal }, | ||
| selectors: { | ||
| // checked 상태에서 overlay 색상을 accent으로 전환 | ||
| '&:has(input[type="radio"]:checked)': { | ||
| vars: { [overlayColor]: vars.color.semantic.accent.neutral }, | ||
| }, | ||
| // overlay shape: element 경계와 동일. empty 모드에서는 compoundVariants로 확장 | ||
| "&::after": { inset: 0, borderRadius: "inherit" }, | ||
| // focus ring shape: element 경계와 동일. empty 모드에서는 compoundVariants로 확장 | ||
| "&::before": { inset: 0, borderRadius: "inherit" }, |
There was a problem hiding this comment.
해당 부분은 interation 정책을 잘 따르고 있는 구간인거 같아요 :)
| radioAlign?: RadioAlign; | ||
| disabled?: boolean; | ||
| value?: string; | ||
| defaultValue?: string; |
There was a problem hiding this comment.
해당 컴포넌트를 처음 구현할때에는 비제어로 사용하는 부분을 배제했었는데 완성도가 더 올라간거 같습니다.
There was a problem hiding this comment.
다만 제어 형태와 비제어 형태를 동시에 사용하는 케이스가 없기 때문에 추가로 narrowing 할 수 있는 부분인거 같아요.
| */ | ||
| export function useContainerPressable({ disabled = false }: UseContainerPressableOptions = {}) { | ||
| const { hoverProps, isHovered } = useHover({ isDisabled: disabled }); | ||
| const { pressProps, isPressed } = usePress({ isDisabled: disabled }); |
There was a problem hiding this comment.
useRadio가 노출하는 isPressed 값을 사용하지 않고 usePress 로 가져온 이유가 있으신가요?
There was a problem hiding this comment.
useRadio가 isPressed를 제공하는 것은 맞지만 아래 이유로 별도의 usePress를 사용했습니다
-
useRadio는 내부 컴포넌트인RadioBasicGrouped에서 호출되고,data-pressed가 필요한 요소는 부모인RadioItem의 div입니다.isPressed를 상위로 전달하려면 컨텍스트나 prop drilling이 필요해 구조가 복잡해질 것으로 생각했습니다. -
useContainerPressable은usePressable에 대응되는 컨테이너용 범용 훅으로 설계했습니다.Radio뿐 아니라Checkbox처럼 native form 요소를 감싸는 다양한 컨테이너 컴포넌트에서 재사용할 수 있도록 특정 훅에 의존하지 않도록 했습니다.
| : radioStyle === "outline"; | ||
| const parentContext = useRadioContext(); | ||
|
|
||
| const size = radioSize ?? parentContext?.size ?? "md"; |
There was a problem hiding this comment.
(참고) 예측가능성면에서 내부 자체 prop 과 context 의 prop을 함수 등으로 추출할 수 있을 거 같아요.
RadioItem(현재 부분) 에서는 직접 선언한 prop이 우선시 되고 Basic에서는 Context 우선이 되기 때문에 생각하기에 따라서 일관성이 떨어질 수 있을 거 같아요.
다만 저 역시 Basic이 당연히 Context를 따를 거라는 생각은 가지고 있어서 단순 코멘트로 남깁니다!
암묵적으로 이해하고 있는 Item 내에서 선언한 prop과 Context 내부 공통 prop 값, 그리고 선언되지 않았을 때 할당되는 default 값을 밖으로 드러내도 좋을 거 같습니다.
There was a problem hiding this comment.
우선 해당 우선 순위 차이는 말씀해 주신 것처럼 의도한 동작이 맞습니다
RadioItem은 prop > context > default 순으로 값을 결정한 뒤 하위 컨텍스트로 전달하고, RadioBasic은 항상 상위에서 최종 결정된 값을 따르도록 context > prop > default 순으로 처리했습니다. (그래서 개별 항목의 size 변경은 RadioBasic보다는 RadioItem에서 처리해야 합니다)
하지만 말씀 주신 것처럼 코드만 봐서는 의도가 암묵적으로 느껴질 수 있을 것 같아 언급해주신 방법을 포함해서 개선 방향을 고민해보겠습니다 🙂
ccconac
left a comment
There was a problem hiding this comment.
확인하면서 질문 및 문제가 될 수 있을 만한 부분 코멘트 남겨 두었습니다~
간단하게 확인 부탁드려요. 작업하시느라 수고 많으셨습니다! 👏
| > | ||
| {children} | ||
| </RadioGroupProvider> | ||
| <div {...radioGroupProps} style={{ display: "contents" }}> |
There was a problem hiding this comment.
display: "contents"를 className이 아닌 inline style로 작성하신 특별한 이유가 있을까요?
There was a problem hiding this comment.
단순한 스타일링이라 별다른 의도 없이 인라인으로 작성했던 것 같습니다..😅 8cbeaf3 에서 VE className 기반으로 교체했습니다
| <RadioBasicGrouped | ||
| size={size} | ||
| value={String(value ?? "")} | ||
| isDisabled={isDisabled} | ||
| state={context.state} | ||
| forwardedRef={forwardedRef} | ||
| restProps={restProps} | ||
| /> |
There was a problem hiding this comment.
(참고) line:117에서 onChange prop을 받고 있는데, 그룹으로는 전달되지 않고 있어서 Radio.Basic 개별 onChange가 호출되지 않을 수 있다는 이슈가 있어 보여요.
삭제된 기존 코드를 보면:
- 그룹
onChange호출 - 개별 input
onChange호출
의 흐름이라 기존 사용처의 side effect가 누락될 수 있어 보여서 검토 부탁드립니다!
const handleChange = (e: ChangeEvent<HTMLInputElement>) => {
if (groupContext?.onChange && value !== undefined) {
groupContext.onChange(String(value));
}
onChange?.(e);
};There was a problem hiding this comment.
확인해 보니 실제로 그룹 케이스에서 onChange 전달이 누락된 상태였네요 😮 구조분해에서 onChange가 restProps에서 빠져 나왔는데, 이후 RadioBasicGrouped로 전달되지 않아 실제로 호출되지 않고 있었습니다
RadioBasicGroupedProps에 onChange를 추가하고, mergeProps(inputProps, restProps, { onChange }) 형태로 내부 핸들러와 체이닝되도록 수정했습니다! (9946e13)
| ({ radioSize, value, checked, disabled, onChange, name, ...restProps }, forwardedRef) => { | ||
| const context = useRadioContext(); | ||
| const size = context?.size ?? radioSize ?? "md"; | ||
| const isDisabled = disabled ?? context?.disabled ?? false; |
There was a problem hiding this comment.
Root/Item 둘 중 하나가 disabled면 하위 요소가 모두 disabled 된다는 조건문이 line:64에 작성되어 있는데, 이 부분은 앞서 말한 조건을 해제할 수 있어 보여요.
// line:64 → Radio.Item이 disabled거나 parentContext(Radio.Root)가 disabled일 경우 true
const isDisabled = disabled || (parentContext?.disabled ?? false);
// line:120 → 상위 context가 disabled여도 Basic에서 false 지정할 경우 isDisabled: false가 됨
// false ?? true ?? false → false
const isDisabled = disabled ?? context?.disabled ?? false;전체적으로 비활성화 상태인데 혼자 활성화가 되는 등, 상위 컨텍스트를 무시할 수 있어 보여서 아래처럼 교체되어야 할 것 같습니다.
const isDisabled = disabled || (context?.disabled ?? false);| return ( | ||
| <RadioBasicGrouped | ||
| size={size} | ||
| value={String(value ?? "")} |
There was a problem hiding this comment.
(참고) 타입을 확인해 보니 RadioBasicProps에서 value가 optional이라 value가 누락된 item들이 동일한 value로 등록될 수 있어 보여요. 예외를 막기 위해 required로 제한하는 등의 guard가 필요할 듯합니다.
예외적 상황이 발생할 수 있어 참고 코멘트로 남깁니다!
There was a problem hiding this comment.
마이그레이션 전에는 value가 undefined인 경우 onChange를 호출하지 않는 방식으로 처리하고 있었는데요, 기존 스토리 파일에서도 value 없이 사용되고 있었고, 해당 케이스들이 정상 동작하는 것처럼 보여 value가 optional이어도 괜찮다고 생각했던 것 같습니다
이후 마이그레이션 과정에서 useRadio가 value를 필수값으로 요구하면서, optional인 value를 처리하기 위해 String(value ?? "")로 처리했었습니다.
8158441 에서 value를 필수값으로 강제하고, 스토리 파일에도 value를 추가했습니다!
💡 작업 내용
Radiovanilla-extract 마이그레이션@react-aria/radio도입 (그룹 접근성, 화살표키 내비게이션)useContainerPressable훅 분리💡 자세한 설명
1. 개요
Radio의 스타일링 레이어를 Emotion에서 vanilla-extract로 마이그레이션했습니다. 기존 Emotion 파일을 제거하고 동일 디렉토리에 VE 구현체로 대체했습니다.Radio/ ├── Radio.style.ts ← Emotion (삭제) ├── RadioGroupContext.tsx ← @react-aria/radio 도입으로 제거 ├── radio.css.ts ← VE 스타일 (신규) ├── Radio.tsx ← VE 기반 구현으로 교체 ├── RadioContext.tsx ← 단일 컨텍스트로 통합 ├── radio.types.ts ← Emotion 전용 타입 제거 ├── Radio.stories.tsx ← 리팩토링 ├── index.ts └── preset/ ├── RadioContent.tsx └── radioContent.types.ts2.
useContainerPressable훅 도입Radio는 버튼 컴포넌트들과 달리usePressable을 그대로 적용하기 어려웠습니다.usePressable은 다음 조합으로 구성됩니다.1)
useButton사용 불가useButton은 대상 element에role="button"과 Enter/Space 키 인터랙션을 추가합니다. 하지만Radio.Item은 실제 인터랙션 주체가 내부<input type="radio">인 컨테이너 구조입니다. 이 상태에서 바깥<div>에 button 시맨틱까지 부여하면, native radio와 button 역할이 동시에 존재하게 되어 스크린리더가 올바르게 해석하지 못할 수 있고, 키보드 입력 역시 컨테이너와 input 양쪽에서 처리될 수 있습니다.따라서
Radio.Item에는useButton을 적용하지 않고,usePress(@react-aria/interactions)를 사용했습니다.usePress는useButton과 달리 role이나 tabIndex를 주입하지 않고 pressed 상태만 제공합니다.2)
useFocusRing({ within: true })사용Radio.Item자체는 포커스를 받지 않고 내부<input>이 포커스를 받습니다. 기존usePressable내부의useFocusRing()은within: true없이 호출되므로,Radio.Item에서는isFocusVisible이 항상false였습니다.이를 위해
useFocusRing({ within: true })를 사용했습니다. 자식 input에 포커스가 이동해도 컨테이너에서 focus-visible 상태를 감지할 수 있습니다.해당 훅 조합은
usePressable구조를 참고해useContainerPressable로 분리했습니다.Checkbox처럼 내부에 native form 요소를 포함하는 컨테이너 컴포넌트에서도 동일하게 재사용할 수 있습니다.3. 인터랙션 상태 처리 방식 변경
Interaction())RadioItem):hover::afteruseHover→data-hoveredRadioItem):active::afterusePress→data-pressedRadioItem):focus-visible(실제 미동작)useFocusRing({ within: true })radioVisual):hover::after:hover유지radioVisual):active::after:active유지radioVisual):focus-visibleinput:focus-visible + .visual기존
Radio.Item의:focus-visible스타일은 실제로 포커스를 받지 않는 요소에 선언되어 있어 동작하지 않는 상태였고,useFocusRing({ within: true })기반으로 교체했습니다.radioVisual의 hover 상태는 단순 장식 요소(aria-hidden="true")이므로, 별도 상태 관리 없이 CSS:hover를 유지합니다.4. 컨텍스트 구조 단일화
기존 구현은 두 개의 컨텍스트가 중첩된 구조였습니다.
Radio.Item이 상위 컨텍스트를 읽어 다시 하위 컨텍스트로 전달하는 구조였으며, 실제 사용 값도 중복되어 있어 하나의 컨텍스트로 통합했습니다.Radio.Root가 전체 값을 제공하고,Radio.Item은 부모 컨텍스트를 읽어 자신의 props와 합쳐 override하는 방식으로 정리했습니다.createContext<RadioContextValue | null>(null)로 기본값을 두어 단독 사용 여부를 컨텍스트 존재 여부로 구분할 수 있도록 했습니다.5.
radioSizeMap타입 정리 및 토큰화기존 구현에서는 동일한 맵 내부에 숫자와 문자열 타입이 혼재되어 있었습니다.
radioSize는 런타임 계산용 숫자였고,border는 실제로는 토큰 키였지만 string으로 표현되고 있어 모두 최종 사용 형태 기준으로 통일했습니다.6. overlay 확장 영역 처리
Radio.Item의empty스타일에서는::after(overlay)와::before(focus ring)가 element 경계 바깥으로 확장됩니다.현재
LabelButton에서는 확장 영역을inset기반으로 표현하고 있어,Radio역시 동일한 방식으로 통일했습니다.기존
Math.floor(w / 2) + 1계산은 왼쪽 영역이 1px~2px 더 확장되어 비대칭을 만들고 있었습니다. 다만 해당 오차가 의도된 구현이었는지는 명확하지 않고, 피그마에서도 관련 기준을 확인할 수 없어 우선 대칭으로 수정해 두었습니다.7. VE selector 제약 대응
VE의
selectors는 내부에서 현재 클래스(&) 바깥의 임의 클래스 선택을 제한합니다. 초기 구현에서 다음 패턴들이 이 규칙을 위반했습니다.1) 부모에서 형제 요소 타겟
'& input[type="radio"]:checked + .visual'.visual이 최종 타겟이므로 VE 런타임 에러가 발생합니다. 이를 해결하기 위해 선택자를radioVisual내부로 이동했습니다.'input[type="radio"]:checked + &'2) recipe 내부에서 자식 selector 직접 타겟
"& > :nth-child(1)"이 역시 VE 제약에 걸리므로, 자식 레이아웃은
globalStyle로 분리했습니다.그리드 정렬도
style()클래스 +globalStyle조합으로 분리했습니다.8. disabled 색상 전파 방식 변경
$isDisabledprop을 직접 전달해 텍스트 색상을 변경하던 로직을 부모의data-disabled상태를 기준으로 변경하여 prop drilling 없이 disabled 상태가 하위 텍스트까지 전파되도록 했습니다.9.
@react-aria/radio도입접근성과 그룹 상태 관리는
@react-aria/radio+react-stately에 위임했습니다. 버튼 컴포넌트가@react-aria/button의useButton기반으로 접근성을 처리하므로Radio역시 react-aria 훅 기반으로 상태 관리와 접근성 처리를 가져가도록 했습니다.1)
Radio.Root기존에는 래퍼 요소가 없었지만,
radioGroupProps적용을 위해 래퍼 div를 추가했습니다. 해당 요소가 ARIA 속성은 DOM에 유지하면서, 레이아웃을 방해하지 않도록display: contents를 적용했습니다.2)
Radio.BasicuseRadio는RadioGroupState를 필수로 요구하므로 그룹/단독 사용을 분기했습니다. 이를 통해 그룹 내부에서는checked,name,aria-*,onChange등이 자동으로 관리됩니다.10. 접근성 개선 사항
role="radiogroup"자동 적용data-disabled수동 처리aria-disabled자동 적용useRadioGroupState기반이제
Radio.Root내부에서는checkedprop을 직접 사용할 수 없습니다. 선택 상태는RadioGroupState기준으로 관리됩니다.📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
✅ 셀프 체크리스트
closes #433