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.

Sign up for GitHub

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

Jump to bottom

[Modal] Convert code to typescript #34793

Merged
merged 17 commits into from
Dec 20, 2022
Merged

[Modal] Convert code to typescript #34793

merged 17 commits into from
Dec 20, 2022

Conversation

leventdeniz
Copy link
Contributor

@leventdeniz leventdeniz commented Oct 16, 2022 •
edited by michaldudak
Loading

closes #34716

@zannager zannager added status: waiting for maintainer These issues haven't been looked at yet by a maintainer component: modal This is the name of the generic UI component, not the React module! labels Oct 17, 2022
@oliviertassinari oliviertassinari removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 17, 2022
michaldudak
michaldudak requested changes Oct 18, 2022
View reviewed changes
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 work! There is a couple of changes needed before we can merge it.

docs/data/base/components/modal/ServerModal.tsx Outdated Show resolved Hide resolved
docs/data/base/components/modal/ServerModal.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalUnstyled.spec.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalUnstyled.tsx Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalUnstyled.types.ts Outdated Show resolved Hide resolved
packages/mui-base/src/ModalUnstyled/ModalUnstyled.tsx Outdated
@@ -249,7 +274,7 @@ const ModalUnstyled = React.forwardRef(function ModalUnstyled(props, ref) {
ownerState,
});

const BackdropComponent = components.Backdrop;
const BackdropComponent = components?.Backdrop || 'div';
Copy link
Member

Choose a reason for hiding this comment

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

Why the || div is added? This changes the default behavior and makes the backdrop always visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useSlotProps expects elementType not to be undefined, so I added this as a default that seemed reasonable.
After checking other usages, maybe this would be better?

Suggested change
const BackdropComponent = components?.Backdrop || 'div';
const BackdropComponent = components?.Backdrop ?? 'div';

If not, I need some Help resolving this.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, it seems it's necessary to extend useSlotProps with the support for undefined component types. It should return an empty object then. This could be done in a separate PR to keep things focused. Would you like to work on it as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I would love to do that.

Copy link
Member

@michaldudak michaldudak Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

All right, so let's get that one done first and then we can go back to the Modal.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 19, 2022
@leventdeniz
Copy link
Contributor Author

Thank you for your review, I will work on these on the weekend!

@mui-bot
Copy link

mui-bot commented Oct 26, 2022 •
edited
Loading

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

Details of bundle changes

Generated by 🚫 dangerJS against b511f65

@levent-check24
Merge branch 'master' into convert-modal-to-typescript
3c8db62 
# Conflicts:
#	packages/mui-base/src/ModalUnstyled/ModalUnstyled.tsx
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 26, 2022
@leventdeniz
Copy link
Contributor Author

I can't seem to get these last two type errors solved. Maybe someone could help me with them.

@leventdeniz leventdeniz mentioned this pull request Nov 18, 2022
Merged
1 task
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 6, 2022
@michaldudak
Copy link
Member

Do you need any help resolving merge conflicts or making CI checks green?

Regarding type errors, you need to cast the const ModalUnstyled to OverridableComponent<ModalUnstyledTypeMap> as React.forwardRef messes up the generics.
The proper definition of types should allow for the removal of the last @ts-expect-error in ModalUnstyled.spec.tsx.

@levent-check24
Merge branch 'master' into convert-modal-to-typescript
01a388f 
# Conflicts:
#	packages/mui-base/src/ModalUnstyled/ModalUnstyled.tsx
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 12, 2022
@leventdeniz
Copy link
Contributor Author

I did what you suggested but this somehow gave me even more errors in the ModalUnstyled.test.tsx.
Thank you for your patience but I think I need help resolving these last few errors.

@michaldudak
Copy link
Member

You have done the majority of hard work, thanks! The remaining issue was missing types of React.createRef. I added them.
I also removed one of the tests as prop forwarding is already being tested in describeConformanceUnstyled.

@michaldudak michaldudak added typescript enhancement This is not a bug, nor a new feature package: base-ui Specific to @mui/base labels Dec 14, 2022
@leventdeniz
Copy link
Contributor Author

Thank you so much for your patience and help, I really appreciate it!

@michaldudak
Copy link
Member

Thank you for contributing! Feel free to tackle any other issues.

@michaldudak michaldudak merged commit 48fb870 into mui:master Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees
No one assigned
Labels
component: modal This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature package: base-ui Specific to @mui/base typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Modal][base] Convert code to TypeScript
6 participants
@leventdeniz @mui-bot @michaldudak @oliviertassinari @zannager @levent-check24