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.89k stars 32.26k forks source link

[Theme Customization] defaultProps doesn't support deep merge #37585

Open guoyunhe opened 1 year ago

guoyunhe commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example:

Steps:

  1. Create a customized theme:
    const theme = createTheme({
    components: {
    MuiCardHeader: {
      defaultProps: {
        titleTypographyProps: {
          variant: 'h3',
        },
      }
    }
    }
    });
  2. Use the theme:
    <ThemeProvider theme={theme}>
    <Card>
    <CardHeader title="Title" />
    </Card>
    <Card>
    <CardHeader title="Title" titleTypographyProps={{}} />
    </Card>
    </ThemeProvider>
  3. Compare the two cards

Current behavior 😯

The second card doesn't respect the theme customization. Because it has an {} prop value that replace (instead of merge) the theme's defaultProps.

Expected behavior 🤔

Should the props be deep merged with defaultProps?

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo ``` Don't forget to mention which browser you used. Output from `npx @mui/envinfo` goes here. ```
alexey-kozlenkov commented 3 months ago

Facing the same issue, can't define default elevation for dialog via i.e. { defaultProps: { PaperProps: { elevation: 5 } } }. Anytime anybody provides some PaperProps when using dialog, elevation is lost.

@mnajdova you have any update on this?

alexey-kozlenkov commented 3 months ago

I did some digging and found out that this MR happened, which seem to resolve deep merge, but only for *slotProps properties.

However, not all of the components internals customisable via slotProps, i.e. Paper is not a slot for Dialog component. Same goes for original example by @guoyunhe.

As an idea perhaps that deep merge could be applied to any prop that ends with Props (e.g. /[a-z]+Props$/) ? Summoning @flaviendelangle @siriwatknp as authors of the change for slot props

Just in case it needs any more reproduction: https://codesandbox.io/p/devbox/default-props-deep-merge-issue-h46fxm?file=%2Fsrc%2FDemo.tsx%3A36%2C52

siriwatknp commented 3 months ago

At first, the plan was to add missing slotProps to all components in v6 but we are still discussing the possibility of splitting each component into smaller subcomponents. That's why we haven't move forward with the missing slotProps.

Anyway, I still want to add the missing slotProps to make v6 complete because I don't think we will remove them in v7 regardless of subcomponents. What I picture in v7 is that the component + its slots will be exported and users can decide which one to use based on their use cases, for example, an Autocomplete:

// v7
// use case I: simple use case
import Autocomplete from '@mui/material/Autocomplete';

<Autocomplete … slotProps={{ input: { … }, arrow: {…} }} />

// use case II: composition
import { AutocompleteRoot, AutocompleteInput, Autocomplete… } from '@mui/material/Autocomplete';

<AutocompleteRoot>…</AutocompleteRoot>

What's your thought? @DiegoAndai @aarongarciah

alexey-kozlenkov commented 3 months ago

@siriwatknp

to add missing slotProps to all components in v6

Does it mean that all component will have ability to provide any of their children Props via slotProps? i.e. for Dialog, PaperProps would appear as part of slotProps (cause right now PaperProps is just a part of DIalog's props).

siriwatknp commented 3 months ago

Does it mean that all component will have ability to provide any of their children Props via slotProps? i.e. for Dialog, PaperProps would appear as part of slotProps (cause right now PaperProps is just a part of DIalog's props).

Yes, that's correct. @DiegoAndai I think we miss the Dialog, it haven't been migrated to slotProps from https://next.mui.com/material-ui/migration/migrating-from-deprecated-apis/.

alexey-kozlenkov commented 2 months ago

@siriwatknp If I willing to contribute to expedite this issue, could you perhaps point me to some PR example for this change?

DiegoAndai commented 2 months ago

@alexey-kozlenkov you can follow the progress on slots being implemented here: https://github.com/mui/material-ui/issues/41281

But this issue is on hold as we discuss the path forward regarding slots. @siriwatknp's suggestion makes sense to me, but there's no agreement yet. When there's an agreement, which should come soon, we'll know the next step to solve this issue.

alexey-kozlenkov commented 2 months ago

@DiegoAndai I see, thanks for replying here. I'll follow noted issue then and wait for the updates