-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[charts] Fix some bad practice with useSlotProps
#13141
Conversation
Deploy preview: https://deploy-preview-13141--material-ui-x.netlify.app/ |
Probably yes
Yes, that's also how I understand it's purpose. |
@JCQuintas I think this PR contains some code standards :) |
@@ -145,17 +145,16 @@ function BarElement(props: BarElementProps) { | |||
elementType: Bar, | |||
externalSlotProps: slotProps?.bar, |
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 should work and allow you to not spread other
below:
externalSlotProps: slotProps?.bar, | |
externalSlotProps: slotProps?.bar, | |
externalForwardedProps: other, |
It should also automatically merge the event handlers and stuff like ref
and style
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 is a nice suggestion! 👍
Other than that, the changes look great! 💯
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've not done the other components, because the TS result seems buggy. even if other
type contains d: string
the resulting props do not contain it
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 typing of useSlotProps
is ... complex 😆
Will add a section about slots 😅 |
@@ -145,17 +145,16 @@ function BarElement(props: BarElementProps) { | |||
elementType: Bar, | |||
externalSlotProps: slotProps?.bar, |
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 is a nice suggestion! 👍
Other than that, the changes look great! 💯
Btw, not sure who handles this, but it would be really helpful if these functions/types we use extensively had more comprehensive typedocs with how to use them and examples. |
There is nothing preventing us from opening a PR on the core to improve the JSDOC 👍 |
I would if I knew how they worked 😆 |
Sure, I was not asking you to open the PR for something like |
Follow up on #12921 comments
Each comment get its commit
@flaviendelangle about this last comment, I thought the goal of this hook is to do the merging, and so I could delegate it the fact of merging multiple className props.
This pattern of passing
className
to theadditionalProps
also exists in the pickers package. Is it a mistake?https://github.com/mui/mui-x/blob/master/packages/x-date-pickers/src/internals/components/PickersArrowSwitcher/PickersArrowSwitcher.tsx/#L121-L134