-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[core] Remove createStyles
from @material-ui/core
#26018
Conversation
On second thought, let's not go that way. Too many possible opinionated discussions. Let's keep it as |
Just to make sure I understand, are you saying I should add it back to |
|
||
// let warnOnce = false; | ||
|
||
// To remove in v5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to v6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a deprecation notice now? It was mentioned that it's only needed for notistack. notistack could clone this helper.
Also, with #1824 and #18098 we will likely roll out our own solution to the problem, so this demo https://next.material-ui.com/components/snackbars/#notistack is likely on its way out, in the long term.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added back warning
I understood this as: it's too early to remove the module from the codebase, moving it from the core to styles should be enough. |
Alright, but I cannot remove it from core completely because of the failing issue with |
Yes. We'll just keep it at runtime to not cause crashes needlessly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the docs demos don't use createStyles
we can drop the babel plugin as well. That was only used for internal usage.
@@ -179,7 +179,8 @@ interface Props { | |||
However this isn't very [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) because it requires you to maintain the class names (`'root'`, `'paper'`, `'button'`, ...) in two different places. We provide a type operator `WithStyles` to help with this, so that you can just write: | |||
|
|||
```ts | |||
import { WithStyles, createStyles } from '@material-ui/core'; | |||
import { createStyles } from '@material-ui/styles'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the usage here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like if we remove this we should update the whole page, as it actually talks about how to solve typescript issues for withStyles
. Should we recommend casting to const
there too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's do this another time.
@eps1lon how should we fix the type errors? There are issues on each file I touched. There are types issues on |
Where removal of -const styles = createStyles({});
+const styles = {} as const; |
Removed. |
|
||
// let warnOnce = false; | ||
|
||
// To remove in v5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a deprecation notice now? It was mentioned that it's only needed for notistack. notistack could clone this helper.
Also, with #1824 and #18098 we will likely roll out our own solution to the problem, so this demo https://next.material-ui.com/components/snackbars/#notistack is likely on its way out, in the long term.
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+291 −716
😍
I have updated the label as it seems to only be a depreciation now (there are no breaking changes). |
Deprecations
createStyles
function from@material-ui/core/styles
was moved to@material-ui/styles
. This helps use remove JSS from the core package.One of the PRs that is necessary for removing
@material-ui/styles
as a dependency of@material-ui/core
.