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

[Base] Allow useSlotProps to receive undefined elementType #35192

Merged
merged 5 commits into from Nov 23, 2022

Conversation

leventdeniz
Copy link
Contributor

To make this PR #34793 possible, we agreed to extend useSlotProps with the support for undefined component types.

See this comment: #34793 (comment)

@mui-bot
Copy link

mui-bot commented Nov 18, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-35192--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against f1b5e6a

@zannager zannager added the package: base-ui Specific to @mui/base label Nov 18, 2022
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please create a test that verifies the behavior you added?

otherProps: OtherProps,
ownerState: OwnerState,
): AppendOwnerStateReturnType<ElementType, OtherProps, OwnerState> {
if (isHostComponent(elementType)) {
if (elementType && isHostComponent(elementType)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to append ownerState when ElementType is not defined.

Suggested change
if (elementType && isHostComponent(elementType)) {
if (elementType === undefined || isHostComponent(elementType)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick review! My bad, fixed it.

@@ -29,7 +29,7 @@ export type AppendOwnerStateReturnType<
/**
* Appends the ownerState object to the props, merging with the existing one if necessary.
*
* @param elementType Type of the element that owns the `existingProps`. If the element is a DOM node, `ownerState` is not applied.
* @param elementType Type of the element that owns the `existingProps`. If the element is undefined or a DOM node, `ownerState` is not applied.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the DOM node case is likely much more common, so I'd put it first.

Suggested change
* @param elementType Type of the element that owns the `existingProps`. If the element is undefined or a DOM node, `ownerState` is not applied.
* @param elementType Type of the element that owns the `existingProps`. If the element is a DOM node or undefined, `ownerState` is not applied.

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution!

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a great first pull request on MUI, thank you for working on it! 👌

@mnajdova mnajdova merged commit f397fa9 into mui:master Nov 23, 2022
@leventdeniz leventdeniz deleted the use-slot-props-supports-undefined branch November 23, 2022 15:08
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants