mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.75k stars 32.24k forks source link

Custom components can no longer be styled the same way as Paper #27980

Open dantman opened 3 years ago

dantman commented 3 years ago

Summary 💡

In MUIv4 the styles that <Paper> used were relatively simple and custom components that didn't need the complex (square, outlined, animated multi-elevation) behaviours could easily match the same Material surface style simply by using a few variables from the MUI theme. e.g.

const CustomSurface = styled('div')(({ theme }) => ({
  color: theme.palette.text.primary,
  backgroundColor: theme.palette.background.paper,
  boxShadow: theme.shadows[2],
  borderRadius: theme.shape.borderRadius,
}), []);

However with the introduction of Material's changes to dark mode surface styles (#18309), the way it was implemented means it's no longer possible for custom surfaces to use the standard surface background color without inheriting from <Paper> and moving elevation handling from styles in the custom surface to props passed from the owner component.

This is because the new dark mode background was implemented using a linear-gradient hardcoded in <Paper> that uses a complex getOverlayAlpha function which is hardcoded into Paper.js. As a result any custom component wanting to match <Paper>'s background must now copy the getOverlayAlpha and linear-gradient source code from Paper.js in order to use the correct background in their own component.

https://github.com/mui-org/material-ui/blob/7e7f40fff30ab0c2ec7a0003055a6508e11bcbb7/packages/material-ui/src/Paper/Paper.js#L61-L69

https://github.com/mui-org/material-ui/blob/7e7f40fff30ab0c2ec7a0003055a6508e11bcbb7/packages/material-ui/src/Paper/Paper.js#L13-L21

Expectation

It should be possible to get a Material surface style background in a custom component using the Theme alone. Perhaps as a mixin. e.g. { ...theme.mixin.paperBackground(elevation) }.

Motivation 🔦

There are a lot of reasons for custom components to not want to inherit from Paper. The elevation of custom components is typically fixed and part of the component's intrinsic styles (e.g. how Button hardcodes its elevations) even if it is a paper-like surface that has the same background color behaviour. styled does not allow overriding props with a huge mess. It's even more complex if elevation is actually based on something dynamic like :hover. And the TypeScript types also end up more complex than they need to be for a component not using all of Paper's features.

I actually had to implement a modification to Accordion where it behaves like the Material demo where 2 surfaces join together into 1 surface. This was only possible using a :before which needed to use the same background as the <Paper> that Accordion is based on. This worked seamlessly in MUIv4. But after the MUIv5 update I had to copy getOverlayAlpha and hardcode it and the linear-gradient code into my app just to make it work in dark mode again.

eps1lon commented 3 years ago

Thanks for the feedback.

I would agree that the Paper should just use the shadows from the theme as before. Anything else would not really be a design system if every component had different shadows i.e. shadows should not be part of the theme if not every component uses the same shadows.

Seems like https://github.com/mui-org/material-ui/pull/25522 was fairly controversial. We may want to revert before stable release until we have an approach that works on all components.

Brodzko commented 2 years ago

Thanks for the feedback.

I would agree that the Paper should just use the shadows from the theme as before. Anything else would not really be a design system if every component had different shadows i.e. shadows should not be part of the theme if not every component uses the same shadows.

Seems like #25522 was fairly controversial. We may want to revert before stable release until we have an approach that works on all components.

I would argue to the contrary - I think it's sensible to work with surface lightening in dark mode theme but would prefer more flexibility on the system level. Here's some points that I'm finding I could use in our DS but having to work around them with current release:

However I'm not sure how to approach the latter point, since having no shadows kind of renders the semantic meaning of elevation pointless?

I'd be happy if this discussion got some attention, happy to contribute 🙂

Brodzko commented 2 years ago

I've been thinking about this recently and I think a good alternative would be to control the dark mode overlays and shadows separately? Based on the Material Design Elevation spec, shadows are determined by distance from the elevated surface to the closest surface underneath it - so e.g. a Paper with elevation={8} on top of another Paper with elevation={4} should

Has this been discussed anywhere? The only way to achieve this I can currently see is by having a custom Paper component (and having to deal with replacing default Paper instances in components that use it by default, which is not always possible (think Dialog), or by passing a component prop to Paper by default - but Paper then doesn't forward relevant props like elevation.

Apologies if I'm unclear in some points, writing this in a bit of a haste.