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

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

Open
2 tasks done
guoyunhe opened this issue Jun 14, 2023 · 8 comments · May be fixed by #44729
Open
2 tasks done

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

guoyunhe opened this issue Jun 14, 2023 · 8 comments · May be fixed by #44729
Assignees
Labels
component: card This is the name of the generic UI component, not the React module! customization: dom Component's DOM customizability, e.g. slot customization: theme Centered around the theming features package: material-ui Specific to @mui/material

Comments

@guoyunhe
Copy link

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example:

Steps:

  1. Create a customized theme:
const theme = createTheme({
  components: {
    MuiCardHeader: {
      defaultProps: {
        titleTypographyProps: {
          variant: 'h3',
        },
      }
    }
  }
});
  1. Use the theme:
<ThemeProvider theme={theme}>
  <Card>
    <CardHeader title="Title" />
  </Card>
  <Card>
    <CardHeader title="Title" titleTypographyProps={{}} />
  </Card>
</ThemeProvider>
  1. 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.
@guoyunhe guoyunhe added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 14, 2023
@DavidCnoops DavidCnoops added component: card This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 14, 2023
@alexey-kozlenkov
Copy link
Contributor

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

alexey-kozlenkov commented Jul 26, 2024

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

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

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

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

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

@DiegoAndai DiegoAndai added the customization: dom Component's DOM customizability, e.g. slot label Aug 27, 2024
@DiegoAndai DiegoAndai assigned DiegoAndai and unassigned mnajdova Aug 27, 2024
@DiegoAndai DiegoAndai added customization: theme Centered around the theming features customization: dom Component's DOM customizability, e.g. slot and removed customization: dom Component's DOM customizability, e.g. slot labels Aug 27, 2024
@DiegoAndai
Copy link
Member

DiegoAndai commented Aug 27, 2024

@alexey-kozlenkov you can follow the progress on slots being implemented here: #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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: card This is the name of the generic UI component, not the React module! customization: dom Component's DOM customizability, e.g. slot customization: theme Centered around the theming features package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants