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

[Icon] Add a fontSize prop which accepts default and inherit #11986

Merged
merged 7 commits into from
Jun 26, 2018
Merged

[Icon] Add a fontSize prop which accepts default and inherit #11986

merged 7 commits into from
Jun 26, 2018

Conversation

sakulstra
Copy link
Contributor

I realized that in every project I use, I write a wrapper around SvgIcon to basically provide what @oliviertassinari suggested in #11493 (comment) so why not just open a pr to provide it :)

@@ -32,6 +31,12 @@ export const styles = theme => ({
colorDisabled: {
color: theme.palette.action.disabled,
},
fontSizeDefault: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it probably makes more sense to set this inside root and remove the whole fontSizeDefault class, does it?

Copy link
Member

Choose a reason for hiding this comment

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

I agree 👍

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.

@sakulstra We have been doing some back and forth here. This sounds like a good idea, I already had a use case required it. If you need it every single time, maybe you can consider the theme.overrides feature or after this pull request the theme.props feature.
Can we:

  • Add this property to the Font component
  • Update the TypeScript definitions

Thanks!

@@ -32,6 +31,12 @@ export const styles = theme => ({
colorDisabled: {
color: theme.palette.action.disabled,
},
fontSizeDefault: {
Copy link
Member

Choose a reason for hiding this comment

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

I agree 👍

@@ -48,6 +54,7 @@ function SvgIcon(props) {

const className = classNames(
classes.root,
classes[`fontSize${capitalize(fontSize)}`],
Copy link
Member

Choose a reason for hiding this comment

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

Can be simplified following your previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean like we do with color or sth more static like: classes.fontSizeInherit: fontSize === 'inherit'?

@oliviertassinari oliviertassinari added new feature New feature or request component: Icon The React component. component: SvgIcon The React component. labels Jun 26, 2018
@sakulstra
Copy link
Contributor Author

Add this property to the Font component

which font component?

Update the TypeScript definitions

Isn't this what I did here? https://github.com/mui-org/material-ui/pull/11986/files#diff-87364477e84c8259e0ccf7a194346096 I actually never used typeScript :/

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 26, 2018

which font component?

https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Icon/Icon.js Is the equivalent of the SvgIcon component but using the font icon.

Isn't this what I did here? https://github.com/mui-org/material-ui/pull/11986/files#diff-87364477e84c8259e0ccf7a194346096 I actually never used typeScript :/

👍

@sakulstra
Copy link
Contributor Author

sakulstra commented Jun 26, 2018

@oliviertassinari ah thanks for pointing out - never used the Icon component before :)

Should be all fixed now. If I find some time, i'll have a look at the theme.override feature in another pr.
What do I have to do regarding argos?

@oliviertassinari
Copy link
Member

If I find some time, i'll have a look at the theme.override feature in another pr.

@sakulstra It's a userland work only:

const theme = createMuiTheme({
  overrides: {
    MuiSvgIcon: {
      root: {
        fontSize: 'inherit',
      }
    }
  }
})

After this pull request, you can do:

const theme = createMuiTheme({
  props: {
    MuiSvgIcon: {
      fontSize: 'inherit',
    }
  }
})

@oliviertassinari
Copy link
Member

What do I have to do regarding argos?

Argos? You need to run yarn docs:api and to increase the size-limit.config.js threshold.

@oliviertassinari oliviertassinari changed the title [SvgIcon] add a fontSize prop which accepts default and inherit [Icon] Add a fontSize prop which accepts default and inherit Jun 26, 2018
increase size by 0.1kb
@oliviertassinari oliviertassinari merged commit 585b099 into mui:master Jun 26, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 26, 2018

@sakulstra Great job!

acroyear pushed a commit to acroyear/material-ui that referenced this pull request Jul 14, 2018
* feat(icons): add a fontSize prop which accepts default and inherit

* add test

* remove default Class

* add fontSize to icon as well

* add test for icon as well

* regenerate api docs

* increase size

increase size by 0.1kb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Icon The React component. component: SvgIcon The React component. new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants