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

Documentation showing '@material-ui/styles' rather than '@material-ui/core/styles' #15867

Closed
liywjl opened this issue May 26, 2019 · 27 comments · Fixed by #17447
Closed

Documentation showing '@material-ui/styles' rather than '@material-ui/core/styles' #15867

liywjl opened this issue May 26, 2019 · 27 comments · Fixed by #17447
Labels
docs Improvements or additions to the documentation priority: important This change can make a difference

Comments

@liywjl
Copy link

liywjl commented May 26, 2019

Documentation showing '@material-ui/styles' rather than '@material-ui/core/styles'

(notably in the theming section)

@eps1lon eps1lon added discussion docs Improvements or additions to the documentation labels May 26, 2019
@eps1lon
Copy link
Member

eps1lon commented May 26, 2019

Could you point to specific sections and explain why they should use core/styles instead?

@liywjl
Copy link
Author

liywjl commented May 26, 2019

https://material-ui.com/customization/themes/

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

after some deeper digging it appears that i needed to install https://www.npmjs.com/package/@material-ui/styles

this should be mentioned in the documentation regarding theming (although i did see ThemeProvider being exported by @material-ui/core so I may be wrong)

@eps1lon
Copy link
Member

eps1lon commented May 26, 2019

material-ui.com/customization/themes

Yes, that section should import MuiThemeProvider from core/styles. Would help a lot if you could file a PR with those change.

@eps1lon eps1lon added good first issue Great for first contributions. Enable to learn the contribution process. and removed discussion labels May 26, 2019
@mesvil7
Copy link

mesvil7 commented Jun 7, 2019

When use makeStyles from @material-ui/styles, I got. Cannot read property 'common' of undefined error. Also I get the error if I use createStyles like the page example

import { Theme } from '@material-ui/core'
import { makeStyles } from '@material-ui/styles'
const useStyles = makeStyles((theme: Theme) => ({
  root: {
    color: theme.palette.common.black,
  }
})) 

But when use makeStyles from '@material-ui/core/styles' the code above work

@mesvil7
Copy link

mesvil7 commented Jun 10, 2019

Also I'm using import { StylesProvider, createGenerateClassName } from '@material-ui/styles' folowing you documentation here https://material-ui.com/styles/api/#creategenerateclassname-options-class-name-generator and this is triggering the next error: It looks like there are several instances of @material-ui/styles initialized in this application.
This may cause theme propagation issues, broken class names and makes your application bigger without a good reason.
When run npm ls @material-ui/styles get
@material-ui/core@4.0.1 │ └── @material-ui/styles@4.0.2 └── @material-ui/styles@4.0.0-rc.0
But it seems there some functionality like StylesProvider, createGenerateClassName not present in the core/styles

@mnbroatch
Copy link

I am also confused at the difference between /core/styles and /styles . The former has some-but-not-all styles functionality?

@bigtone1284
Copy link
Contributor

@liywjl @eps1lon I'd like to help here if I can.

So context is that the current docs does not have the correct import path for functions like makeStyles and ThemeProvider? The documentation has them as being imported from '@material-ui/styles' when they should be imported from '@material-ui/core/styles'?

@joshwooding
Copy link
Member

@mnbroatch Does this help? https://material-ui.com/customization/default-theme/#material-ui-core-styles-vs-material-ui-styles

@bigtone1284 The docs has a section about @material-ui/styles where it’s used exclusively. All component demos should be using @material-ui/core/styles

@mnbroatch
Copy link

mnbroatch commented Jun 26, 2019

@joshwooding Not really...

Given that wording ("@material-ui/core/styles/makeStyles wraps @material-ui/styles/makeStyles") I would expect that if I wanted to use makeStyles I could import { makeStyles } from '@material-ui/core/styles' just as easily as using @material-ui/styles. But I'm on what as far as I can tell is the latest 3.x.x (3.9.3), and I see makeStyles in the 3.x docs, but that import is undefined and in fact a grep in node_modules/@material-ui shows me that makeStyles only shows up in the changelog. WithStyles on the other hand is there and we're using import { withStyles } from '@material-ui/core/styles' without issue.

@bigtone1284
Copy link
Contributor

@mnbroatch so you feel as though based on the wording of the docs, that one should be able import makeStyles and similar from @material-ui/styles, not from @material-ui/core/styles?

@joshwooding I think you are explaining that these methods are to be imported and used after they are wrapped with the default theme at the /core level?

@mnbroatch
Copy link

There's nothing (in that section of the docs at least) telling me that withStyles and makeStyles should behave differently in that regard, or that there would be behavior differences between the wrapped and unwrapped versions of those functions. The usage of the word "wrap" is pretty vague, so one really can't predict the behavior.

@joshwooding
Copy link
Member

joshwooding commented Jun 27, 2019

The docs currently reflect the v4 code, relying on them for v3 is not recommended. v3 docs

There was a fairly major style change in v4 where @material-ui/styles is now used behind the scenes hence makeStyles is now exported in @material-ui/core whilst it wasn't in v3. In v3 you need to use @material-ui/styles alongside @material-ui/core to get the newer functionality, v4 provides it out of the box.

I agree we can do better with the descriptions, I'll suggest a change.

@mnbroatch
Copy link

ok, yea now that I understand a little better, I think https://v3.material-ui.com/css-in-js/basics/ is OK

@bigtone1284
Copy link
Contributor

Cool. So the current docs/examples can be changed to import withStyles, etc from @material-ui/core/styles? and @joshwooding you will suggest a change for the descriptions in the docs around how the functions are wrapped and exported?

@joshwooding
Copy link
Member

@bigtone1284 The styles section is supposed to be using @material-ui/styles.

@bigtone1284
Copy link
Contributor

@joshwooding so styles section should import from @material-ui/styles while components section (demos) should import from @material-ui/core/styles?

@joshwooding
Copy link
Member

@bigtone1284 correct :)

@mesvil7
Copy link

mesvil7 commented Jun 28, 2019

Just to be sure. are You planning to move all the styles components within core/styles?

@bigtone1284
Copy link
Contributor

@joshwooding I am going to make the edits for this.

@bigtone1284
Copy link
Contributor

@mesvil7 mesvil7

I think moving styles components to within core/styles is not in the scope of this issue and might be a different conversation.

@joshwooding I can make the change related to updating the import examples in the docs as we discussed above.

@bigtone1284
Copy link
Contributor

bigtone1284 commented Jun 28, 2019

Hey, I started looking at this issue. I think the original solution of @eps1lon to MuiThemeProvider from @material-ui/core/styles might not be correct. I tried making this change in the codesandbox associated with the page that @liywjl and @eps1lon initial started talking about, and trying to import MuiThemeProvider from @material-ui/core/styles did not work.

tl;dr, I don't think the docs are incorrect. Maybe they need to say that users need to import @material-ui/stylesto useMuiThemeProvider`, but I don't think the existing import statements are incorrect.

@eps1lon @joshwooding Any thoughts on this? Unless I am missing something, I don't think the docs should be changed regarding importing from '@material-ui/styles' rather than '@material-ui/core/styles'.

@joshwooding
Copy link
Member

joshwooding commented Jun 29, 2019

@bigtone1284 I think the only thing that might need a change is the difference section just so the wording is improved.

@bigtone1284
Copy link
Contributor

@joshwooding were you going to make that change? I can give it a try but you might have more familiarity with this issue. I'd be happy to provide some eyes on the review if it helps?

@joshwooding
Copy link
Member

@bigtone1284 I was but I’m on holiday right now, if you or someone else wants to re-word it slightly I’ll be happy to take a a look otherwise I’ll make in change in around a weeks time.

@mesvil7
Copy link

mesvil7 commented Jun 30, 2019

Hey, I started looking at this issue. I think the original solution of @eps1lon to MuiThemeProvider from @material-ui/core/styles might not be correct. I tried making this change in the codesandbox associated with the page that @liywjl and @eps1lon initial started talking about, and trying to import MuiThemeProvider from @material-ui/core/styles did not work.

tl;dr, I don't think the docs are incorrect. Maybe they need to say that users need to import @material-ui/stylesto useMuiThemeProvider`, but I don't think the existing import statements are incorrect.

@eps1lon @joshwooding Any thoughts on this? Unless I am missing something, I don't think the docs should be changed regarding importing from '@material-ui/styles' rather than '@material-ui/core/styles'.

Import MuiThemeProvider from '@material-ui/core/styles' work fine since this component is importing ThemeProvider from '@material-ui/styles'. (@material-ui/core/styles/MuiThemeProvider.d.ts)
@material-ui/styles has no exported member 'MuiThemeProvider' it has ThemeProvider.

I think the confusion is you have components doing the same things in core/styles and styles in the root

@bigtone1284
Copy link
Contributor

@joshwooding @mesvil7 @mnbroatch

I opened a PR on this and appreciate any input on this. I gave a first effort at adding some language to help clarify. Thanks! I can probably address any concerns early next week.

@levrik
Copy link

levrik commented Jul 17, 2019

Wow... my editor auto imported from @material-ui/styles for one component and it took me quite some time to figure out why it was asking for a ThemeProvider now.
Can the error message please be updated pointing out that you could import the wrong package?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation priority: important This change can make a difference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants