material-ui
material-ui copied to clipboard
mui
[material-ui][Backdrop] Deprecate TransitionComponent
Part of: https://github.com/mui/material-ui/issues/40417
@DiegoAndai here's my submission for the backdrop component update, please let me know if it's correct. I did ask for clarification on the original post, but I didn't get a response. Enjoy the rest of this fine Thursday!
Changes made to backdrop component backdrop: Add deprecations for transition props and the slots API that should replace it.
Questions for reviewers I see that the transition slot in other components has "transition props" under the prop "slotProps" is this required for this component? as the original code did not have this provided.
Netlify deploy preview
Bundle size report
Details of bundle changes (Toolpad) Details of bundle changes
Generated by :no_entry_sign: dangerJS against 9649458bf606ecb6264271415f4eac52af6229a6
@DiegoAndai Thanks for the opportunity to contribute, and thanks for taking the time to review my code.
Sorry about the docs.... I had read it, but completely forgot.🙃
I hope you'll see I've made the requested changed, I've checked and double checked, but please let me know if I missed anything.
Hi @DiegoAndai thanks again for the code review, as always its appreciated. 🤟
I've made the changes you requested. I'm a little unsure on the tests, so if you could just have a quick look that would be awesome.
Afternoon @DiegoAndai! Thanks for the code review! 🤟(as always)
Please find the amended code pushed. A quick point of note, running the tests throws an error, as the describeConformance prop "testLegacyComponentsProp" throws. After looking at it, I came to the assumption it wasn't required as it tests for props that we just depreciated. Is this correct? I noticed in your example and the other CR's that the prop isn't present.
The failing test:
On a side note, I just wanted to say thanks for all the pointers along the way. It's probably frustrating having to make all these comments, so thanks for the time, the effort is appreciated! The next component will be a lot smoother. 🫡
Good afternoon @DiegoAndai, I hope you had a good start to the week!
Please find my attempt at the codemod pushed! I haven’t had much experience with code mods before, and given theres only one example up, I hope its okay... I tried my best.
Let me know if theres any amendments you need made, hope the rest of the week goes well!
@DiegoAndai Good morning man!
Hope your having a good start to the week! Please find the updated code so that it's synced with master. I pulled re-based, hence why all the commits of the previous merges are at the bottom of the stack again.
Have a good start to the week! looking forward to hearing what you have to say.
Hey @harry-whorlow. Initially I suggested deprecating
transitionDurationin favor ofslotProps.transition.timeout, but now I think we can keeptransitionDurationas it's a common use case so it's easier to have a direct prop for it. What do you think? If we choose to keep it, then we'll have to remove thetransitionDurationcode from the codemods.
@DiegoAndai, ooof good question... I'm all for uniformity, but yeah I would agree it makes sense to keep the prop available. Besides, its just a number passed to an animation. I'll make the change!
Afternoon @DiegoAndai, or good morning... I suppose it depends on where you live.
If its cool with you, I'll pull rebase to update with main and get rid of the "out of date" tag...
If theres more comments to make I'll leave it as it is, so I'm not constantly resolving conflicts in the "packages/mui-codemod/README.md" and "packages/mui-codemod/src/deprecations/all/deprecations-all.js" files.
Let me know what you think! 🤟
@DiegoAndai Hi man, good point on the transitionDuration... went right over my head. Please find the modified code pushed. 🤟
Hey @harry-whorlow, great work! This is ready to merge 🎉.
Next week, we'll freeze v5 and open the next branch to release v6 alpha versions. Would you mind waiting until then to merge this? This way, we'll avoid fixing any error in v5 that may have slipped. This is to be extra careful as we'll try not to release any v5 patches after it's frozen. Does that make sense?
If you're on board, I'll ask you to wait a bit longer, and when the next branch is created next week, point this PR to that branch instead. I'll let you know.
@DiegoAndai dude you've got to hit me with the "LGTM 🚀". 😂
On a more serious note... I'm happy to wait till next week, I get the versioning concerns, and I'd hate to be the reason for screwing it up. When about's next week are you branching? or has this not been decided yet.
To finish up, thanks for all the help along the way, it's been super cool working in this code base! It's really appreciated, and It's opened my eyes to a lot of stuff that I wouldn’t of came across otherwise. And thanks to @sai6855 too!
If it's cool with you I'll carry on with speed dial, I hope I'm a bit more independent this time round. 🤟
@harry-whorlow, we're close, I promise 😂
Thanks for your patience and continued working on this through the various revisions. I expect the next branch to be created on Tuesday or Wednesday of next week, and then we can point this PR to it. I'm happy it has been enjoyable and fruitful for you as well.
Feel free to carry on with the speed dial 🙌🏼 for that one, let's not create the PR yet. We should create it directly to the next branch to avoid creating it pointing to master and then having to change it a few days later.
@DiegoAndai, Thats cool! hit me up when the branch is live. Looking forward to merging it!
The speed dial is something I already started, here, but I think it fell through the gaps. Plus, I know it needs rebasing and updating with things like code mods and that... So I wouldn’t worry about code reviewing, but as you said need to change where it's pointing to.
Either way, enjoy the rest of you day!
Hey @harry-whorlow! I pointed the PR to the next branch and resolved merge conflicts. You'll have to pull these changes.
May I ask you to add the Backdrop section to the migration guide, we should follow the current format: https://github.com/mui/material-ui/blob/next/docs/data/material/migration/migrating-from-deprecated-apis/migrating-from-deprecated-apis.md
@DiegoAndai like so? 🤟
Yes 👌🏼
I pushed some fixes, I'm doing the some final testing 👍🏼
Ah... you beat me to the capitalisation. The fastest hands in the west
@DiegoAndai, thank you for all the help along the way! you've been excellent.
I'll see you over in speedDial in the coming week! 🚀