Skip to content

feat(Button): 버튼 컴포넌트의 최소 WAI-ARIA 기본 접근성 개선(#215) #244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
osohyun0224 merged 1 commit into from
Apr 24, 2026
+24 −1
Merged

feat(Button): 버튼 컴포넌트의 최소 WAI-ARIA 기본 접근성 개선(#215) #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/button/src/Button.css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ export const button = recipe({
transition: 'all 0.2s ease-in-out',
border: 'none',
fontFamily: vars.typography.fontFamily,
':focus-visible': {
outline: `2px solid ${vars.color.primary}`,
outlineOffset: '2px',
},
},
variants: {
variant: {
Expand Down
18 changes: 18 additions & 0 deletions packages/button/src/Button.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,21 @@ test('large size works correctly', () => {
expect(button).toBeInTheDocument();
expect(button.className).toBeTruthy();
});

test('type="button" is used as default when type is not provided', () => {
render(<Button>Test</Button>);

const button = screen.getByRole('button');

// Should default to type="button"
expect(button).toHaveAttribute('type', 'button');
});

test('type="submit" works correctly', () => {
render(<Button type="submit">Submit</Button>);

const button = screen.getByRole('button');

// Should allow overriding type
expect(button).toHaveAttribute('type', 'submit');
});
3 changes: 2 additions & 1 deletion packages/button/src/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export const Button = forwardRef(function Button(
{
variant = ButtonVariant.filled,
size = ButtonSize.lg,
type = 'button',
Copy link
Copy Markdown
Contributor

@froggy1014 froggy1014 Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osohyun0224

허걱쓰 default submit 이였다니 ㅎㅎ..

asChild,
disabled,
className: _className,
Expand All @@ -39,5 +40,5 @@ export const Button = forwardRef(function Button(
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} />;
Copy link
Copy Markdown

@minji0214 minji0214 Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asChild가 true일 때 type이 비-button 요소에도 전달됩니다. 아래처럼 조건부로 전달하면 안전합니다.

Suggested change
return <Comp ref={ref} className={className} disabled={disabled} {...props} />;
return <Comp ref={ref} {...(!asChild && { type })} className={className} disabled={disabled} {...props} />;

Copy link
Copy Markdown
Member Author

@osohyun0224 osohyun0224 Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minji0214 좋은 의견 감사합니다!! 다만 현재 코드에서 type은 명시적으로 설정하지 않고 ...props에 포함되어, 직접 전달하지 않는 한 문제가 발생하지 않습니다!! 또한 Radix UI의 Slot 패턴에서는 props 위임이 의도된 동작이고, type만 필터링하면 disabled 등 다른 button-specific 속성도 같은 문제를 갖게 될 것 같습니다! props 필터링이 필요하다면 전체적으로 접근해야 할 것 같아, 현재 Radix 컨벤션을 따르는 것이 적절하다고 생각했숩니다,,! 혹시 요 의견에 대해서는 어떻게 생각하시는지 궁금합니다!

Copy link
Copy Markdown

@minji0214 minji0214 Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그렇네용 다른 컴포넌트들의 소스를 보니 동일하게 props를 전달하고 있네요, radix의 기본 설계를 따르는게 맞을거 같습니다! @osohyun0224

return <Comp ref={ref} type={type} className={className} disabled={disabled} {...props} />;
});
Loading