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.

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

[icons] Add fontSize small and large #12865

Merged
merged 6 commits into from
Sep 16, 2018

Conversation

JoshuaLicense
Copy link
Contributor

@JoshuaLicense JoshuaLicense commented Sep 13, 2018

I ran into the same issue as #12863 when using a dense Toolbar and an IconButton. This resolves the sizing in the dense Toolbar and closes #12863.

The small & large sizing is 8px either side of the medium size.

Closes #12863

@JoshuaLicense JoshuaLicense changed the title [Icon Button] Add a size prop to control IconButton size [IconButton] Add a size prop to control IconButton size Sep 13, 2018
@mbrookes
Copy link
Member

@JoshuaLicense Nice job! Please could you run yarn docs:api?

@mbrookes mbrookes added new feature New feature or request component: icon button This is the name of the generic UI component, not the React module! labels Sep 13, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 13, 2018

@JoshuaLicense I have rebased your branch on master, it was quite old. What do you think of moving the size property to the icon itself? We could add a padding: 12 to the icon button to restore the default size.

@JoshuaLicense
Copy link
Contributor Author

@mbrookes Sure, forgot about that.

@oliviertassinari Apologies, should be up-to-date now.

I wasn't sure on resizing the Icon component as the actual icon size was not the issue (in this case).

Would adding a fontSize to the sizeSmall and sizeLarge classes to correctly adjust the size accordingly when using fontSize="inherit" on the Icon be better? Which would allow more control of the icon size in the different size buttons?

Example
sizeSmall: {
    width: 40,
    height: 40,
+   fontSize: theme.typography.pxToRem(23),
  },
<IconButton aria-label="Delete" size="small" className={classes.button}>
    <DeleteIcon 
+    fontSize="inherit" 
    />
</IconButton>

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 14, 2018

What do you think of moving the size property to the icon itself?

I'm asking because we could solve two problems at once. Vuetify or Font awesome have a property to change the icons size. Once we have that, the icon button size issue can be solved almost for free.

@oliviertassinari oliviertassinari merged commit fc42dde into mui:master Sep 16, 2018
@oliviertassinari oliviertassinari changed the title [IconButton] Add a size prop to control IconButton size [icons] Add fontSize small and large Sep 16, 2018
@JoshuaLicense JoshuaLicense deleted the Icon-Button-Sizing branch September 17, 2018 18:09
marcelpanse pushed a commit to marcelpanse/material-ui that referenced this pull request Oct 2, 2018
* [Icon Button] Add a size prop to control button size

* Update API

* [icons] Add a size property

* Merge size and fontSize prop

& update docs with an example using IconButton.

* Fix comments incorrectly referring to the size prop

* fix the visual regressions
@Mr-XiaYang
Copy link

Mr-XiaYang commented Oct 13, 2018

image

image

image
@oliviertassinari
"We could add a padding: 12 to the icon button to restore the default size."
Should I change padding: 12 to padding: 8?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: icon button This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants