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][Checkbox] large size added in type #34909

Merged
merged 2 commits into from
Mar 4, 2024
Merged

[material-ui][Checkbox] large size added in type #34909

merged 2 commits into from
Mar 4, 2024

Conversation

smox
Copy link
Contributor

@smox smox commented Oct 27, 2022 •
edited by michaldudak
Loading

Here is a proposal

Fixes #34907

@mui-bot
Copy link

mui-bot commented Oct 27, 2022 •
edited
Loading

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 86d13f6

@zannager zannager added the component: checkbox This is the name of the generic UI component, not the React module! label Oct 28, 2022
@zannager zannager requested a review from michaldudak October 28, 2022 06:43
@michaldudak
Copy link
Member

michaldudak commented Oct 28, 2022 •
edited
Loading

Could you please create a Checkbox.spec.ts file and verify if the change works?
Also, a unit test verifying that the checkbox is actually larger would be great (this will also make sure we don't introduce regressions in the future).

@michaldudak michaldudak added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Oct 28, 2022
@michaldudak michaldudak changed the title Checkbox size large added [Checkbox] Size large added Oct 28, 2022
@michaldudak michaldudak added PR: needs test The pull request can't be merged and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Oct 28, 2022
oliviertassinari
oliviertassinari reviewed Oct 28, 2022
View reviewed changes
packages/mui-material/src/Checkbox/Checkbox.d.ts
@@ -88,7 +88,7 @@ export interface CheckboxProps
* `small` is equivalent to the dense checkbox styling.
* @default 'medium'
*/
size?: OverridableStringUnion<'small' | 'medium', CheckboxPropsSizeOverrides>;
size?: OverridableStringUnion<'small' | 'medium' | 'large', CheckboxPropsSizeOverrides>;
Copy link
Member

Choose a reason for hiding this comment

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

This is not a value supported by default by the Checkbox, it's a user theme extension, so

Suggested change
size?: OverridableStringUnion<'small' | 'medium' | 'large', CheckboxPropsSizeOverrides>;
size?: OverridableStringUnion<'small' | 'medium', CheckboxPropsSizeOverrides>;

Copy link
Member

@michaldudak michaldudak Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

How so? https://codesandbox.io/s/keen-panini-nu7vsh?file=/demo.tsx seems to work with the vanilla theme (sans the type checker, of course).

Copy link
Member

@oliviertassinari oliviertassinari Oct 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From my perspective, a large checkbox makes little sense at the design level. I can't recall one interface where I saw a normal and a large checkbox variant (≠ customizing the default size checkbox to look smaller or bigger), and where it felt awesome, I recall opposite instances. It feels strange in https://blueprintjs.com/docs/#core/components/checkbox and https://chakra-ui.com/docs/components/checkbox/usage#checkbox-sizes.

The SvgIcon has some logic to handle the size large prop but we don't document that large is possible, e.g. https://mui.com/material-ui/react-checkbox/#size. It wasn't done on purpose, nor the actual size it has was designed on purpose. IHMO, if we decide to support the large variant, then we would need to customize it. It could feel better smaller.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This issue was also opened in #36326 (PR #36327) without knowing it was a duplicate. How do we proceed now? If having a large checkbox does not make sense, then logically, we should not support it design-wise by applying it internally via the Icon's fontSize. 

@oliviertassinari oliviertassinari added the package: material-ui Specific to @mui/material label Oct 29, 2022
@sai6855 sai6855 mentioned this pull request Feb 28, 2023
Closed
1 task
@michaldudak michaldudak mentioned this pull request Aug 1, 2023
Closed
2 tasks
@ZeeshanTamboli
Copy link
Member

No decision has been made based on the above discussion, so I'm closing this pull request.

@ZeeshanTamboli ZeeshanTamboli reopened this Mar 4, 2024
@ZeeshanTamboli ZeeshanTamboli added typescript and removed PR: needs test The pull request can't be merged labels Mar 4, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [Checkbox] Size large added [material-ui][Checkbox] large size added in type Mar 4, 2024
ZeeshanTamboli
ZeeshanTamboli approved these changes Mar 4, 2024
View reviewed changes
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.

Given the upvotes and requests in #34907, I believe we can consider supporting the type. Let's discuss further in a separate issue to decide whether to support the feature or not.

@ZeeshanTamboli ZeeshanTamboli merged commit 4e549d0 into mui:master Mar 4, 2024
20 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
@smox @ZeeshanTamboli @mnajdova
[material-ui][Checkbox] large size added in type (mui#34909)
f9363e0 
Co-authored-by: Michael Brunner <michael.brunner@sm0x.org>
Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@ZeeshanTamboli ZeeshanTamboli ZeeshanTamboli approved these changes

@michaldudak michaldudak Awaiting requested review from michaldudak

Assignees
No one assigned
Labels
component: checkbox This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Checkbox] Property size="large" not possible altough it is working
6 participants
@smox @mui-bot @michaldudak @ZeeshanTamboli @oliviertassinari @zannager