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][material] Fix console warning when onTransitionEnter , onTransitionExit provided #38868

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Sep 8, 2023

closes: #38843

before: https://codesandbox.io/s/mui-swipabledrawer-example-new-7pnch2

after: https://codesandbox.io/s/affectionate-hooks-vwlqrk

Base Modal extracted onTransitionEntered, onTransitionExited here from props before passing to useSlotProps here. That's why same error didn't occured in BaseUI Modal. This PR follows same implemetation as BaseUI Modal to fix the warning

@sai6855 sai6855 marked this pull request as draft September 8, 2023 08:57
@sai6855 sai6855 added bug 🐛 Something doesn't work component: drawer This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Sep 8, 2023
@mui-bot
Copy link

mui-bot commented Sep 8, 2023

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b2d20f0

@sai6855 sai6855 added component: modal This is the name of the generic UI component, not the React module! and removed component: drawer This is the name of the generic UI component, not the React module! labels Sep 8, 2023
@sai6855 sai6855 changed the title [Modal][material] Fix warning when onTransitionEnter , onTransitionExit provided [Modal][material] Fix console warning when onTransitionEnter , onTransitionExit provided Sep 8, 2023
@sai6855 sai6855 marked this pull request as ready for review September 8, 2023 09:39
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Nice catch. We make sure they are not spread when using the getRootProps callback, but here they are spread as part of the other. One more benefit of being able to send all props to the getSlotProps callbacks - we can intercept props that should not be propagated further cc @mj12albert

Copy link
Member

@DiegoAndai DiegoAndai 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 jumping into this one @sai6855! 🎉

@DiegoAndai DiegoAndai merged commit 34c1ffc into mui:master Sep 8, 2023
9 of 10 checks passed
@sai6855 sai6855 deleted the drawer-transition branch September 8, 2023 14:27
xcode-it pushed a commit to xcode-it/material-ui that referenced this pull request Sep 11, 2023
@mnajdova
Copy link
Member

@sai6855 this pull request was a duplicate of #38745. For more trivial fixes, we can allow first-time contributors or some contributors that are not that active to pick them up. It would have been much better if you'd reviewed the other pull request, rather than creating the pull request yourself. A note for next time - check if there is an ongoing pull request and link it with the original issue - this will help with not picking up duplicate work.

@sai6855
Copy link
Contributor Author

sai6855 commented Sep 13, 2023

@sai6855 this pull request was a duplicate of #38745. For more trivial fixes, we can allow first-time contributors or some contributors that are not that active to pick them up. It would have been much better if you'd reviewed the other pull request, rather than creating the pull request yourself. A note for next time - check if there is an ongoing pull request and link it with the original issue - this will help with not picking up duplicate work.

Oh, since PR wasn't linked to the issue I wasn't aware of that pr . If it was linked i wouldn't touch the issue or create new PR. Also PR was opened before issue was created so there is very little chance that i would be aware of that PR. But from next time i'll be bit more careful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SwipeableDrawer][material] onTransitionEnter prop throws an error starting in version 5.14.7
4 participants