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.

Sign up for GitHub

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

Jump to bottom

[material-ui][Autocomplete] deprecate *Component and *Props for v6 #41875

Merged
merged 24 commits into from May 8, 2024
Merged

[material-ui][Autocomplete] deprecate *Component and *Props for v6 #41875

merged 24 commits into from May 8, 2024

Conversation

lhilgert9
Copy link
Contributor

@lhilgert9 lhilgert9 commented Apr 12, 2024 •
edited

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.

@mui-bot
Copy link

mui-bot commented Apr 12, 2024 •
edited

Netlify deploy preview

Autocomplete: parsed: +0.73% , gzip: +0.77%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against d859029

@lhilgert9 lhilgert9 marked this pull request as ready for review April 12, 2024 20:32
@zannager zannager added the component: autocomplete This is the name of the generic UI component, not the React module! label Apr 15, 2024
@DiegoAndai DiegoAndai mentioned this pull request Apr 17, 2024
Open
34 tasks
DiegoAndai
DiegoAndai reviewed Apr 24, 2024
View reviewed changes
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.

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.

packages/mui-material/src/Autocomplete/Autocomplete.d.ts Outdated Show resolved Hide resolved
packages/mui-material/src/Autocomplete/Autocomplete.d.ts Show resolved Hide resolved
packages/mui-material/src/Autocomplete/Autocomplete.js Outdated Show resolved Hide resolved
@DiegoAndai
Copy link
Member

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.

Let's do it in a separate follow-up PR to keep things orderly 😊

@DiegoAndai DiegoAndai mentioned this pull request Apr 24, 2024
Closed
1 task
@DiegoAndai
Copy link
Member

Requested review from @michaldudak as the owner of the Autocomplete component.

michaldudak
michaldudak reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@michaldudak michaldudak left a 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.

docs/pages/material-ui/api/autocomplete.json Outdated Show resolved Hide resolved
packages/mui-material/src/Autocomplete/Autocomplete.js Outdated Show resolved Hide resolved
@lhilgert9
Copy link
Contributor Author

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?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 26, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 26, 2024
@lhilgert9 lhilgert9 closed this Apr 26, 2024
@lhilgert9 lhilgert9 deleted the deprecate-autocomple-props branch April 26, 2024 18:51
@lhilgert9 lhilgert9 restored the deprecate-autocomple-props branch April 26, 2024 18:52
@lhilgert9
Copy link
Contributor Author

Sorry, I just wanted to rename the branch and the PR has closed.

@lhilgert9 lhilgert9 reopened this Apr 26, 2024
@lhilgert9
Copy link
Contributor Author

@DiegoAndai I have an idea to improve the CreateSlotsAndSlotProps type by rewriting the slots as Partial:

- slots?: Slots
+ slots?: Partial<Slots>

For example, it happened to me now that I forgot to make the slots optional and that wouldn't happen that way.

@DiegoAndai
Copy link
Member

Hey @lhilgert9!

Should I also change the demos in this PR?

Yes, please 😊

I have an idea to improve the CreateSlotsAndSlotProps type by rewriting the slots as Partial

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 🙌🏼

@lhilgert9
Copy link
Contributor Author

Demos are changed by the codemod so it looks like it works on a real example🚀🚀

@lhilgert9 lhilgert9 mentioned this pull request May 1, 2024
Open
1 task
michaldudak
michaldudak approved these changes May 2, 2024
View reviewed changes
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

LGTM

DiegoAndai
DiegoAndai reviewed May 3, 2024
View reviewed changes
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.

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
DiegoAndai
DiegoAndai approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment •
edited

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 🙌🏼

@lhilgert9
Copy link
Contributor Author

Let me know if these changes make sense to you and I'll merge 🙌🏼

LGTM
Just pushed pnpm docs:api. I have no idea why e2e fails now but I think we can merge the PR.🚀

@DiegoAndai DiegoAndai merged commit d8a5f53 into mui:next May 8, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak approved these changes

@DiegoAndai DiegoAndai DiegoAndai approved these changes

Assignees
No one assigned
Labels
component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lhilgert9 @mui-bot @DiegoAndai @michaldudak @zannager