Skip to content
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.

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

feat(Button): allow to pass any valid <button> or <a> HTML attributes #1594

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

amje
Copy link
Contributor

@amje amje commented May 20, 2024

This PR introduces an easier way to pass any extra properties to the component and deprecates extraProps prop:

// Before
<Button extraProps={{onKeyDown: () => {}}} />

// After
<Button onKeyDown={() => {}} />

By default, extra properties extend React.ButtonHTMLAttributes and when href property is passed they extend React.AnchorHTMLAttributes.

PR potentially will break types because ButtonProps can no longer be extended in interface declaration:

// Can't extend in interface
interface MyButton extends ButtonProps {
    myProp: string;
}

// This works fine
type MyButton = ButtonProps & {
    myProp: string;
}

@gravity-ui-bot
Copy link
Contributor

Playwright Test Component is ready.

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@amje amje force-pushed the improve-button-types branch from 3569fb0 to a7576c6 Compare May 20, 2024 00:52
@@ -125,64 +122,81 @@ const ButtonWithHandlers = React.forwardRef<HTMLElement, ButtonProps>(function B
view,
},
});

if (onClickCapture) {
onClickCapture(event as any);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is type cast used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It requires weird type, intersection of HTMLButtonElement and HTMLAnchorElement

return (
<button {...props} style={{boxShadow: '2px 2px 2px 2px deepskyblue'}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why element type changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To ensure any root tag is valid

@goshander
Copy link
Member

@amje aria-label props pass to React component as is, without camelCase converting?

@amje
Copy link
Contributor Author

amje commented May 20, 2024

@amje aria-label props pass to React component as is, without camelCase converting?

Yes, it's valid

@amje amje added this to the V7 milestone Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants