[Tooltip] Fix className not getting applied from PopperProps

[ZeeshanTamboli]

• I have followed (at least) the PR section of the contributing guide.

Closes #28991

As per this #28692 (comment) the PopperProps take precedence over the componentsProps.popper.

I am not sure why we have both PopperProps and componentsProps.popper. Maybe PopperProps was not removed to avoid breaking changes.

CodeSandbox after the fix.

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 3c83490

[mnajdova]

Good catch

[ZeeshanTamboli]

If somebody gets time, this can be reviewed further.

[michaldudak]

Thanks!

[ZeeshanTamboli]

@michaldudak
I have four questions, maybe out of the scope of this PR.

1. Should we pass and support className for TransitionComponent as well like for PopperComponent?

Current TransitionComponent:
https://github.com/mui-org/material-ui/blob/d11ef0f82f1ef6d62518f0879b674feb9f916d66/packages/mui-material/src/Tooltip/Tooltip.js#L686-L689

PopperComponent (after fix):
https://github.com/ZeeshanTamboli/material-ui/blob/ffb9389ede1b2c782b6110e9aae1ab9375242a29/packages/mui-material/src/Tooltip/Tooltip.js#L681-L682
Maybe it would cause issues later for TransitionProps.className.

2. Should we mark TransitionComponent, PopperComponent, TransitionProps and PopperProps deprecated? (@deprecated)
3. For native className prop we allow it from both componentProps and PopperProps, but say in the docs that one take precedence over another. Should we add except for className?
4. For latest preview codesandbox of this PR: https://codesandbox.io/s/popperprops-issue-o0wkg?file=/src/App.tsx, the style now gets applied fixing the issue linked but the popper type fails as open prop is required in PopperProps. I doubt there should be an open prop required with an already open tooltip.

[michaldudak]

Should we pass and support className for TransitionComponent as well like for PopperComponent?

Yes, we should.

Should we mark TransitionComponent, PopperComponent, TransitionProps and PopperProps deprecated?

I suppose we can. @mnajdova any thoughts on this?

For native className prop we allow it from both componentProps and PopperProps, but say in the docs that one take precedence over another. Should we add except for className?

Yep, good idea.

For latest preview codesandbox of this PR: https://codesandbox.io/s/popperprops-issue-o0wkg?file=/src/App.tsx, the style now gets applied fixing the issue linked but the popper type fails as open prop is required in PopperProps. I doubt there should be an open prop required with an already open tooltip.

Good catch. The componentsProps.popper type should be Partial<PopperProps> & TooltipComponentsPropsOverrides. Would you be willing to create a PR?

[mnajdova]

I suppose we can. @mnajdova any thoughts on this?

I would say it's too early now, but we can add in our v6 milestone to remove those and add the deprecation warnings closer before we actually release v6. Adding new deprecation warnings in the 5.0.x it's too much :)

[ZeeshanTamboli]

Should we pass and support className for TransitionComponent as well like for PopperComponent?

Yes, we should.

For native className prop we allow it from both componentProps and PopperProps, but say in the docs that one take precedence over another. Should we add except for className?

Yep, good idea.

For latest preview codesandbox of this PR: https://codesandbox.io/s/popperprops-issue-o0wkg?file=/src/App.tsx, the style now gets applied fixing the issue linked but the popper type fails as open prop is required in PopperProps. I doubt there should be an open prop required with an already open tooltip.

Good catch. The componentsProps.popper type should be Partial<PopperProps> & TooltipComponentsPropsOverrides. Would you be willing to create a PR?

I can fix these three points in a new PR after we merge this.

[michaldudak]

Thanks for your work on this!