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

[Modal][base] Drop component prop #37058

Merged
merged 4 commits into from
Apr 30, 2023
Merged

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Apr 27, 2023

Part of #36679

@mui-bot
Copy link

mui-bot commented Apr 27, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 3cfe75f

@zannager zannager requested a review from mnajdova April 27, 2023 13:35
@hbjORbj hbjORbj force-pushed the modal/component-prop branch from 9a924ae to 8e41846 Compare April 28, 2023 11:33
@hbjORbj hbjORbj requested review from michaldudak and removed request for mnajdova April 28, 2023 11:33
@hbjORbj hbjORbj force-pushed the modal/component-prop branch from 8e41846 to 9c640b4 Compare April 28, 2023 11:41
@hbjORbj hbjORbj force-pushed the modal/component-prop branch from ac4a911 to c83ecea Compare April 28, 2023 16:02
@@ -91,7 +90,7 @@ const Modal = React.forwardRef(function Modal<RootComponentType extends React.El
slots = {},
...other
} = props;

const manager = managerProp as any;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is temporary. I think there is some type error in this file. For example,

we should change

  const modal = React.useRef<{
    modalRef?: typeof modalRef.current;
    mountNode?: typeof mountNodeRef.current;
  }>({});

to

  const modal = React.useRef<{
    mount?: Element;
    modalRef?: Element;
  }>({});

to match the type of methods provided by ModalManager. I'd rather do these changes in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Weird. But yes, I agree, let's solve this in a separate PR. Could you just add a TODO comment above this line explaining what is it for?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@hbjORbj hbjORbj merged commit 3bf332a into mui:master Apr 30, 2023
binh1298 pushed a commit to binh1298/material-ui that referenced this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants