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

[Paper] Typing error when trying to pass component to PaperProps of Drawer #32404

Closed
wants to merge 4 commits into from

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Apr 21, 2022

Closes #27703
Closes #32392

Problem:

  • the interface PaperProps doesn't have component as a key
  • e.g., <Drawer PaperProps={{ component:"..." }} ... /> displays a Typescript error
  • e.g., <Dialog PaperProps={{ component:"..." }} ... /> displays a Typescript error

Before:

After:

@hbjORbj hbjORbj added bug 🐛 Something doesn't work component: drawer This is the name of the generic UI component, not the React module! component: Paper This is the name of the generic UI component, not the React module! typescript labels Apr 21, 2022
@mui-bot
Copy link

mui-bot commented Apr 21, 2022

No bundle size changes

Generated by 🚫 dangerJS against 75999c7

@jussirantala
Copy link

jussirantala commented Apr 21, 2022

This is the type of Paper component:
OverridableComponent<PaperTypeMap>

OverridableComponent is using OverrideProps to do some magic to add the props of the component to the typings. So for example if you had component="form", the Paper component would have React.FormEventHandler<HTMLFormElement> type for the onSubmit prop. You can test that with the Paper component.

What I'm saying is that this isn't probably how it should be fixed but I'm not advanced enough in TS to do a PR myself.

@mnajdova
Copy link
Member

Note that, this pattern is used in all components that support the component prop. At large, if we want to fix this problem, we would need to update the OverrideProps type. cc @michaldudak as this is used in the base package too.

Overall, I think that we should not use the PaperProps as a type in these components, but that may be another topic.

@siriwatknp
Copy link
Member

I think this approach is a workaround which is fine. To really fix the underlying problem, we have to revisit the Overriable... generic.

@jussirantala
Copy link

jussirantala commented Apr 22, 2022

I think this approach is a workaround which is fine. To really fix the underlying problem, we have to revisit the Overriable... generic.

This fix won't let the user to use component specific props like onSubmit if form element is used as the component. You can do that with Paper component so you should be able to do that in Dialog etc also when you are giving those as PaperProps.

@m4theushw
Copy link
Member

Overall, I think that we should not use the PaperProps as a type in these components, but that may be another topic.

@mnajdova In the DataGrid we use the xxxProps interfaces in a few places, #4654 is one example. Besides PaperProps, we would have to also apply the fix here to IconButtonProps and MenuItemProps, or we fix it in our side.

@hbjORbj hbjORbj force-pushed the fix/ts-error-paper-props branch from 98d0592 to 75999c7 Compare May 9, 2022 09:14
@hbjORbj
Copy link
Member Author

hbjORbj commented May 9, 2022

@m4theushw Thanks I just applied the diff P = { component?: React.ElementType }, to IconButtonProps and MenuItemProps as well.

@hbjORbj
Copy link
Member Author

hbjORbj commented May 9, 2022

The following should output typescript error, but it doesn't because of the change in this PR (AppBar extends Paper). I think we shouldn't go with the approach of this PR.

const CustomComponent: React.FC<{ stringProp: string; numberProp: number }> = () => <div />;

<AppBar component={CustomComponent} />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: dialog This is the name of the generic UI component, not the React module! component: drawer This is the name of the generic UI component, not the React module! component: Paper This is the name of the generic UI component, not the React module! typescript
Projects
None yet
6 participants