[Icon] fontSize prop not applied to Icon component

jonomatusky commented 3 years ago

[x] The issue is present in the latest release.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The fontSize prop does not affect the component. Using sx doesn't affect size either. Tested in Chrome 89.0 and Safari 14.0.

See example in the new docs. The fontSize prop doesn't impact the size of the icons:

https://next.material-ui.com/components/icons/#font-material-icons

Expected Behavior 🤔

Expect same behavior as in v4, displayed in the current docs:

https://next.material-ui.com/components/icons/#font-material-icons

Your Environment 🌎

`npx @material-ui/envinfo`

System: OS: macOS 10.15.7 Binaries: Node: 14.15.0 - ~/.nvm/versions/node/v14.15.0/bin/node Yarn: Not Found npm: 7.6.0 - ~/Software/plynth/Current/plynth/plynth-frontend/node_modules/.bin/npm Browsers: Chrome: 89.0.4389.128 Edge: Not Found Firefox: 81.0.1 Safari: 14.0.3 npmPackages: @emotion/react: ^11.1.5 => 11.1.5 @emotion/styled: ^11.1.5 => 11.1.5 @material-ui/core: ^5.0.0-alpha.27 => 5.0.0-alpha.27 @material-ui/icons: ^5.0.0-alpha.28 => 5.0.0-alpha.28 @material-ui/lab: ^5.0.0-alpha.27 => 5.0.0-alpha.27 @material-ui/styled-engine: 5.0.0-alpha.25 @material-ui/styles: 5.0.0-alpha.27 @material-ui/system: 5.0.0-alpha.27 @material-ui/types: 5.1.7 @material-ui/unstyled: 5.0.0-alpha.27 @material-ui/utils: 5.0.0-alpha.27 @types/react: 17.0.2 react: ^17.0.2 => 17.0.2 react-dom: ^17.0.1 => 17.0.1

oliviertassinari commented 3 years ago

@jonomatusky Thanks for opening an issue so we can have a focused discussion on this problem. We have been aware of this problem for a while.

On the surface, it's a problem specific to the documentation. It works once you open the demo in codesandbox: https://codesandbox.io/s/b8ve3?file=/index.tsx. The documentation needs to set injectFirst to support all the legacy makeStyles/withStyles code we still have: #16947.

At the root cause, it's an issue with emotion that doesn't provide the option to determine the exact injection location of the styles in the <head>. @mnajdova I couldn't find the past discussion we had on this problem. Would it make sense to open a new issue on emotion? I thought there was already one but I couldn't find it. For styled-components, this problem is solved.

mnajdova commented 3 years ago

As far as I remember this is an issue during the transition period, where JSS is currently applied after emotion so that the overrides would work. I think emotion should always be added after the icons's styles is loaded, isn't that right?

oliviertassinari commented 3 years ago

@mnajdova Correct.

Based on the amount of JSS we have in the documentation, it might take a while before we can fix the docs (what I called the surface issue as opposed to the root).

mnajdova commented 3 years ago

I can't seem to find the discussion, I remember that the initial implementation of StylesProvider (back in emotion v10) didn't have the prepend option so we manually were adding the style tag - https://github.com/mui-org/material-ui/pull/23934/files#diff-679c878910ba4ea2dbd14791e3ffb71dd394886d694de7991083b9069612bee6R10 but then with the upgrade do 11, we changed it to prepend. I don't remember discussing that we need to have exact injection location, but I may be wrong.

luminaxster commented 3 years ago

This is not relevant but related: On release 5.0.0-alpha.24, a breaking change was listed as non-breaking:

[styled-engine] Rename StylesProvider to StyledEngineProvider (#24429) @mnajdova

The failure is now observable on v5.0.0-alpha.34:

[core] Move StyledEngineProvider to @material-ui/core/styles (#26265) @mnajdova

I confused the two providers:

import {
   ThemeProvider,
   StyledEngineProvider, // StylesProvider rename missing as breaking change in  log
   CssBaseline,
} from '@material-ui/core';
// import { StylesProvider } from '@material-ui/styles'; // I confused it with this change

mnajdova commented 3 years ago

@luminaxster thanks for the feedback. It was a mistake at the start that we went with the same name StylesProvider. Regarding the changelog:

On release 5.0.0-alpha.24, a breaking change was listed as non-breaking:

[styled-engine] Rename StylesProvider to StyledEngineProvider ([styled-engine] Rename StylesProvider to StyledEngineProvider #24429) @mnajdova

Good catch! Seems like the PR description contain the BC description, but the changelog not.

~@siriwatknp may be a good opportunity for fixing this in https://github.com/mui-org/material-ui/pull/26948~ Nevermind, it was added in v5-alpha

mui / material-ui