Skip to content

fix(Button): fix button prop inference when using as prop #131

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

Open
kyletsang wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(Button): fix button prop inference when using as prop #131

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
30 changes: 14 additions & 16 deletions src/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from 'react';
import type { DynamicRefForwardingComponent } from './types.js';

export type ButtonType = 'button' | 'reset' | 'submit';

Expand Down Expand Up @@ -108,7 +109,7 @@ export function useButtonProps({
];
}

export interface BaseButtonProps {
export interface ButtonProps extends React.ButtonHTMLAttributes<HTMLElement> {
/**
* Control the underlying rendered element directly by passing in a valid
* component type
Expand All @@ -127,21 +128,18 @@ export interface BaseButtonProps {
rel?: string | undefined;
}

export interface ButtonProps
extends BaseButtonProps,
React.ComponentPropsWithoutRef<'button'> {}

const Button = React.forwardRef<HTMLElement, ButtonProps>(
({ as: asProp, disabled, ...props }, ref) => {
const [buttonProps, { tagName: Component }] = useButtonProps({
tagName: asProp,
disabled,
...props,
});

return <Component {...props} {...buttonProps} ref={ref} />;
},
);
const Button: DynamicRefForwardingComponent<'button', ButtonProps> =
Copy link
Member

@jquense jquense Dec 8, 2025

Choose a reason for hiding this comment

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

IIRC the reason we didn't do this was to keep the type simple since button is used everywhere and the as types break or are complex in weird ways

Copy link
Collaborator Author

@kyletsang kyletsang Dec 10, 2025

Choose a reason for hiding this comment

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

It "seems" ok from cursory testing, but could be overlooking a more complex case. I can look into an alternative way to typing when I get some spare time.

Copy link
Member

@jquense jquense Dec 22, 2025

Choose a reason for hiding this comment

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

I think the question was more why does button need to allow any as since things should be using it as the base not the other way around

Copy link
Collaborator Author

@kyletsang kyletsang Dec 22, 2025

Choose a reason for hiding this comment

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

This change actually doesn't loosen the as prop to allow any component. It's still restricted to HTML elements.

The original type remains intact here:

ui/src/Button.tsx

Line 117 in afae825

as?: keyof React.JSX.IntrinsicElements | undefined;

Fix was intended to address this issue (sorry forgot to link when I created):
react-bootstrap/react-bootstrap#6941

React.forwardRef<HTMLElement, ButtonProps>(
({ as: asProp, disabled, ...props }, ref) => {
const [buttonProps, { tagName: Component }] = useButtonProps({
tagName: asProp,
disabled,
...props,
});

return <Component {...props} {...buttonProps} ref={ref} />;
},
);

Button.displayName = 'Button';

Expand Down
Loading