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] Fix crashing of DraggableDialog demo #44747

Merged
merged 6 commits into from
Dec 12, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Dec 12, 2024

closes #44732

Error occurs because react-draggable is using the deprecated findDOMNode method. On updating code to use refs fixed the issue

before: https://mui.com/material-ui/react-dialog/#draggable-dialog

after: https://deploy-preview-44747--material-ui.netlify.app/material-ui/react-dialog/#draggable-dialog

@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! package: material-ui Specific to @mui/material regression A bug, but worse labels Dec 12, 2024
@mui-bot
Copy link

mui-bot commented Dec 12, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 995d4e8

@sai6855 sai6855 marked this pull request as draft December 12, 2024 08:27
@sai6855 sai6855 changed the title [material-ui][Dialog] Fix DraggableDialog demo [material-ui][Dialog] Fix crashing of DraggableDialog demo Dec 12, 2024
Copy link

@Thor-x86 Thor-x86 left a comment

Choose a reason for hiding this comment

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

I'm not sure if we have to suppress the null type with exclamation mark (!) here. Since the nodeRef won't be null anyway, only its "current" member be nullable. I suggest you to not use it for safety reason.

Nevermind, it just gone. Thanks

return (
<Draggable
nodeRef={nodeRef as React.RefObject<HTMLDivElement>}
Copy link
Contributor Author

@sai6855 sai6855 Dec 12, 2024

Choose a reason for hiding this comment

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

nodeRef doesn't accept a null type. If null is removed from the nodeRef declaration, TypeScript throws an error in the Paper component. So i used type assertion as a workaround.

Copy link
Member

Choose a reason for hiding this comment

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

FYI there's an open issue and PR in the react-draggable repo about this TS error.

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, FYI react-draggable last published version is on last year. I'm not sure if we have to wait for them to merge the PR.

https://www.npmjs.com/package/react-draggable

Copy link
Member

Choose a reason for hiding this comment

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

Yeah! My intention wasn't for us to wait, just to be aware of it.

@sai6855 sai6855 marked this pull request as ready for review December 12, 2024 09:36
@aarongarciah aarongarciah merged commit a491dc2 into mui:master Dec 12, 2024
22 checks passed
@mj12albert
Copy link
Member

mj12albert commented Dec 12, 2024

Thanks ~ @sai6855

@sai6855 sai6855 deleted the fix-reactdraggle branch December 12, 2024 13:15
@aarongarciah
Copy link
Member

aarongarciah commented Dec 12, 2024

This is now fixed on https://mui.com

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! docs Improvements or additions to the documentation package: material-ui Specific to @mui/material regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Demo DraggableDialog crashes
5 participants