Skip to content

Conversation

@I-lost-everytime
Copy link

Improves accessibility semantics for ResponsiveButton.

  • Adds accessible labeling for icon-only buttons on smaller screens
  • Ensures aria-disabled is set when the button is disabled
  • Marks decorative icons as aria-hidden

This keeps visual behavior unchanged while improving keyboard and
screen-reader support, especially when asChild is used.

{...props}
variant={variant}
asChild={asChild}
disabled={disabled}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should add these additional props - they should be handled by shadcn (our component is just a wrapper for a shadcn button).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the clarification, that makes sense 👍

I agree that accessibility semantics should live in the underlying shadcn Button rather than being duplicated in a wrapper component. Since ResponsiveButton is intentionally thin, adding extra props here could introduce inconsistencies over time.

I’m happy to:

revert the additional accessibility props in this PR, or

alternatively explore improvements directly in the shadcn button usage/config if that’s preferred.

Let me know which direction you’d like — thanks for the guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants