mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.64k stars 32.22k forks source link

[Docs] No mention of having to wrap the components with ThemeProvider #15528

Closed janhesters closed 4 years ago

janhesters commented 5 years ago

I upgraded my project from Material UI V3 to V4 (Beta).

After the upgrade I changed my withStyles imports from:

import withStyles from '@material-ui/core/styles/withStyles';

to

import withStyles from '@material-ui/styles/withStyles';

Now I get the error message:

index.js:1446 Warning: Material-UI: the `styles` argument provided is invalid.
You are providing a function without a theme in the context.
One of the parent elements needs to use a ThemeProvider.

Do you always have to use a ThemeProvider as of Material-UI 4? If yes, I think this should be mentioned in the docs somewhere. Before I think the was a default value supplied. At least my styles would work without having a ThemeProvider.

eps1lon commented 5 years ago

Do you always have to use a ThemeProvider as of Material-UI 4?

For import withStyles from '@material-ui/styles/withStyles';, yes. For import withStyles from '@material-ui/core/styles/withStyles';, no.

It's the first segment in https://next.material-ui.com/css-in-js/advanced/. Maybe we can emphasize more that the two imports use different defaults but I doubt it will help much.

PS: Prefer import { withStyles } from '@material-ui/core/styles'; . The path import has limited tree-shaking support.

janhesters commented 5 years ago

@eps1lon Aaah, so when using V4 I don't even need @material-ui/styles? I can keep using @material-ui/core/styles?

janhesters commented 5 years ago

@eps1lon Also another question about styling if you are so kind to answer:

Now, that I used your fixes, I get the error:

Warning: Material-UI: theme.spacing.unit usage has been deprecated.
It will be removed in v5.
You can replace `theme.spacing.unit * y` with `theme.spacing(y)`.

How are you supposed to fix theme.spacing.unit? With no args: theme.spacing()?

numToStr commented 5 years ago

I also faced this issue. With no args theme.spacing() just returns a empty string 🤔. I think it should returns the default value.

How are you supposed to fix theme.spacing.unit? With no args: theme.spacing()?

To get this fix, I passed 1 in the args i.e. theme.spacing(1).

janhesters commented 5 years ago

@vkasraj Nice thanks. Maybe we can include this in the docs somewhere?

numToStr commented 5 years ago

@janhesters I think without no args should give the default value. I created this issue #15530 for reference.

nareshbhatia commented 5 years ago

I am getting this error only when I am running tests. Don't remember seeing it before. Trying to track down.

I import like this from everywhere in the app:

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

and I have my own ThemeProvider.

Could it be that tests are not getting a ThemeProvider?

nareshbhatia commented 5 years ago

Ok, figured it out. The issue with tests is that they are not getting a theme - this is because the components are importing from @material-ui/styles. Now I have to explicitly wrap all the components under test with a ThemeProvider. I can write a wrapper for this, but is there a better solution?

import React from 'react';
import ReactDOM from 'react-dom';
import { App } from './App';
import CssBaseline from '@material-ui/core/CssBaseline';
import { ThemeProvider } from '@material-ui/styles';
import { theme } from './theme';

it('renders without crashing', () => {
    const div = document.createElement('div');
    ReactDOM.render(
        <ThemeProvider theme={theme}>
            <CssBaseline />
            <App />
        </ThemeProvider>,
        div
    );
    ReactDOM.unmountComponentAtNode(div);
});
nareshbhatia commented 5 years ago

@eps1lon, I didn't quite understand your recommendation:

PS: Prefer import { withStyles } from '@material-ui/core/styles'; . The path import has limited tree-shaking support.

Can you please elaborate? In the docs (Styles/Basic and Styles/Advanced), all the examples show import { makeStyles } from '@material-ui/styles'. Also there is no TypeScript example for makeStyles (either here or in the TypeScript section). I guess I am not understanding the dependencies conceptually. Is @material-ui/styles low-level stuff and generally we should not bother with it? Is @material-ui/core/styles a higher-level package that should be used generally?

Also related question: if @material-ui/styles requires a ThemeProvider somewhere as an ancestor, tests would have to wrap the component under test with a ThemeProvider, correct? This is a pain. What is the recommended way to test components that need a theme?

Sorry for all these questions, but I suspect many people are in the same boat. Please help us understand the concepts and intentions behind this design.

eps1lon commented 5 years ago

all the examples show import { makeStyles } from '@material-ui/styles'

That's fine. I was referring to the initial post that used import withStyles from '@material-ui/core/styles/withStyles';. We don't recommend this pattern and if you find examples in our docs using path imports deeper than the first level please open a pull request

import {} from '@material-ui/core'; // root level, fine
import {} from '@material-ui/core/styles'; // 1st level, fine
import {} from '@material-ui/core/styles/withStyles'; // > 1st level, not fine

This is a pain.

That is how context works though. There's nothing much we can do about this. Any other styling solution with custom theming support will require this. React doesn't offer a feature (yet) to improve the situation.

It also indicates that something else is wrong with your tests. There should be little to no friction adding another provider to your tests. You could always write a little render helper that takes a component you want to tests and renders them inside some mocked providers.

nareshbhatia commented 5 years ago

Thanks for the clarifications @eps1lon and the guidance on render helpers. Will give it a shot.

eps1lon commented 5 years ago

Thanks for the clarifications @eps1lon and the guidance on render helpers. Will give it a shot.

We had the same pain over the fast few months. We were relying on a specific component tree structure and every time we added or removed some hoc or react wrapper our tests broke even if the observable behavior didn't change.

jeremy-coleman commented 5 years ago

I raged over this. Imo very poor design decision

eps1lon commented 5 years ago

I raged over this. Imo very poor design decision

Do you have an alternative proposal? I'm not familiar with other styling solutions. How do they solve theming without context? React context seems to be best way to have some global state currently. Once the Context.write proposal is implemented in the react core you can change the default value as you please.

jeremy-coleman commented 5 years ago

@eps1lon sorry, i was referring to the mui/styles - mui/core/styles mismatch breaking things, not the usage of react context as a theme value provider.

Although, i do think there are better alternatives / improvements to be had, here are a couple thoughts that probably dont belong in this thread:\

option 1

the first and best option is css variables. this allows you to statically define a theme object and simply include ~15-20 variables in the head or write using js (which can be derived from createMuiTheme), and which can also be themed using setProperty on any element. no need for !important or anything else. For example, mui could just write the vars to the head, and any consumer could call some function that redefines 1 to all of the variables, multiple times without recreating ANY styles.

so at design time, it looks something like

//mui.js 
var theme = {
 primary: 'var(--mui-primary)'
}

//some_component.js
import {theme} from mui
import {cssFunction} from 'jss-thing'

myStyle = cssFunction({backgroundColor: theme.primary})
}

MyComponent = () => <div className={myStyle}>

and in your head you can just include a

or using document.querySelector(#whatever).style.setProperty('--mui-theme-primary', 'orange')

this behaves exactly the same as a react context theme provider with infinite nesting possibilities and a fraction of the overhead. also as i mentioned above, does not require any work by jss OR react.

this actually requires no changes to the current createMuiTheme , i do this in my current project and apply office ui fabric's createTheme() and createMuiTheme() to a set of common vars , which lets me match the theme between the two libs , use the theme in my own component creation , and also use outside of react.

option 2

a typical style defining function tends to be of the shape

style = theme => css({backgroundColor: theme.primary})

there are a few other options besides context when using the above traditional callback theming

in general i think some thought needs to be given to what happens when you call a style function, what scope the object lives in, what the browser will react to and how to notify the css function to run again if the theme object changes. - the simplest way would be to reflect property access on the theme and link that to the css function inside the jss instance, probably 5 lines of code.

another thing to consider/optimize for is to ask, do the classs names change? if not - react doesn't need to re-render updated className to the dom and therefore doesn't need to rerender anything at all or even be aware of theme changes. and stylesheet lifecycle could be handled through a simple emitter. the browser will update to changes in css. you can call a function from inside a function that's primary purpose is to create a react element, but that doesn't mean that it needs to be associated with react in anyway, but in that case you should probably hoist it anyway for instance say we have


theme = {
 primary: 'red'
}
ButtonStylesheet = theme => css({
root: {
backgroundColor: theme.primary
}
})

var Button = props => {
buttonClasses = ButtonStylesheet(props.theme) <-- important bit
return <button className={buttonClasses .root}/>
}

which creates

<button class='MuiButton-1'/>

and jss will write the equivalent css

MuiButton-1:{
background-color:red;
}
what happens if we do theme.primary = 'blue' ?

jss may write
```css
MuiButton-1:{
background-color:blue;
}

and now the button is blue

OR with coupling (order matters here ) case1) theme object -> change detection -> jss writes -> react reconciles -> react rerenders case2) theme object -> change detection -> react updates theme context -> jss reconciles (via cache/hash) and jss rewrites css (this is worst case because its not going to be batched)

for case 1- jss writes (plugin dependent of course)

MuiButton-1:{
background-color:red;
}
MuiButton-2:{
background-color:blue;
}

or possibly (anything except the original, even if only 1 copy in the dom)

MuiButton-2:{
background-color:blue;
}

and now the button is still red because react hasn't written updated the class in the dom.

so we need to run the component again. props changed so lets do a === check across the entire mui theme and then lets reconcile and re-render stuff via the Button component function which writes

Zache commented 5 years ago

I avoid using the ThemeProvider in my tests by providing a defaultTheme to makeStyles

mayteio commented 5 years ago

@Zache can you please provide an example?

Zache commented 5 years ago

I'm on vacation so I don't have computer access, but the makeStyles function takes an object with options. One of the options is defaultTheme, which I assign the same theme that the ThemeProvider gets (imported from some module)

I hope this makes sense

On Thu, 1 Aug 2019, 12:42 Harley Alexander, notifications@github.com wrote:

@Zache https://github.com/Zache can you please provide an example?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mui-org/material-ui/issues/15528?email_source=notifications&email_token=AA3AXQBWIDFEKW7TPH35443QCK4ZDA5CNFSM4HJJFZFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3KEZTQ#issuecomment-517229774, or mute the thread https://github.com/notifications/unsubscribe-auth/AA3AXQBQGXF6ABQ52VDAPHTQCK4ZDANCNFSM4HJJFZFA .

oliviertassinari commented 4 years ago

I think that we can close now, we have updated the documentation to reference the import of the styles from the core package as preferred (at least, if you plan to use our components, not just the styles).