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

Theme provider wrong import results in inconsitent styling #27628

Closed
stevemk14ebr opened this issue Aug 6, 2021 · 7 comments
Closed

Theme provider wrong import results in inconsitent styling #27628

stevemk14ebr opened this issue Aug 6, 2021 · 7 comments
Labels
status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it v5.x

Comments

@stevemk14ebr
Copy link

stevemk14ebr commented Aug 6, 2021

This one took me quite a while to figure out. What was occurring for me was inconsistent application of style at different levels of the dom hierarchy and different styling across browsers. The reason was the following:

import { ThemeProvider } from '@material-ui/styles';

instead of

import { ThemeProvider } from '@material-ui/core/styles';

symptoms:

  • darkscrollbar did not work on chrome, but did on firefox
  • some nested elements of theme provider did not have access to theme and would instead use the default. I fixed this by doing theme={theme} with the useTheme hook, but this was wrong!

It would be nice if the incorrect import would fail, or at the very least warn the user with a big fat console message. It's very odd debugging something that partially works!

    "@material-ui/core": "^5.0.0-beta.2",
    "@material-ui/icons": "^5.0.0-beta.1",
    "@material-ui/lab": "^5.0.0-alpha.41",
    "@material-ui/styles": "^5.0.0-beta.2",
@stevemk14ebr stevemk14ebr added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 6, 2021
@stevemk14ebr stevemk14ebr changed the title Theme provider import inconsitent styling Theme provider wrong import results in inconsitent styling Aug 6, 2021
@povilass
Copy link
Contributor

povilass commented Aug 7, 2021

Can you make some examples of what does not work because import { ThemeProvider } from '@material-ui/core/styles'; and import { ThemeProvider } from '@material-ui/styles'; are working differently:

  1. import { ThemeProvider } from '@material-ui/core/styles applies emotion styling + jss styling
  2. import { ThemeProvider } from '@material-ui/styles'; jss styling

Can you share an example of a wrongly processed theme or do you have nested theming in application?

@stevemk14ebr
Copy link
Author

Sure here is a simple example: https://codesandbox.io/s/theming-material-demo-forked-fnhvx?file=/demo.js

The issue also applies to many other components whos type depends on a theme, as well as the darkscrollBar helper.

@povilass
Copy link
Contributor

povilass commented Aug 8, 2021

It works as expected behaviour. Look you got context from import { ThemeProvider } from '@material-ui/styles'; comes from import { ThemeProvider} from '@material-ui/private-theming'; This provider job is to merge outer theme with inner theme properties if you want to override some styles from your root theme and pass that theme to consumer code.

But material UI in v5 under the hood uses emotion as its styling engine. If you look at import { ThemeProvider } from '@material-ui/core/styles code you will see that this theme provider also uses this import { ThemeProvider as MuiThemeProvider } from '@material-ui/private-theming'; and passes a theme correctly but you also need to give theme object to emotion style engine. All buttons and other stuff are connected to emotion style engine that's why this is not working for you.

In my opinion import { ThemeProvider} from '@material-ui/private-theming'; is more like ThemeMergerAndProvider but its purpose now probably to be as internal API not public. If later material UI will give us have more unstyled components and you don't want to rely on emotion and write your own style components using a different engine this provider is for you as theming API.

@oliviertassinari oliviertassinari added status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 8, 2021
@oliviertassinari
Copy link
Member

@stevemk14ebr Were you using v4 before, migrating from it to v5? I'm asking to understand why you are using @material-ui/styles.

@stevemk14ebr
Copy link
Author

stevemk14ebr commented Aug 8, 2021

I used @material-ui/styles because there are examples here https://next.material-ui.com/styles/api/#heading-themeprovider that use that import style.
image

From this documentation the difference between the two imports doesn't seem to be mentioned at all. As a user I found the difference in behavior extremely confusing. Under no circumstance would I expect that a button who is inside of a theme provider, would not use that theme! This is an example of what I mean:
https://codesandbox.io/s/theming-material-demo-forked-fnhvx?file=/demo.js

There should be no difference between these two in my opinion:

 <ThemeProvider theme={theme}>
        <Button>Test Wrong</Button>
      </ThemeProvider>
      <ThemeProvider theme={theme}>
        <Button theme={theme}>WHAT?? i thought theme provider did this!</Button>
      </ThemeProvider>

The documentation states that theme provider provides a theme to sub-elements, if it does not do that, as this examples show, then I would kindly suggest it be renamed to avoid user confusion like this!

@stevemk14ebr
Copy link
Author

stevemk14ebr commented Aug 8, 2021

from: https://next.material-ui.com/styles/api/#heading-themeprovider
This example is a timebomb for user confusion:
image

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 8, 2021

@stevemk14ebr Ok great we are good then, for now (until we get more feedback). In #27639 we make it clearer

Capture d’écran 2021-08-09 à 01 30 52

It's also an argument in favor of having a separate docs environment for different parts of the project. To emphasize that this doc, includes multiple different products.

@mnajdova When should we leverage https://docs.npmjs.com/deprecating-and-undeprecating-packages-or-package-versions#deprecating-an-entire-package?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it v5.x
Projects
None yet
Development

No branches or pull requests

3 participants