Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # 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