Skip to content

a11y: improve disabled and labeling support in ResponsiveButton #1992

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

Closed
I-lost-everytime wants to merge 1 commit into from
Closed

a11y: improve disabled and labeling support in ResponsiveButton #1992

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
10 changes: 7 additions & 3 deletions frontend/src/components/ResponsiveButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Button } from "./ui/button";
import { buttonVariants } from "./ui/buttonVariants";

type Props = {
icon: React.ComponentType;
icon: React.ComponentType<{ className?: string }>;
text: string;
} & React.ComponentProps<"button"> &
VariantProps<typeof buttonVariants> & {
Expand All @@ -19,11 +19,12 @@ const ResponsiveButton = ({
asChild,
children,
className,
disabled,
...props
}: Props) => {
const content = (
<>
<Icon />
<Icon aria-hidden="true" />
<span className="hidden lg:inline">{text}</span>
</>
);
Expand All @@ -33,9 +34,12 @@ const ResponsiveButton = ({
{...props}
variant={variant}
asChild={asChild}
disabled={disabled}
Copy link
Contributor

@rolznz rolznz Dec 23, 2025

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

@I-lost-everytime I-lost-everytime Jan 2, 2026

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.

Copy link
Contributor

@rolznz rolznz Jan 7, 2026

Choose a reason for hiding this comment

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

Hi, yes any accessibility improvements that should be fixed inside the shadcn UI directly, it would be better to do a PR there, not here

Copy link
Author

@I-lost-everytime I-lost-everytime Jan 10, 2026

Choose a reason for hiding this comment

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

Hi, yes any accessibility improvements that should be fixed inside the shadcn UI directly, it would be better to do a PR there, not here

Thanks for the clarification — that makes sense.

Since ResponsiveButton is intentionally a thin wrapper around shadcn, I agree accessibility semantics should live in the underlying shadcn Button rather than being reimplemented here.

I’ll close this PR and follow up with accessibility improvements at the correct layer if helpful.

Appreciate the guidance

Copy link
Contributor

@rolznz rolznz Jan 11, 2026

Choose a reason for hiding this comment

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

Sounds great, thank you!

aria-disabled={disabled}
aria-label={text}
className={cn(
className,
"max-lg:size-9" /* apply size="icon" only for mobile */
"max-lg:size-9" /* apply size=\"icon\" only for mobile */
)}
>
{asChild
Expand Down