-
-
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
[material-ui][Modal] migrate useSlotProps to useSLot #42150
Conversation
Netlify deploy previewhttps://deploy-preview-42150--material-ui.netlify.app/ Drawer: parsed: -0.35% 😍, gzip: -0.43% 😍 Bundle size reportDetails of bundle changes (Toolpad) |
9750bba
to
f9d91d7
Compare
3b0add0
to
ee945b8
Compare
a327ab1
to
6b8fd4f
Compare
@@ -44,7 +44,6 @@ describe('<Modal />', () => { | |||
'themeDefaultProps', // portal, can't determine the root | |||
'themeStyleOverrides', // portal, can't determine the root | |||
'reactTestRenderer', // portal https://github.com/facebook/react/issues/11565 | |||
'slotPropsCallback', // not supported yet |
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.
proof of slotPropsCallback is working
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 working on this @sai6855
const externalForwardedProps = { | ||
slots: { | ||
root: slots.root ?? components.Root, | ||
backdrop: slots.backdrop ?? components.Backdrop, | ||
}, | ||
slotProps: { | ||
root: slotProps.root ?? componentsProps.root, | ||
backdrop: backdropSlotProps, | ||
}, | ||
}; |
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.
Could this be?:
const externalForwardedProps = { | |
slots: { | |
root: slots.root ?? components.Root, | |
backdrop: slots.backdrop ?? components.Backdrop, | |
}, | |
slotProps: { | |
root: slotProps.root ?? componentsProps.root, | |
backdrop: backdropSlotProps, | |
}, | |
}; | |
const externalForwardedProps = { | |
slots: { | |
root: components.Root, | |
backdrop: components.Backdrop, | |
...slots, | |
}, | |
slotProps: { | |
...componentsProps, | |
...slots | |
}, | |
}; |
This would allow us to remove the rootSlotProps
and backdropSlotProps
variables.
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.
@DiegoAndai refactored in this commit f7a2329, PR is now ready for next review
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 taking this @sai6855
since #42469 is handling deprecating props, this PR focuses on migrating useSlotProps to useSlot
Not sure about argos failure, don't think it's related to this PR, same errors can be found in #42471 PR too