-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[Button][base] Drop component
prop
#36677
Conversation
Netlify deploy preview
Bundle size report |
@@ -165,6 +165,10 @@ function testSlotsProp(element: React.ReactElement, getOptions: () => UnstyledCo | |||
}); | |||
|
|||
it('uses the component provided in the `component` prop when both `component` and `slots.root` are provided', () => { | |||
if (skip && skip.indexOf('componentProp') >= 0) { |
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.
This test should be run only if we are not skipping the component
prop test.
/** | ||
* @ignore | ||
*/ | ||
onBlur: PropTypes.func, |
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.
@michaldudak I think we should go the other way around and explicitly define these props in the ButtonUnstyledProps interface, as if a custom component is used, that does not have these props in the props interface, there will be a typescript error, which would be a bug in my opinion.
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.
I'm not sure if I understand you correctly. Why would there be a TS error? We don't actually use these props anywhere. Also in ButtonUnstyled.spec.tsx we have a case with CustomComponent
that don't have these defined and it works fine.
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.
We don't have with custom component. For e.g. this is failing:
<ButtonUnstyled<typeof CustomComponent>
slots={{ root: CustomComponent }}
stringProp="test"
numberProp={0}
onClick={() => {}}
/>
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.
OK, I get it now. It's an interesting problem. The ButtonUnstyled
doesn't handle (or require) the click (or several other) events, so strictly speaking, it's unnecessary. It's necessary in the slots.root
, though, but there is no way of enforcing it.
In other words, even if we had onClick
in ButtonUnstyledProps
, it would be possible to provide CustomComponent
that ignores its onClick
prop and effectively create a broken button.
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.
Here is a failing test: https://app.circleci.com/pipelines/github/mui/material-ui/95773/workflows/1ec2c63d-7103-4340-b894-909bf873b7a0/jobs/509209
I think we should update the types to add these event handlers.
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.
A couple of thoughts I have about this:
- How do we decide which events to define explicitly?
- If they are defined explicitly, we will lose polymorphism in their parameters. We'll have to type them as either
MouseEventHandler<Element>
orMouseEventHandler<HTMLButtonElement>
. In either case, another test will fail (the last one from the .spec suite). - If we use this pattern, we should also update all the other components
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.
I see your point. I've added ts-expect-error directive on the new test, so that in the future, if we decide to handle this, we have some record that it was an intended decision. We can discuss it separately tough in the future.
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.
I wonder if the ButtonUnstyled definition shouldn't be function ButtonUnstyled(props: ButtonUnstyledProps, forwardedRef: React.ForwardedRef<any>)
(without the generics). This way, we get strongly typed props inside the component definition. Nothing would change with the exported definition as we cast it to PloymorphicComponent anyway.
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.
I like the explicitness that we have now. We can open a separate issue for this and see what other things I guess. It's anyway internal change
…ova/material-ui into base/remove-component-prop
@samuelsycamore do we want to still keep the "Unstyled" in the title for the base pages, for e.g. https://deploy-preview-36677--material-ui.netlify.app/base/react-button/ vs https://mui.com/base/react-button/. I would vote for keeping only Button, as we are talking about the concept of button that is implemented trough component and hook. |
@@ -682,22 +685,25 @@ export default async function generateComponentApi( | |||
// eslint-disable-next-line no-console | |||
console.log('Built API docs for', reactApi.name); | |||
|
|||
const normalizedApiPathname = reactApi.apiPathname.replace(/\\/g, '/'); | |||
const noramlizedFilename = reactApi.filename.replace(/\\/g, '/'); |
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.
const noramlizedFilename = reactApi.filename.replace(/\\/g, '/'); | |
const normalizedFilename = reactApi.filename.replace(/\\/g, '/'); |
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.
addressed
The same applies for props specific to custom primitive elements: | ||
|
||
```tsx | ||
<Button<'svg'> viewBox="0 0 24 24" /> |
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.
It should be:
<Button<'svg'> viewBox="0 0 24 24" /> | |
<Button<'svg'> slots={{ root: 'svg' }} viewBox="0 0 24 24" /> |
but technically, this won't render anything as the svg should have something inside.
Perhaps an example with another element would work better, such as:
<Button<'svg'> viewBox="0 0 24 24" /> | |
<Button<'img'> slots={{ root: 'img' }} src="button.png" /> |
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.
addressed
@hbjORbj, you may have your TS definitions cached from another branch (the Button definition says it's an |
@michaldudak Yes it's gone ;) thanks |
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.
looks good!
Agreed, I think it's better to drop "Unstyled" from the name wherever it comes up. From a branding perspective, I'd much rather teach our users to call it a "Base UI Button" than an "Unstyled Button." |
docs/data/base/guides/overriding-component-structure/overriding-component-structure.md
Outdated
Show resolved
Hide resolved
docs/data/base/guides/overriding-component-structure/overriding-component-structure.md
Outdated
Show resolved
Hide resolved
component
propcomponent
prop
Signed-off-by: Benny Joo <sldisek783@gmail.com>
I made the final touches. Will merge it once CIs all pass. |
This PR removes the
component
prop from theButtonUnstyled
component. The motivation for this is that there is a duplicated API for setting the root slot (through thecomponent
prop and through theslots.root
prop). The difference between these two APIs is that thecomponent
prop makes the component polymorphic, which means that you can send the root component's props on theButtonUnstyled
component directly, without getting typescript issues. However, this has shown to have a big performance implications when doing the type checks (see some existing issues for the Material UI's components). One of #36679BREAKING CHANGE
It will be covered by codemod, see #36831
For Base UI components, the
component
prop value should be moved toslots.root
:If using TypeScript, you should add the custom component type as a generic on the
Button
component.