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

[material-ui][Dialog] Add slots and slotProps #44792

Merged
merged 33 commits into from
Jan 15, 2025
Merged

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Dec 17, 2024

Part of #41281

@sai6855 sai6855 marked this pull request as draft December 17, 2024 10:38
@sai6855 sai6855 added docs Improvements or additions to the documentation component: dialog This is the name of the generic UI component, not the React module! deprecation New deprecation message package: material-ui Specific to @mui/material labels Dec 17, 2024
@mui-bot
Copy link

mui-bot commented Dec 17, 2024

Netlify deploy preview

https://deploy-preview-44792--material-ui.netlify.app/

@material-ui/core: parsed: +0.10% , gzip: +0.10%
Dialog: parsed: +0.59% , gzip: +0.32%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 51ca92b

@sai6855
Copy link
Contributor Author

sai6855 commented Dec 17, 2024

In Dialog BackdropComponent and PaperComponent are being used as as={Component}

BackDropComponent

PaperComponent

Should i deprecate these or leave as it is similarly to IconComponent like here #42157 (comment)

cc @DiegoAndai @siriwatknp

@sai6855 sai6855 requested a review from siriwatknp December 18, 2024 05:57
@DiegoAndai
Copy link
Member

@sai6855, I think:

  • BackdropComponent can be deprecated in favor of slots.backdrop.component
  • PaperComponent can be deprecated in favor of slots.paper.component

Please test this though, as I haven't.

@siriwatknp
Copy link
Member

siriwatknp commented Jan 7, 2025

@sai6855 Is this PR ready for review? if yes, please switch from "draft" to "ready".

@sai6855
Copy link
Contributor Author

sai6855 commented Jan 7, 2025

Migratio guide is pending, I'll add it mark it ready for review

@sai6855
Copy link
Contributor Author

sai6855 commented Jan 9, 2025

I'm facing few issues while deprecating PaperComponent, will try to find solution and update


export interface DialogProps extends StandardProps<ModalProps, 'children'> {
export interface DialogSlots {
Copy link
Member

Choose a reason for hiding this comment

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

Missing root, backdrop, container, and paper slots.

@sai6855 sai6855 marked this pull request as ready for review January 14, 2025 06:27
@sai6855
Copy link
Contributor Author

sai6855 commented Jan 14, 2025

@siriwatknp PR is ready for review now, I haven't deprecated PaperComponent reason being, in existing version PaperComponent is not equivalent to PaperProps.component. In this demo if PaperComponent='form' instead of PaperProps.component styling of Dialog breaks, so i haven't deprecated PaperComponent yet.

check the demo here: https://stackblitz.com/edit/react-mlv38cpv?file=Demo.tsx,

@sai6855 sai6855 requested a review from siriwatknp January 14, 2025 06:54
@@ -1,12 +1,96 @@
import * as React from 'react';
import { SxProps, Breakpoint } from '@mui/system';
import { InternalStandardProps as StandardProps, Theme } from '..';
import { BackdropProps, InternalStandardProps as StandardProps, Theme } from '..';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { BackdropProps, InternalStandardProps as StandardProps, Theme } from '..';
import { InternalStandardProps as StandardProps, Theme } from '..';
import { BackdropProps } from '../Backdrop';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed here 4c61efb

Comment on lines 16 to 18
transition?: React.JSXElementConstructor<
TransitionProps & { children?: React.ReactElement<any, any> }
>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
transition?: React.JSXElementConstructor<
TransitionProps & { children?: React.ReactElement<any, any> }
>;
transition?: React.ElementType

Is there a blocker using React.ElementType? The reason to use it is to widening the type so user can pass their custom component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, i used previous type. updated here 6ae70b7

* By default, the avaible props are based on a div element.
*/
container: SlotProps<
React.ElementType<React.HTMLAttributes<HTMLDivElement>>,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
React.ElementType<React.HTMLAttributes<HTMLDivElement>>,
'div',

I found that this is simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 79e8ea6

@@ -125,3 +212,5 @@ export interface DialogProps extends StandardProps<ModalProps, 'children'> {
* - inherits [Modal API](https://mui.com/material-ui/api/modal/)
*/
export default function Dialog(props: DialogProps): React.JSX.Element;

export interface DialogOwnerState extends DialogProps {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export interface DialogOwnerState extends DialogProps {}
export interface DialogOwnerState extends Omit<DialogProps, 'slots' | 'slotProps'> {}

to prevent recursive types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 12c6f4f

@sai6855 sai6855 changed the title [material-ui][Dialog] Add slots and slotProps and deprecate *ComponentProps [material-ui][Dialog] Add slots and slotProps Jan 15, 2025
@sai6855 sai6855 requested a review from siriwatknp January 15, 2025 07:59
slotProps: backwardCompatibleSlotProps,
};

const [RootSlot, rootSlotProps] = useSlot('root', {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [RootSlot, rootSlotProps] = useSlot('root', {
const [RootSlot, rootSlotProps] = useSlot('root', {
shouldForwardComponentProp: true,

If the elementType is a styled component of another styled component, this prop should be set to true, so that slotProps.root.component does not replace the inner styled component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it added here 51ca92b.

If the elementType is a styled component of another styled component, this prop should be set to true, so that slotProps.root.component does not replace the inner styled component.

Does that mean, shouldForwardComponent:true is missing here? given TableSortLabelRoot is of another styled component (ButtonBase)

https://github.com/sai6855/material-ui/blob/51ca92bdb08fadb1a08ce75ac2ed1b75f02fa445/packages/mui-material/src/TableSortLabel/TableSortLabel.js#L132-L133

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's right.

Comment on lines 313 to 317
const [BackdropSlot, { as: backdropComponent, ...backdropSlotProps }] = useSlot('backdrop', {
elementType: DialogBackdrop,
externalForwardedProps,
ownerState,
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [BackdropSlot, { as: backdropComponent, ...backdropSlotProps }] = useSlot('backdrop', {
elementType: DialogBackdrop,
externalForwardedProps,
ownerState,
});
const [BackdropSlot, backdropSlotProps] = useSlot('backdrop', {
shouldForwardComponentProp: true,
elementType: DialogBackdrop,
externalForwardedProps,
ownerState,
});

With shouldForwardComponentProp: true, the backdropSlotProps will not produce as prop.

@sai6855 sai6855 requested a review from siriwatknp January 15, 2025 09:41
@sai6855 sai6855 merged commit ff9b9e4 into mui:master Jan 15, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dialog This is the name of the generic UI component, not the React module! deprecation New deprecation message docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants