[material] Add missing size classes to components

DiegoAndai commented 1 year ago

Duplicates

[X] I have searched the existing issues

Latest version

[X] I have tested the latest version

Steps to reproduce 🕹

For the Radio, InputBase, InputLabel, and Slider components, the size classes are not standardized.

Link to live example:https://codesandbox.io/s/radio-size-classes-report-5zplt2?file=/src/App.tsx

Steps:

Use a Radio component with size="small"

Current behavior 😯

The MuiRadio-sizeSmall class is not applied

Expected behavior 🤔

The size classes should be added to the root component, for all the possible sizes of the component.

Slider and InputBase only applies the "small" class, missing the "medium" one
InputLabel only applies the "small" class, missing the "normal" one.
Radio doesn't apply either

This is how the Button component behaves and should be the standard.

the fix should be similar to https://github.com/mui/material-ui/pull/38182

Context 🔦

There are other components that have a size prop and don't apply the class, but I didn't add them here because these only forward the prop to other components (for example ButtonGroup forwards it to Button), so those shouldn't apply the classes.

Your environment 🌎

npx @mui/envinfo

``` Cross-browser System: OS: macOS 13.4.1 Binaries: Node: 18.16.1 - ~/.nvm/versions/node/v18.16.1/bin/node Yarn: 1.22.19 - ~/.nvm/versions/node/v18.16.1/bin/yarn npm: 9.5.1 - ~/.nvm/versions/node/v18.16.1/bin/npm Browsers: Chrome: 115.0.5790.170 Edge: Not Found Safari: 16.5.1 npmPackages: @emotion/react: ^11.11.1 => 11.11.1 @emotion/styled: ^11.11.0 => 11.11.0 @mui/base: 5.0.0-beta.9 @mui/codemod: 5.14.1 @mui/core-downloads-tracker: 5.14.3 @mui/docs: 5.14.3 @mui/envinfo: 2.0.9 @mui/icons-material: 5.14.3 @mui/internal-waterfall: 1.0.0 @mui/joy: 5.0.0-beta.0 @mui/lab: 5.0.0-alpha.138 @mui/markdown: 5.0.0 @mui/material: 5.14.3 @mui/material-next: 6.0.0-alpha.95 @mui/private-theming: 5.13.7 @mui/styled-engine: 5.13.2 @mui/styled-engine-sc: 5.12.0 @mui/styles: 5.14.3 @mui/system: 5.14.3 @mui/types: 7.2.4 @mui/utils: 5.14.3 @mui/x-data-grid: 6.10.2 @mui/x-data-grid-generator: 6.10.2 @mui/x-data-grid-premium: 6.10.2 @mui/x-data-grid-pro: 6.10.2 @mui/x-date-pickers: 6.10.2 @mui/x-date-pickers-pro: 6.10.2 @mui/x-license-pro: 6.10.2 @types/react: ^18.2.17 => 18.2.17 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 styled-components: 5.3.11 typescript: ^4.9.5 => 4.9.5 ```

sai6855 commented 1 year ago

Hey @DiegoAndai if you haven't started yet, can I work on this issue

DiegoAndai commented 1 year ago

@sai6855 please do 😊

ZeeshanTamboli commented 1 year ago

@DiegoAndai, I recalled an unfinished PR - https://github.com/mui/material-ui/pull/34655 for TextField component which uses InputBase under the hood. I had raised the same problem but there is a discussion here mentioning avoiding extra classes for default values to keep the HTML concise. However, I agree that consistency is important if it's done in the Button component. You can check the related issue of the PR at https://github.com/mui/material-ui/issues/34504 (but that is for className when a custom prop is passed).

DiegoAndai commented 1 year ago

@ZeeshanTamboli thanks for bringing it up! Seems like we need to decide on a standard 🤔

@mnajdova is for not adding classes for default values, and custom prop values should receive classes, I think that makes sense if we choose that as a standard. I would suggest:

For v5, only add the missing "small" class to the Radio button
For v6, enforce the standard of not adding the default value class for any prop. So for example, we would remove sizeMedium from the Button. We should have a test for it so it stays consistent.

Does that make sense to everybody?

ZeeshanTamboli commented 1 year ago

Does that make sense to everybody?

Makes sense to me. 👍

mnajdova commented 1 year ago

For v5, only add the missing "small" class to the Radio button

Yep, this is missing and it seems like a bug.

For v6, enforce the standard of not adding the default value class for any prop. So for example, we would remove sizeMedium from the Button. We should have a test for it so it stays consistent.

Yep, makes sense 👍

mui / material-ui