Skip to content
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

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

Open
1 task done
dantman opened this issue Aug 26, 2021 · 3 comments
Open
1 task done

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

dantman opened this issue Aug 26, 2021 · 3 comments
Labels
component: Paper This is the name of the generic UI component, not the React module! new feature New feature or request v5.x migration

Comments

@dantman
Copy link
Contributor

dantman commented Aug 26, 2021

  • I have searched the issues of this repository and believe that this is not a duplicate.

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.

@dantman dantman added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 26, 2021
@eps1lon
Copy link
Member

eps1lon commented Aug 27, 2021

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.

@eps1lon eps1lon added component: Paper This is the name of the generic UI component, not the React module! new feature New feature or request v5.x migration and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 27, 2021
@Brodzko
Copy link

Brodzko commented Nov 14, 2021

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:

  • Configure custom white overlays for different elevations as per Dark mode elevation: Make white overlay configurable in theme? #29643
  • Having the option to disableShadows on a Paper - I like the idea of working with a surface on a system level but not all our surfaces have shadows, however by adding a new variant, say shadowLess to a Paper removes the white overlays altogether. Disabling boxShadow manually feels unsystematic and having a custom component requires us to pass PaperComponent whenever we want a custom component to use this variant.

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
Copy link

Brodzko commented Aug 25, 2022

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

  • have a box-shadow equivalent to elevation={4} (8 - 4)
  • have an overlay in dark theme equivalent to elevation={8} (it is still closer to the "light source" than the underlying Paper 4

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Paper This is the name of the generic UI component, not the React module! new feature New feature or request v5.x migration
Projects
None yet
Development

No branches or pull requests

3 participants