-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
Comments
Could you point to specific sections and explain why they should use |
https://material-ui.com/customization/themes/ all examples using 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) |
Yes, that section should import MuiThemeProvider from core/styles. Would help a lot if you could file a PR with those change. |
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
But when use makeStyles from '@material-ui/core/styles' the code above work |
Also I'm using |
I am also confused at the difference between /core/styles and /styles . The former has some-but-not-all styles functionality? |
@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 |
@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 |
@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 |
@mnbroatch so you feel as though based on the wording of the docs, that one should be able import @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 |
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. |
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 I agree we can do better with the descriptions, I'll suggest a change. |
ok, yea now that I understand a little better, I think https://v3.material-ui.com/css-in-js/basics/ is OK |
Cool. So the current docs/examples can be changed to import |
@bigtone1284 The styles section is supposed to be using |
@joshwooding so styles section should import from |
@bigtone1284 correct :) |
Just to be sure. are You planning to move all the styles components within core/styles? |
@joshwooding I am going to make the edits for this. |
@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. |
Hey, I started looking at this issue. I think the original solution of @eps1lon to tl;dr, I don't think the docs are incorrect. Maybe they need to say that users need to import @material-ui/styles @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'. |
@bigtone1284 I think the only thing that might need a change is the difference section just so the wording is improved. |
@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? |
@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. |
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) I think the confusion is you have components doing the same things in core/styles and styles in the root |
@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. |
Wow... my editor auto imported from |
Documentation showing '@material-ui/styles' rather than '@material-ui/core/styles'
(notably in the theming section)
The text was updated successfully, but these errors were encountered: