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 should not have hardcoded font size, it prevents them from scaling with text #11493

Closed
2 tasks done
jedwards1211 opened this issue May 19, 2018 · 5 comments
Closed
2 tasks done
Labels
component: Icon The React component. discussion

Comments

@jedwards1211
Copy link
Contributor

jedwards1211 commented May 19, 2018

  • This is a v1.x issue (v0.x is no longer maintained).
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Icons (both the <Icon> and specific icon components like <ChevronRight>) inherit their parent's fontSize so that they scale with the text

Current Behavior

Icons have a hardcoded fontSize of 24px. This makes width: 1em and height: 1em pointless, because the em square is fixed to the element's own fontSize of 24px instead of the parent's font size.

Steps to Reproduce (for bugs)

In this example I have two icons inside of an <h1> with fontSize: 4rem, and also inside of another <h1> with fontSize: 3rem. The first icon is default and fails to scale to the parent font size. The second icon has CSS overrides to produce the expected behavior.
https://codesandbox.io/s/8n27qrv9q8

image

Workaround (in theme.overrides)

  MuiIcon: {
    root: {
      fontSize: 'inherit',
    },
  },
  MuiSvgIcon: {
    root: {
      fontSize: 'inherit',
      verticalAlign: 'middle',
    },
  },

Context

As seen in the example, I am using inline chevron icons as part of a breadcrumb component.

Your Environment

Tech Version
Material-UI 1.0.0-beta or 1.0.0-rc.1
React 16.2.0
browser Chrome 66.0.3359.181
jedwards1211 added a commit to jedwards1211/material-ui that referenced this issue May 19, 2018
@oliviertassinari oliviertassinari added out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) and removed out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) labels May 19, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented May 19, 2018

@jedwards1211 We have been doing some back and forth on this point in the past: #10349.

This makes width: 1em and height: 1em pointless,

The point is that you can apply a font size instead of changing the width and the height.

Icons (both the and specific icon components like ) inherit their parent's fontSize so that they scale with the text

This is a tradeoff. We follow the same one than the material font icons set: https://fonts.googleapis.com/icon?family=Material+Icons

...

.material-icons {
  ...
  font-size: 24px;
  ..
}

Also, this is an important breaking change, something we don't want to introduce for the next 6 months+.

Maybe we should add a property to the component: fontSize?: 'inherit' | 'default'.

@jedwards1211
Copy link
Contributor Author

@oliviertassinari I like the component property idea. Considering that 1em behaves pretty much the same as inherit, it would also be nice if the property passes values other than 'inherit' | 'default' straight to the CSS.

One problem: since both the <Icon> wrapper component and the SVG icons set their own fontSize, one would have to pass the property on both, which is kind of annoying. Can you think of any way to work around this?

@oliviertassinari
Copy link
Member

one would have to pass the property on both

@jedwards1211 Why would one have to pass the property on both? Shouldn't a user pick on approach, font vs svg and stick to it?

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented May 20, 2018

@oliviertassinari oh...seems I thought I had to wrap my svg icons in <Icon> for some reason, was this a relic of an old way of doing things?

@oliviertassinari
Copy link
Member

@jedwards1211 I don't think that we document this pattern anywhere in the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Icon The React component. discussion
Projects
None yet
Development

No branches or pull requests

2 participants