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

[Icon] fontSize prop not applied to Icon component #25829

Open
2 tasks done
jonomatusky opened this issue Apr 18, 2021 · 6 comments
Open
2 tasks done

[Icon] fontSize prop not applied to Icon component #25829

jonomatusky opened this issue Apr 18, 2021 · 6 comments
Labels
component: Icon The React component. docs Improvements or additions to the documentation package: styled-engine Specific to @mui/styled-engine

Comments

@jonomatusky
Copy link

jonomatusky commented Apr 18, 2021 •
edited by oliviertassinari
Loading

  • The issue is present in the latest release.
  • 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:

Screen Shot 2021-04-18 at 8 21 29 AM

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

Expected Behavior 🤔

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

Screen Shot 2021-04-18 at 8 22 26 AM

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
@jonomatusky jonomatusky added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Apr 18, 2021
@oliviertassinari oliviertassinari added package: styled-engine Specific to @mui/styled-engine component: Icon The React component. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 18, 2021
@oliviertassinari oliviertassinari changed the title fontSize prop not applied to Icon component [Icon] fontSize prop not applied to Icon component Apr 18, 2021
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Apr 18, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 18, 2021 •
edited
Loading

@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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

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
Copy link
Contributor

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

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

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
Copy link
Member

mnajdova commented Jun 29, 2021 •
edited
Loading

@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 #26948
Nevermind, it was added in v5-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component: Icon The React component. docs Improvements or additions to the documentation package: styled-engine Specific to @mui/styled-engine
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@oliviertassinari @jonomatusky @mnajdova @luminaxster