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.12k stars 32.07k forks source link

[Paper] Custom variants disregard elevation prop #34615

Open NerdCowboy opened 1 year ago

NerdCowboy commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Steps:

  1. Create a custom Paper variant
  2. Add variant prop with the new variant to a Paper component (or Paper-composed components e.g. Card)
  3. Add elevation prop greater than 0 to component
  4. Inspect Paper component's properties. The elevation's box-shadow and classnames will be missing.

Codesandbox demo

Current behavior 😯

If you create a custom variant for a Paper component, it won't include any elevation styles if you try to add the elevation prop in tandem with the custom variant prop e.g. <Paper variant="blue" elevation={5}>No elevation shadows here</Paper>

Expected behavior 🤔

Custom variants should include the default elevation styles unless explicitly overridden

Context 🔦

I'm attempting to create a new Paper variant and expect it to inherit all the other styles of Paper.

Your environment 🌎

npx @mui/envinfo ``` Browser: Chrome 105.0.5195.125 System: OS: macOS 12.6 Binaries: Node: 16.13.2 - ~/.nvm/versions/node/v16.13.2/bin/node Yarn: Not Found npm: 8.5.5 - ~/.nvm/versions/node/v16.13.2/bin/npm Browsers: Chrome: 105.0.5195.125 Edge: Not Found Firefox: 103.0.1 Safari: 16.0 npmPackages: @emotion/react: ^11.10.4 => 11.10.4 @emotion/styled: ^11.10.4 => 11.10.4 @mui/base: 5.0.0-alpha.98 @mui/core-downloads-tracker: 5.10.6 @mui/icons-material: ^5.10.6 => 5.10.6 @mui/lab: 5.0.0-alpha.101 => 5.0.0-alpha.101 @mui/material: ^5.10.6 => 5.10.6 @mui/private-theming: 5.10.6 @mui/styled-engine: 5.10.8 @mui/system: 5.10.8 @mui/types: 7.2.0 @mui/utils: ^5.10.6 => 5.10.6 @mui/x-data-grid: 5.17.5 @mui/x-data-grid-pro: ^5.17.5 => 5.17.5 @mui/x-date-pickers: 5.0.3 => 5.0.3 @mui/x-date-pickers-pro: 5.0.3 => 5.0.3 @mui/x-license-pro: ^5.17.0 => 5.17.0 @types/react: ^18.0.21 => 18.0.21 react: ^18.2.0 => 18.2.0 react-dom: ^18.2.0 => 18.2.0 typescript: ^4.8.4 => 4.8.4 ```
siriwatknp commented 1 year ago

Before diving into the solution, I see 2 ways:

  1. If the Paper's elevation prop is designed to work only with variant="elevation", this issue is not a bug but more of the docs improvement that should clarify this point.
  2. If elevation prop is independent of other props, then this is a bug.

Currently, I don't see a good solution without breaking changes. The structure of the current props does not sound right to me. Ideally, I favor independent props over interrelated props, meaning there should be no such variant="elevation":

Let's find out the proper solution first, opinions are welcome!

siriwatknp commented 1 year ago

@NerdCowboy For the workaround, you can access the ownerState in a callback and apply the elevation manually. https://codesandbox.io/s/paper-variant-doesnt-inherit-elevation-forked-9xrw7n?file=/index.js

<ThemeProvider
  theme={createTheme({
    components: {
      MuiPaper: {
        variants: [
          {
            props: { variant: "blue" },
            style: ({ ownerState, theme }) => ({
              backgroundColor: "blue",
              boxShadow: theme.shadows[ownerState.elevation]
            })
          }
        ]
      }
    }
  })}
>
NerdCowboy commented 1 year ago

@siriwatknp Appreciate the workaround!

Ya I'd agree. I'd say the code appears to be a bit nonstandard since elevation is a separate prop, but is treated as another variant under the hood which is rather unintuitive.

imo most components should have the ability to set elevation height independently of their other styles as its one job is to show depth regardless of other styles (e.g. Button has multiple variants, but it handles elevation separately, albeit only on/off).

It seems elevation's utility became conflated due to the presence of Paper's outlined variant since elevation isn't desired there, so I think the solution should be to warn when using together for that variant but always show otherwise.

mightym commented 1 year ago

@siriwatknp awesome workaround! Thanks a lot. That just saved me a ton of time. I often have the situation where I need to combine elevation and different background colors <3