[typescript] [createMuiTheme] Fix typings & deepmerge shape #11993
Conversation
| transitions?: TransitionsOptions; | ||
| typography?: TypographyOptions | ((palette: Palette) => TypographyOptions); | ||
| zIndex?: ZIndexOptions; | ||
| status?: Record<string, string>; |
There was a problem hiding this comment.
No, it's too specific.
|
|
||
| export interface ThemeOptions { | ||
| shape?: ShapeOptions; | ||
| shape?: Shape; |
There was a problem hiding this comment.
Don't we still need the partial? Like we do for the Spacing.
There was a problem hiding this comment.
Unlike spacing, which is deepmerged, shape is not.
https://github.com/mui-org/material-ui/blob/ca33d9773581a8b0ff37c92260b48c9f72b42de9/packages/material-ui/src/styles/createMuiTheme.js#L38-L44
This means that for spacing, we can specify just partial of it, but for shadow, we need to specify the full props, from what I understand.
There was a problem hiding this comment.
FYI - if you take a look at shadow type definition under the ThemeOptions interface, it is not Partial<> either.
https://github.com/mui-org/material-ui/blob/ca33d9773581a8b0ff37c92260b48c9f72b42de9/packages/material-ui/src/styles/createMuiTheme.d.ts#L23
There was a problem hiding this comment.
Thanks for raising the issue! Should we fix the implementation over the TypeScript definition then? I have missed this part. I think that spacing and shape should behave the same way.
There was a problem hiding this comment.
I see. I'll do this. BTW, what about shadows? Should it also be a partial in the ThemeOptions?
There was a problem hiding this comment.
we need to specify the full props
I don't think that it's the behavior people expect. We need to fix the shape key :).
There was a problem hiding this comment.
NVM. shadows is different.
There was a problem hiding this comment.
Regarding the shadows, yeah it's fine. Relative values are important, people need to provide the full property.
| getContrastText?: (background: string) => string; | ||
| } | ||
|
|
||
| //export type PaletteOptions = DeepPartial<Palette>; |
There was a problem hiding this comment.
This was commented out. Why do we want to preserve the commented out code?
There was a problem hiding this comment.
I was wondering why it was commented out; Let's remove it then :)
| transitions: Transitions; | ||
| typography: Typography; | ||
| zIndex: ZIndex; | ||
| status?: Record<string, string>; |
There was a problem hiding this comment.
No, it's too specific.
franklixuefei
changed the title
|
@franklixuefei So many bug fixes in a single pull request 🎉! |
|
@oliviertassinari Thanks for taking care of this! I actually left out those two props |
* fix typings * fix * revert * fix shape * fix unit test * add unit test
2 participants
fixes #11853