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

[core][Modal] Remove unnecessary manager prop handling #43867

Conversation

ZeeshanTamboli
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli commented Sep 24, 2024

While reviewing a PR I noticed the Modal component and useModal hook don't have a manager prop, and we don’t export useModal. This PR removes the manager prop handling and directly uses the default modal manager, in turn improving type safety when using the modal manager methods.

@ZeeshanTamboli ZeeshanTamboli added component: modal This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material enhancement This is not a bug, nor a new feature labels Sep 24, 2024
@mui-bot
Copy link

mui-bot commented Sep 24, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 500ed90

@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Modal] Remove unnecessary manager prop handling [material-ui][Modal] Remove unnecessary manager prop handling Sep 24, 2024
@ZeeshanTamboli ZeeshanTamboli marked this pull request as ready for review September 24, 2024 05:53
@@ -85,15 +83,15 @@ function useModal(parameters: UseModalParameters): UseModalReturnValue {
const handleOpen = useEventCallback(() => {
const resolvedContainer = getContainer(container) || getDoc().body;

manager.add(getModal(), resolvedContainer);
manager.add(getModal(), resolvedContainer as HTMLElement);
Copy link
Member Author

@ZeeshanTamboli ZeeshanTamboli Sep 24, 2024

Choose a reason for hiding this comment

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

I had to cast it because the container prop is typed as Element (see here), but the modal manager methods expect HTMLElement, which is correct. Also, document.body (the default container) is an HTMLElement. Changing the container type in Portal would be a breaking change, though we could also allow both Element and HTMLElement.

@ZeeshanTamboli
Copy link
Member Author

Can I get a review on this?

Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

Looks good. Not sure how we could get rid of the type casting without a breaking change.

@ZeeshanTamboli ZeeshanTamboli merged commit 16097c7 into mui:master Oct 2, 2024
19 checks passed
@ZeeshanTamboli ZeeshanTamboli deleted the remove-unnecessary-manager-prop-modal branch October 2, 2024 12:12
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 2, 2024

This prop <Modal manager={}> was kind of a public API but it was never very clear, e.g. #14980 (comment). We will see if people have use cases to ask for this back.

@aarongarciah
Copy link
Member

@oliviertassinari ouch. Sadly there wasn't a test to catch it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 2, 2024

It's Ok, we can wait to see if people create issues. As far as I remember, the only use case left for this manager prop is for people who load multiple versions of Material UI, and still need the modal manager singleton to correctly handle the top level modal. It was never really documented, more as workaround on issues. If we can drop that prop, great.

@atomiks As I understand it, in Base UI, the modal relies on Floating UI, https://master--base-ui.netlify.app/components/react-dialog/#nested-dialogs. And simply say: let's not fix it: https://github.com/mui/base-ui/blob/f51b2f4587df29a1f0d093745f4b8ed7439aba34/packages/mui-base/src/Dialog/Popup/DialogPopup.tsx#L33. So if we can get away with this in Material UI, great, we reduce the gap with Base UI.

@aarongarciah aarongarciah mentioned this pull request Oct 2, 2024
@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Oct 2, 2024
@oliviertassinari oliviertassinari changed the title [material-ui][Modal] Remove unnecessary manager prop handling [cre][Modal] Remove unnecessary manager prop handling Oct 2, 2024
@oliviertassinari oliviertassinari changed the title [cre][Modal] Remove unnecessary manager prop handling [core][Modal] Remove unnecessary manager prop handling Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: modal This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants