-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Autocomplete] deprecate *Component
and *Props
for v6
#41875
Conversation
Netlify deploy previewAutocomplete: parsed: +0.73% , gzip: +0.77% Bundle size reportDetails of bundle changes (Toolpad) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lhilgert9, thank you so much for working on this, great work. Sorry for the delay in the review.
I just have some minor questions, I leave them below.
Let's do it in a separate follow-up PR to keep things orderly 😊 |
Co-authored-by: Diego Andai <[email protected]> Signed-off-by: Lucas Hilgert <[email protected]>
Requested review from @michaldudak as the owner of the Autocomplete component. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make sure the demos don't reference deprecated props. I found the GitHub label and Virtualize demos still use them.
@michaldudak Should I also change the demos in this PR? |
Sorry, I just wanted to rename the branch and the PR has closed. |
@DiegoAndai I have an idea to improve the CreateSlotsAndSlotProps type by rewriting the slots as Partial:
For example, it happened to me now that I forgot to make the slots optional and that wouldn't happen that way. |
Hey @lhilgert9!
Yes, please 😊
That's an interesting proposal 🤔 I think I'm on board with it. If you're up to it, let's open a separate PR for it, as we should update the other occurrences 🙌🏼 |
Demos are changed by the codemod so it looks like it works on a real example🚀🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @lhilgert9, one last thing about the migration guide:
docs/data/material/migration/migrating-from-deprecated-apis/migrating-from-deprecated-apis.md
Show resolved
Hide resolved
This reverts commit 1a8ebb9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @lhilgert9! Thanks for updating the demos 😊
I pushed some updates:
- Minor copy updates
- Revert the deprecated tests changes, as we want to keep those until the deprecated props are removed
- Added some slots tests
Let me know if these changes make sense to you and I'll merge 🙌🏼
LGTM |
*Component
and *Props
for v6*Component
and *Props
for v6
Part of #41281.
@DiegoAndai
The question is whether we should also deprecate the components and componentProps in this PR at the same time, as that wouldn't be much more work.