-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[TabScrollButton] Extend ButtonBase types #38719
[TabScrollButton] Extend ButtonBase types #38719
Conversation
Netlify deploy previewhttps://deploy-preview-38719--material-ui.netlify.app/ Bundle size report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me but a question came up. InternalStandardProps
was replaced and the types it provides now come from CommonProps
and ComponentPropsWithRef
. My question is, is InternalStandardProps
still useful or should we look into removing it?
I think it remains helpful for components aiming to eliminate conflicts between props and standard HTML attributes. For instance, in the Tooltip component, it prevents the While we could directly extend div attributes without StandardProps like: export interface TooltipProps extends React.HTMLAttributes<HTMLDivElement> { because style, ref, and className are already present in HTMLDivElement: <div style={{ color: 'red' }} className="root" /> the second argument of Moreover, it also eliminates the extended component's EDIT:
Looks like this is not the case because the original classes type will override the extended ones (outlined input classes will override Input Base classes), but I still think it is useful to not define dual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into my question 😊
I saw this while working on #38717. The
TabScrollButton
component usesButtonBase
, but it doesn't support its prop types.Before: https://codesandbox.io/s/great-goldwasser-kqk4sz?file=/src/App.tsx
After: https://codesandbox.io/s/strange-shannon-tdy27s
Open the codesandboxes in full screen to get the scroll buttons.