feat(Button): 버튼 컴포넌트의 최소 WAI-ARIA 기본 접근성 개선(#215)#244
feat(Button): 버튼 컴포넌트의 최소 WAI-ARIA 기본 접근성 개선(#215)#244osohyun0224 wants to merge 1 commit intodev/side-v2from
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
minji0214
left a comment
There was a problem hiding this comment.
asChild 사용 시 type prop이 비-button 요소에 전달되는 문제
asChild=true이면 Comp = Slot이 되고, type="button"이 자식 요소(예: <a>, <div>)에도 그대로 전달됩니다. <a> 태그에 type 속성은 유효하지 않아 React 경고 및 HTML 유효성 오류가 발생할 수 있습니다.
!asChild일 때만 type을 전달하도록 조건부 처리를 제안합니다.
| const Comp = asChild ? Slot : 'button'; | ||
| const className = cx(styles.button({ variant, size }), { [styles.disabled]: disabled }, _className); | ||
|
|
||
| return <Comp ref={ref} className={className} disabled={disabled} {...props} />; |
There was a problem hiding this comment.
asChild가 true일 때 type이 비-button 요소에도 전달됩니다. 아래처럼 조건부로 전달하면 안전합니다.
| return <Comp ref={ref} className={className} disabled={disabled} {...props} />; | |
| return <Comp ref={ref} {...(!asChild && { type })} className={className} disabled={disabled} {...props} />; |
There was a problem hiding this comment.
@minji0214 좋은 의견 감사합니다!! 다만 현재 코드에서 type은 명시적으로 설정하지 않고 ...props에 포함되어, 직접 전달하지 않는 한 문제가 발생하지 않습니다!! 또한 Radix UI의 Slot 패턴에서는 props 위임이 의도된 동작이고, type만 필터링하면 disabled 등 다른 button-specific 속성도 같은 문제를 갖게 될 것 같습니다! props 필터링이 필요하다면 전체적으로 접근해야 할 것 같아, 현재 Radix 컨벤션을 따르는 것이 적절하다고 생각했숩니다,,! 혹시 요 의견에 대해서는 어떻게 생각하시는지 궁금합니다!
There was a problem hiding this comment.
그렇네용 다른 컴포넌트들의 소스를 보니 동일하게 props를 전달하고 있네요, radix의 기본 설계를 따르는게 맞을거 같습니다! @osohyun0224
G-hoon
left a comment
There was a problem hiding this comment.
소현님 여기도 마찬가지로 changeset 추가해주세요~!
| { | ||
| variant = ButtonVariant.filled, | ||
| size = ButtonSize.lg, | ||
| type = 'button', |
There was a problem hiding this comment.
허걱쓰 default submit 이였다니 ㅎㅎ..
Changes
Button 컴포넌트에 최소 WAI-ARIA 기본 접근성을 개선했습니다.
type="button"기본값 추가: 폼 내에서 의도치 않은 submit 동작을 방지합니다. 상위 컴포넌트에서type="submit"으로 오버라이드할 수 있습니다.:focus-visible스타일 추가: 키보드 사용자가 버튼에 포커스했을 때 시각적으로 확인할 수 있도록 outline 스타일을 적용합니다.type기본값 확인 및 오버라이드 테스트Visuals
해당 없음 (CSS outline 스타일은 키보드 Tab 포커스 시에만 표시됩니다)
Checklist
Additional Discussion Points
typeprop은 기존ComponentProps<'button'>에 이미 포함되어 있으므로 별도 타입 추가 없이 기본값만 설정했습니다.