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

[material-ui][Select] Fix variant type #41405

Merged
merged 10 commits into from Mar 14, 2024
Merged

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Mar 8, 2024

closes #41401
closes #41356
closes #41375
closes #41410

@sai6855 sai6855 added component: select This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material regression A bug, but worse labels Mar 8, 2024
@sai6855 sai6855 marked this pull request as draft March 8, 2024 05:04
@mui-bot
Copy link

mui-bot commented Mar 8, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 62ce5b4

@sai6855 sai6855 marked this pull request as ready for review March 8, 2024 05:24
@sai6855 sai6855 requested a review from DiegoAndai March 8, 2024 05:24
@sai6855 sai6855 added bug 🐛 Something doesn't work typescript labels Mar 8, 2024
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 working on this @sai6855, the change looks good for the problem it solves.

I wonder if you have thoughts about #41356. Any ideas on how to solve that?

@sai6855

This comment was marked as off-topic.

@sai6855

This comment was marked as outdated.

@DiegoAndai
Copy link
Member

This looks good @sai6855! #41401 and #41356 are fixed. For #41410, we're missing the error on line 64 of this playground:

Screenshot 2024-03-11 at 15 30 30

If we don't fix that one, then I don't know how users will be able to wrap the Select component, which is a common use case.

There's another error on line 73 for which I couldn't find a fix, but we can accept that one as a TS limitation.

cc: @michaldudak

@sai6855 sai6855 force-pushed the select-type branch 3 times, most recently from 636740c to f4ccdb4 Compare March 12, 2024 09:38
@sai6855
Copy link
Contributor Author

sai6855 commented Mar 12, 2024

@DiegoAndai #41410 is fixed now, i changed implementation by removing Variant argument entirely (it's causing more problems then solving) from Select and SelectProps and retained ability to add hiddenLabel prop to only filled variant.

I hope i covered all possbile scenarios in tests, but let me know if you see anything missing.

Also updated issues in PR description

@sai6855 sai6855 requested a review from DiegoAndai March 12, 2024 13:54
@DiegoAndai
Copy link
Member

Thanks, @sai6855, great work managing all those issues. I think this is the best solution. I agree that the Variant argument seems to be causing more problems than it fixes.

I would like to get @michaldudak's opinion on this before merging.

Also, I'll leave a comment at #39137 to see if people involved in that PR have any opinions about this change.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Since I was involved in reviewing #39137, I also reviewed this PR. I hope you don't mind. The fixes seem good to me. Great job, @sai6855!

packages/mui-material/src/Select/Select.d.ts Show resolved Hide resolved
packages/mui-material/src/Select/Select.spec.tsx Outdated Show resolved Hide resolved
@sai6855 sai6855 force-pushed the select-type branch 2 times, most recently from 268d82e to fd053d4 Compare March 13, 2024 05:10
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.

This is the kind of PR that I'm usually quite afraid to approve. It looks OK, I couldn't find anything suspicious, but I can't think of all the ways it will be used in the wild and all the potential ways in can break.

I give a green light based on what I'm able to anticipate currently.

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.

This seems to me as the best step forward as well, given the amount of issues it closes which were created with the initial attempt at improving the variant type.

Good job @sai6855

@DiegoAndai DiegoAndai merged commit c25839e into mui:master Mar 14, 2024
19 checks passed
@sai6855 sai6855 deleted the select-type branch March 14, 2024 15:16
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: select This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material regression A bug, but worse typescript
Projects
None yet
5 participants