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

[IconButton] Add size large and update styles #26748

Merged
merged 14 commits into from
Jun 17, 2021
Merged

[IconButton] Add size large and update styles #26748

merged 14 commits into from
Jun 17, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 14, 2021 •
edited by oliviertassinari
Loading

BREAKING CHANGE

  • The default size's padding is reduced to 8px which makes the default IconButton size of 40px. To get the old default size (48px), use size="large". The change was done to better match Google's products when Material Design stopped documenting the icon button pattern.

    -<IconButton>
+<IconButton size="large">

close #24045

🔗 https://deploy-preview-26748--material-ui.netlify.app/

  • add size large
  • update padding on small and medium size
  • update ButtonSizes demo to include <IconButton size="large">
  • add to migration-v4.md about breaking change

One thing I notice, should label be removed from IconButton same as #26666

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

@siriwatknp siriwatknp added breaking change component: icon button This is the name of the generic UI component, not the React module! labels Jun 14, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 14, 2021 •
edited
Loading

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 92f3876

mnajdova
mnajdova reviewed Jun 14, 2021
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The argo's diffs look expected. I would just simplify the ButtonSizes example.

@oliviertassinari oliviertassinari changed the title [IconButton] add size large and update styles [IconButton] Add size large and update styles Jun 14, 2021
oliviertassinari
oliviertassinari reviewed Jun 14, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The change definitely yields a different UX when browsing the page. It feels different but it seems OK

This might be off. It doesn't feel vertically aligned:

Capture d’écran 2021-06-15 à 01 01 58

docs/pages/api-docs/icon-button.json Show resolved Hide resolved
packages/material-ui/src/IconButton/IconButton.js Outdated Show resolved Hide resolved
docs/src/pages/components/buttons/ButtonSizes.js Outdated Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

The change definitely yields a different UX when browsing the page. It feels different but it seems OK

I adjust those IconButton to have size="large" to remain the same looks and feel. please review again.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 15, 2021 •
edited
Loading

Overall argos looks fine to me, only TextField adornment that looks a bit weird.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 15, 2021 •
edited
Loading

Overall argos looks fine to me, only TextField adornment that looks a bit weird.

@siriwatknp Actually, I believe the icon button on the app bar should be size="large". It seems to be what Google uses on its products. How about we change it?

only TextField adornment that looks a bit weird.

It feels different, I would personally vote for saying it's OK. But these screenshots, x2 the size on GitHub's comment are disturbing 😁

You increased the size of the padding, but it doesn't feel aligned:

Capture d’écran 2021-06-15 à 12 05 17

I think that it was more visible with the size="medium" but this wasn't the root cause.

@siriwatknp
Copy link
Member Author

I believe the icon button on the app bar should be size="large"

updated the docs AppBar and also some demos in AppBar that make sense to use <IconButton size="large">

@siriwatknp
Copy link
Member Author

I believe the icon button on the app bar should be size="large"

updated the docs AppBar and also some demos in AppBar that make sense to use <IconButton size="large">

You increased the size of the padding, but it doesn't feel aligned:

It is aligned now.

mnajdova
mnajdova reviewed Jun 16, 2021
View reviewed changes
docs/src/pages/guides/migration-v4/migration-v4.md

```diff
-<Icon fontSize="default">icon-name</Icon>
+<Icon>icon-name</Icon>
```

### IconButton

- The default size's padding is reduced to `8px` which makes the default IconButton size of `40px`. To get the old default size (`48px`), use `size="large"`. The change was done to better match Google's products when Material Design stopped documenting the icon button pattern.
Copy link
Member

Choose a reason for hiding this comment

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

nit: the font size is a bit different for the size: 'large' too, but not sure if it is worth mentioning.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you point to me what is the different?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. I think it is fine not to mention.

@siriwatknp siriwatknp merged commit fb960fa into mui:next Jun 17, 2021
@siriwatknp siriwatknp deleted the fix/icon-button-sizes branch June 17, 2021 01:58
@siriwatknp siriwatknp mentioned this pull request Jun 23, 2021
Merged
1 task
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

@mnajdova mnajdova mnajdova approved these changes

Assignees
No one assigned
Labels
breaking change component: icon button This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[IconButton] Improve size values
4 participants
@siriwatknp @mui-pr-bot @oliviertassinari @mnajdova