[system] Add createTheme util #26490
Conversation
|
Details of bundle changes (experimental) @material-ui/system: parsed: +2.70% , gzip: +2.25% |
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
| @@ -82,69 +79,6 @@ export const sizeHeight: SimpleStyleFunction<'sizeHeight'>; | |||
| export const boxSizing: SimpleStyleFunction<'boxSizing'>; | |||
| export type SizingProps = PropsFor<typeof sizing>; | |||
|
|
|||
| // spacing.js | |||
There was a problem hiding this comment.
Moved to spacing.d.ts and style.d.ts
There was a problem hiding this comment.
Oh yes, we need to break down this file 😅, or even jump one step and migrate the modules to TS.
| const Div = styled('div')` | ||
| width: ${(props) => `${props.theme.spacing}px`}; | ||
| width: ${(props) => props.theme.spacing(1)}; |
There was a problem hiding this comment.
Support the original props.theme.spacing(1)
|
|
||
| export type Direction = 'ltr' | 'rtl'; | ||
|
|
||
| export interface ThemeOptions { |
There was a problem hiding this comment.
Part of these types are already included in the @material-ui/system ThemeOptions & Theme. We are now extending those.
mnajdova
requested review from
eps1lon,
oliviertassinari,
michaldudak and
siriwatknp
| @@ -0,0 +1,44 @@ | |||
| import { deepmerge } from '@material-ui/utils'; | |||
| import createBreakpoints from './createBreakpoints'; | |||
There was a problem hiding this comment.
All these are moved from the @material-ui/core/styles.
| @@ -82,69 +79,6 @@ export const sizeHeight: SimpleStyleFunction<'sizeHeight'>; | |||
| export const boxSizing: SimpleStyleFunction<'boxSizing'>; | |||
| export type SizingProps = PropsFor<typeof sizing>; | |||
|
|
|||
| // spacing.js | |||
There was a problem hiding this comment.
Oh yes, we need to break down this file 😅, or even jump one step and migrate the modules to TS.
| typography: createTypography(palette, typographyInput), | ||
| spacing, | ||
| shape: { ...shape }, | ||
| transitions: createTransitions(transitionsInput), |
There was a problem hiding this comment.
Not sure about the default value. Aren't the transition values specific to Material Design? Same for the shadows, border radius, mixins, spacing, breakpoints.
There was a problem hiding this comment.
I'm not sure that any of them should be in the system. But it's not all black and white; my attempt at ranking by order of what MD is the most unique about and the system should likely no include:
- mixins very specific to the AppBar
- shadows very specific to MD
- border-radius, specific to MD
- transition, specific to MD
- breakpoints, somewhat generic
- spacing, somewhat generic (TW uses 0.25rem, we use 8px, BS uses 0.5rem)
There was a problem hiding this comment.
@material-ui/system: parsed: +7.94% , gzip: +7.85%
To be careful because the larger the system is, the less likely it will be used by third parties. What we currently have: https://bundlephobia.com/result?p=@material-ui/[email protected].
There was a problem hiding this comment.
To be careful because the larger the system is, the less likely it will be used by third parties.
It is still not viable to cater to these people. Tree-shaking is a thing. If you want to live in the past with old technology then stay there. We're moving forwards where overall package size is mostly irrelevant.
There was a problem hiding this comment.
My opinion on the matter:
mixins very specific to the AppBar- agree, will remove it
shadows very specific to MD- agree, will remove it
border-radius, specific to MD - I will think it's good default, would rather leave it, other can override
transition, specific to MD- agree, will remove it
breakpoints, somewhat generic - agree would leave it
spacing, somewhat generic (TW uses 0.25rem, we use 8px, BS uses 0.5rem) - agree, would leave it
Regarding the typings, I would leave all values in the ThemeOptions and as optional in the Theme, and in the core we can make the typings stricter, basically as they are in the moment. What do you think?
Regarding the bundle size, we are extending this package to be the styling package for building a custom design system, so I don't think we can compare with the previous implementation, but agree that we should be careful around not adding too many things which are not generic enough.
There was a problem hiding this comment.
@mnajdova Sounds fair.
Tree-shaking
@eps1lon In the current approach, there is no tree shaking possible when importing Box or styled, we force the default theme. AFAIK, these two are meant to be the most popular modules used from @material-ui/system.
Do you mean that you would make the default version of the helper with an empty theme and allow developers to cherry-pick which baseline theme they want? It could be a thing
If you want to live in the past with old technology then stay there.
I assume "you" is targeted at the audience that is excessively worried about bundle size. The number of developers that were complaining about bundle size was relatively low in our last survey, maybe this audience is marginal. Assuming it's not a filtering bias, it supports the idea that we can take some liberties with bundle size and not care too much. For instance, chakra having all the styles of the components inside a non-tree-shakeable theme.
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
The
createThemeas part of the system has all defaults as the original @material-ui/core'screateTheme, except the values inside thepalette,typography,zIndex.The
createThemein the core now uses the one from the system.