[system] Remove theme/styling allocations #43372
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import clsx from 'clsx'; | |
| import chainPropTypes from '@mui/utils/chainPropTypes'; | ||
| import composeClasses from '@mui/utils/composeClasses'; | ||
| import { styled } from '../zero-styled'; | ||
| import memoTheme from '../utils/memoTheme'; | ||
| import { useDefaultProps } from '../DefaultPropsProvider'; | ||
| import Collapse from '../Collapse'; | ||
| import Paper from '../Paper'; | ||
|
|
@@ -46,7 +47,7 @@ const AccordionRoot = styled(Paper, { | |
| ]; | ||
| }, | ||
| })( | ||
| memoTheme(({ theme }) => { | ||
| const transition = { | ||
| duration: theme.transitions.duration.shortest, | ||
|
Comment on lines
47
to
52
There was a problem hiding this comment. Are we sure about the DX of this? As a developer looking at the source:https://github.com/romgrk/material-ui/blob/8921455731c32f13dd9d61fa49002feb21b673fa/packages/mui-material/src/Accordion/Accordion.js#L36, the first thing I ask myself: why is the source verbose like this? Couldn't this be a flag? const AccordionRoot = styled(Paper, {
name: 'MuiAccordion',
slot: 'Root',
+ onlyTheme: true,
overridesResolver: (props, styles) => {
const { ownerState } = props;
return [
{ [`& .${accordionClasses.region}`]: styles.region },
styles.root,
!ownerState.square && styles.rounded,
!ownerState.disableGutters && styles.gutters,
];
},
})(
({ theme }) => {If we had a transpilation step, it could automatically detect those and add the flag 😄, this way, nobody will forget about it, or use it wrong. There was a problem hiding this comment. It could have been a flag, I didn't think about it, but I didn't think about it too much because it can be refactored anytime and I want to iterate on this further. Emotion is serializing+hashing those style objects on every render of every component, even if we return the same styling object. For internal components we can know, either with the flag on the styled option or a flag on the The only thing I need to figure out is variants, we probably need to transfer them from our style value to emotion's serialized style object. One thing though, the flag would need to affect typings, I hope it's not too complex to setup. There was a problem hiding this comment. I remember why I picked a function though, initially I also had I didn't end up including There was a problem hiding this comment.
Oh, nice, like it used to be in Material UI v4 with JSS. Styles created once per component type.
Maybe we can create custom class name for each variant and apply them conditionally, like Pigment CSS is doing or like we were doing in Material UI v4. The difference now is that we have CSS variables, we have fewer variants to create, e.g. only one for color and not one for color primary, color secondary, etc. There was a problem hiding this comment. Follow-up here: #43412 |
||
| }; | ||
|
|
@@ -91,8 +92,8 @@ const AccordionRoot = styled(Paper, { | |
| backgroundColor: (theme.vars || theme).palette.action.disabledBackground, | ||
| }, | ||
| }; | ||
| }), | ||
| memoTheme(({ theme }) => ({ | ||
| variants: [ | ||
| { | ||
| props: (props) => !props.square, | ||
|
|
@@ -122,7 +123,7 @@ const AccordionRoot = styled(Paper, { | |
| }, | ||
| }, | ||
| ], | ||
| })), | ||
| ); | ||
|
|
||
| const AccordionHeading = styled('h3', { | ||
|
|
||
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.
There's a problem with prettier, it's going to forcefully increase the indentation level everywhere inside
memoTheme(({ theme }) => /* here */), which is one of the reasons I dislike prettier. Anyway, I've left the PR without prettifying it to ease the review, but I'll run it as a last step before merging.