[ModalUnstyled] Define ownerState and slot props' types #32901
Conversation
michaldudak
added
typescript
component: modal
| @@ -111,8 +113,8 @@ const Modal = React.forwardRef(function Modal(inProps, ref) { | |||
| ownerState: { ...componentsProps.root?.ownerState }, | |||
| }), | |||
| }, | |||
| backdrop: BackdropProps ?? componentsProps.backdrop, | |||
There was a problem hiding this comment.
Should we merge these two for now?
| @@ -237,23 +226,6 @@ Modal.propTypes /* remove-proptypes */ = { | |||
| * @default false | |||
| */ | |||
| keepMounted: PropTypes.bool, | |||
| /** | |||
There was a problem hiding this comment.
This is suspicious, why were these removed? Should we update the props interface for the @mui/material's Modal?
There was a problem hiding this comment.
These were removed in an older commit. After #32739 was merged, these lines were restored.
| @@ -271,7 +272,7 @@ const ModalUnstyled = React.forwardRef(function ModalUnstyled(props, ref) { | |||
| aria-hidden | |||
| open={open} | |||
| onClick={handleBackdropClick} | |||
| {...BackdropProps} | |||
| {...backdropProps} | |||
There was a problem hiding this comment.
Doesn't have to be part of this pr, but looks like we are missing spreading ownerState here, no?
There was a problem hiding this comment.
Well spotted! I changed the code to use the same pattern as elsewhere (with appendOwnerState)
| }); | ||
|
|
||
| const styledModal = ( | ||
| <ModalUnstyled open components={{ Root }}> |
There was a problem hiding this comment.
Let's add test-case for the Backdrop component too
There was a problem hiding this comment.
| <ModalUnstyled open components={{ Root }}> | |
| <ModalUnstyled open components={{ Root, Backdrop }}> |
| @@ -295,6 +295,7 @@ Dialog.propTypes /* remove-proptypes */ = { | |||
| 'aria-labelledby': PropTypes.string, | |||
| /** | |||
| * A backdrop component. This prop enables custom backdrop rendering. | |||
| * @deprecated Use `components.Backdrop` instead. | |||
There was a problem hiding this comment.
Should we update the component itself to use the components.Backdrop and the componentsProps.backdrop too?
There was a problem hiding this comment.
A feedback from a user left with feedback feature at the bottom of the page:
| role="presentation" | ||
| {...rootProps} | ||
| {...(!isHostComponent(Root) && { | ||
| as: component, |
There was a problem hiding this comment.
Looks like this can create breaking change for the Material UI component, can we handle it there?
| ownerState: { ...componentsProps.root?.ownerState }, | ||
| }), | ||
| }, | ||
| root: componentsProps.root, |
There was a problem hiding this comment.
Maybe add the logic for the components & as prop here?
There was a problem hiding this comment.
👍 Good point. I added it here.
| }); | ||
|
|
||
| const styledModal = ( | ||
| <ModalUnstyled open components={{ Root }}> |
There was a problem hiding this comment.
| <ModalUnstyled open components={{ Root }}> | |
| <ModalUnstyled open components={{ Root, Backdrop }}> |
After #32739 is merged,
yarn proptypesneeds to be run again - this will solve issues with missing fields in API docs.