[material-ui][Backdrop] Deprecate TransitionComponent #40677
Conversation
Netlify deploy previewBundle size reportDetails of bundle changes (Toolpad) |
zannager
added
package: material-ui
There was a problem hiding this comment.
Hey @harry-whorlow, thanks for working on this!
Here's my review so we can keep working on this.
DiegoAndai
changed the title
|
@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. |
There was a problem hiding this comment.
Hey @harry-whorlow, I left a couple comments so we can keep working on this PR 😊
Besides the comments:
- We need to add tests for the slots, it should be similar to: https://github.com/mui/material-ui/pull/40418/files#diff-f88778c6b7a9ab495228d3e0f1b30708032bf6ccdfab7e10a84d43857dc5e143R31-R35
- To fix the failing test, run
pnpm docs:apilocally and push the changes.
harry-whorlow
force-pushed
the
depricate-v6-class-backdrop
branch
from
b9a86e6 to
e0d3417
Compare
|
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! Please find the amended code pushed. A quick point of note, running the tests throws an error, as the describeConformance prop "testLegacyComponentsProp" throws. 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! |
There was a problem hiding this comment.
Hey @harry-whorlow, sorry for the delay, I was waiting for the migration guide PR to get merged.
I left some guidance on the codemod, let me know if it works. You can also follow the recently added codemods contributing guide. That code is a little tricky, if you're stuck let me know and I can review it in more detail.
Now we can also add this deprecation to the migration guide with the corresponding links, you can follow what was done for the accordion in #40767 for guidance.
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. 🫡
It's my pleasure, thanks to you for contributing 🚀 hopefully we can get this one merged soon 😊
packages/mui-codemod/src/deprecations/backdrop-props/backdrop-props.js
Outdated
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/backdrop-props/backdrop-props.js
Outdated
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/backdrop-props/backdrop-props.test.js
Show resolved
Hide resolved
packages/mui-codemod/src/deprecations/backdrop-props/test-cases/actual.js
Show resolved
Hide resolved
github-actions
bot
added
the
PR: out-of-date
| root: { | ||
| expectedClassName: classes.root, | ||
| }, |
There was a problem hiding this comment.
@DiegoAndai was this correct? I was unsure it was required and given that the other example didn't have it I removed it and the tests passed.
harry-whorlow
force-pushed
the
depricate-v6-class-backdrop
branch
from
b7d740d to
6f8ff5c
Compare
github-actions
bot
removed
the
PR: out-of-date
|
@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. Have a good start to the week! looking forward to hearing what you have to say. |
packages/mui-codemod/src/deprecations/backdrop-props/test-cases/actual.js
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
PR: out-of-date
harry-whorlow
force-pushed
the
depricate-v6-class-backdrop
branch
from
b59e68b to
3d2840b
Compare
harry-whorlow
force-pushed
the
depricate-v6-class-backdrop
branch
from
cd51367 to
c10f61c
Compare
github-actions
bot
removed
the
PR: out-of-date
|
Hey @harry-whorlow, great work! This is ready to merge 🎉. Next week, we'll freeze v5 and open the If you're on board, I'll ask you to wait a bit longer, and when the |
|
@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 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 |
|
@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! |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
|
Hey @harry-whorlow! I pointed the PR to the 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 |
harry-whorlow
force-pushed
the
depricate-v6-class-backdrop
branch
from
0e1dbcf to
8766587
Compare
|
@DiegoAndai like so? 🤟 |
harry-whorlow
force-pushed
the
depricate-v6-class-backdrop
branch
from
8766587 to
96a3c94
Compare
github-actions
bot
added
the
PR: out-of-date
Signed-off-by: Harry Whorlow <[email protected]>
github-actions
bot
removed
the
PR: out-of-date
Yes 👌🏼 I pushed some fixes, I'm doing the some final testing 👍🏼 |
DiegoAndai
force-pushed
the
depricate-v6-class-backdrop
branch
from
e960418 to
556158c
Compare
|
Ah... you beat me to the capitalisation. The fastest hands in the west |
There was a problem hiding this comment.
LGTM 🚀
Good job @harry-whorlow! I appreciate your drive to keep working on this through multiple revisions. I hope you're motivated to keep contributing. Congrats 🎉
|
@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! 🚀 |
Part of: #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.