[IconButton] Improve size values

[ghost]

Current Behavior 😯

When giving IconButton size properties medium and larger the size class is not set.

Expected Behavior 🤔

That MuiIconButton get MuiIconButton-sizeMedium and MuiIconButton-sizeLarge the same way as MuiIconButton-sizeSmall

Steps to Reproduce 🕹

<IconButton size="large">
  <FontAwesomeIcon icon={faPlay} />
  <FontAwesomeIcon icon={faChevronRight} />
</IconButton>

<IconButton size="medium">
  <FontAwesomeIcon icon={faPlay} />
  <FontAwesomeIcon icon={faChevronRight} />
</IconButton>

Your Environment 🌎

`System: OS: macOS 11.0.1 Binaries: Node: 12.18.4 - /usr/local/bin/node Yarn: 1.22.5 - ~/.yarn/bin/yarn npm: 6.14.9 - /usr/local/bin/npm Browsers: Chrome: 87.0.4280.88 * Edge: Not Found * Firefox: Not Found Safari: 14.0.1 npmPackages: @emotion/styled: 10.0.27 @material-ui/core: ^4.11.2 => 4.11.2 @material-ui/styles: 4.11.2 @material-ui/system: 4.11.2 @material-ui/types: 5.1.0 @material-ui/utils: 4.11.2 @types/react: ^16.9.53 => 16.14.2 react: ^17.0.1 => 17.0.1 react-dom: ^17.0.1 => 17.0.1 styled-components: ^5.2.1 => 5.2.1 typescript: ^4.0.3 => 4.1.3 `

[oliviertassinari]

@hpeide You will find the same class name behavior in all the components. The default, medium font-size class name isn't applied.

https://github.com/mui-org/material-ui/blob/259253daff62c4c643ae53de5ecce9b2db21c116/packages/material-ui/src/IconButton/IconButton.js#L115

Instead, the components assume that developers write the style of the baseline to match the medium font size. Did you try this approach? We have used Bootstrap as inspiration, in their case, they have to optimize for fewer class names to apply.
Why should it behave differently in your perspective?

Also note that the IconButton only supports two sizes, small and medium. cc @mnajdova.

[ghost]

Thanks! yep, I like that approach, and will discuss with our designer if we really need three different icon sizes. Think what confused me was that the Button component returns class name MuiButton-textSizeSmall and MuiButton-textSizeLarge and I expected the IconButton to do the same.

[mnajdova]

In my opinion, it makes sense for us now to generate all class names, both for the default and non-default props values. The biggest reason for it is, if someone changes their defaults, for example, their default size to be large, they wouldn't be able to change the styles for the medium as they will not be able to select an appropriate global class name. For v5 at least we will make sure that we have global classes for all states, both default and non default.

Regarding the size for the IconButton, I think it would make sense that we support three sizes, as we do with the other buttons.

[oliviertassinari]

In my opinion, it makes sense for us now to generate all class names, both for the default and non-default props values.

@mnajdova I would personally encourage we follow Bootstrap, reducing the number of classname anytime there is an opportunity for, example designers don't need size medium, they can customize the default styles.

Regarding the size of the IconButton, Material Design does no longer document this component. It seems that we can take some freedom. I have noticed that Google, on its product on desktop is reducing the size of the element. If you take Google Calendar, alone, you can find 5 different sizes:

• 
• 
• 
• 
• 

I think that we should do the following:

• small: 32px -> padding: 5px
• medium: 40px -> padding: 8px (default)
• large: 48px -> padding 12px

@hpeide Do you want to work on the problem? The current default at 48px is too much.

[oliviertassinari]

Re-opening as it's a broader issue than the Button component.

[sakura90]

please let me work on this issue

[oliviertassinari]

@sakura90 Sounds great, @siriwatknp could likely provide guidance

[sakura90]

I think that we should do the following:

small: 32px -> padding: 5px
medium: 40px -> padding: 8px (default)
large: 48px -> padding 12px

Have some private things to do, so I am going to be unavailable for atleast about a week. Please feel free to continue the work. The next change is going to address the above step.

For what I have done so far, padding: 5px might be better to be padding: 4px. From what I understand, material design prefers an unit as 4px.

About breaking change, I think backward compatibility is what to look. I know a lot of libraries recently do not have backward compatibility, so maybe backward compatibility is not very big. But I think developers are going to feel more comfortable when their mui apps look the same after switching from v4 to v5. If making small/medium/large become depreciated and adding mui-small, mui-medium, mui-large are possible, it will be nice.

I am going to continue working on the above step when I am back if nobody works on it.

[oliviertassinari]

From what I understand, material design prefers an unit as 4px.

For the overall component size of the component, yes. Not for the implementation details that this padding value is. If we use 4px instead of 5px, the component is now 30px height, which doesn't fit (32/4=8).

But I think developers are going to feel more comfortable when their mui apps look the same after switching from v4 to v5. If making small/medium/large become depreciated and adding mui-small, mui-medium, mui-large are possible, it will be nice.

I don't think that we need to worry about this as long as the component looks great. Better introduced breaking changes if it helps the UX/UI. We can mitigate the migration pain with docs and a codemod.

[siriwatknp]

@oliviertassinari @sakura90 I think 5px looks good to me. Currently, IconButton size 'small' provide has 18px fontsize.

	inherit	small[20px]	medium[24px] (default)	large[35px]
small[5px > 18px]	28px	30px	36px	47px
medium[8px] (default)	-	36px	40px	51px
large[12px > 28px]	52px	44px	48px	59px

Note: first column refers to and first row refers to
the number in the cell is the size (width x height) of the IconButton (padding + icon combined)
so the smallest size of IconButton is 28px and the largest is 59px.

[sakura90]

For the overall component size of the component, yes. Not for the implementation details that this padding value is. If we use 4px instead of 5px, the component is now 30px height, which doesn't fit (32/4=8).

Thanks for explaining to me. Actually I was not familiar with material design and wrote that padding value was preferred to be in a unit as 4px. I did not have enough time to read through the material design conccepts and made a comment with inssufficient understanding. You are correct that the overal component should be in a unit of 4px not the padding.

I don't think that we need to worry about this as long as the component looks great. Better introduced breaking changes if it helps the UX/UI. We can mitigate the migration pain with docs and a codemod.

I am currently in Japan and a lot of Japanese developers prefer correctness actually. Those people prefer having no change in the UI after upgrading from v4 to v5. Nevertheless, practically speaking, there probably are lots of development scenarios in Japan where the UI changes a bit after upgrading from one version to another in a library. I guess not having further discussion on this topic is okay.

[sakura90]

I think 5px looks good to me.

@siriwatknp Nice to know.