[Accordion] Merge object props on resolve props, support TransitionProps

[trizotti]

Resolves #34978

• [x] I have followed (at least) the PR section of the contributing guide.

Before:

• Accordion: https://codesandbox.io/s/twilight-sun-nn4m8m?file=/src/App.tsx
• DatePicker: https://codesandbox.io/s/date-and-time-pickers-forked-3lgyks?file=/src/App.tsx

After:

• Accordion: https://codesandbox.io/s/create-react-app-with-typescript-forked-0kcu01?file=/src/App.tsx
• DatePicker: https://codesandbox.io/s/create-react-app-with-typescript-forked-ep95ci?file=/src/App.tsx

[mui-bot]

Netlify deploy preview

https://deploy-preview-35088--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by :no_entry_sign: dangerJS against 2424f696a9edb12d84d3018e9bf105b00b2f2951

[trizotti]

@siriwatknp , could you review this, please?

[siriwatknp]

@mnajdova if you can have a look I am pretty sure we don't want to blindly merge all the object-like props. The current check would even merge Date object which is really really not what we want. And even apart from date, on the DataGrid if we define a default filterModel with the theme and one with the props, I doubt the expected behavior would be to merge both (same for most of the other object models).

I would prefer to have a white list of props on which this behavior can be applied.

Agree, the merging should be explicit for specific props.

[trizotti]

@flaviendelangle, I added a whitelist for merge specific props. Could take a look at the last changes, please?

[trizotti]

@siriwatknp , I added your suggestions and also small fixes to get rid of some Typescript/CI errors that appeared after the changes:

• [x] Remove optional question mark once there is a default value for shallowMergePropNames;
• [x] Force string conversion for propName;
• [x] Run prettier.

[trizotti]

@ZeeshanTamboli, I added all your suggestions. But I'm facing problems with the CI checks.

The only changes that were made since the last successful checks are in the Accordion test file itself; but after adding this new test the checks are not passing anymore. The errors point to files that just are not related to the task files at all...

Could you help me with this, please?

[siriwatknp]

@ZeeshanTamboli, I added all your suggestions. But I'm facing problems with the CI checks.

The only changes that were made since the last successful checks are in the Accordion test file itself; but after adding this new test the checks are not passing anymore. The errors point to files that just are not related to the task files at all...

Could you help me with this, please?

I'm taking a look.

[siriwatknp]

I am reluctant to merge because this might break some projects that don't want the props merging. cc @mui/core

[flaviendelangle]

@siriwatknp could we merge the new param on useThemeProps even if you are not using it yet on the core components ? On X it starts to cause some serious problems (see https://github.com/mui/mui-x/issues/8345)

Note that on the linked issue the default props on the theme overrides a slot props.

[mnajdova]

@siriwatknp could we merge the new param on useThemeProps even if you are not using it yet on the core components ? On X it starts to cause some serious problems (see https://github.com/mui/mui-x/issues/8345)

Note that on the linked issue the default props on the theme overrides a slot props.

I believe we can do this, add the new option in useThemeProps and don't use it in any core components yet. We can update them in v6. @trizotti are you still interested to continue the effort?

[oliviertassinari]

Thoughts:

I am reluctant to merge because this might break some projects that don't want the props merging.

1. The docs say that these theme values set the default prop: https://mui.com/material-ui/customization/theme-components/#theme-default-props. Changing the behavior to be a deep merge could be considered a breaking change.

2. Personally, the test doesn't connect for me, TransitionProps={{ onEnter }} is setting the TransitionProps prop, so I think that we should expect the onExit property to be reset. For the same reason why when overriding Fade's timeout prop https://mui.com/material-ui/api/fade/, it erases the default prop.

3. How do we document this behavior?

4. The current approach seems to be on a case-by-case basis. Does it mean that we would update all the objects to support this? How much JavaScript to the bundle would be added for everyone? vs. some developers creating a wrapper.

5. At a higher level, I think that it shouldn't be easy to make heavy customization from the theme because it's a singleton. We wouldn't want people to turn their theme into a 50 kB gzipped JS file because they could. Why not make customizations easier, but to be mindful about this.

[siriwatknp]

Thoughts:

I am reluctant to merge because this might break some projects that don't want the props merging.

1. The docs say that these theme values set the default prop: https://mui.com/material-ui/customization/theme-components/#theme-default-props. Changing the behavior to be a deep merge could be considered a breaking change.
2. Personally, the test doesn't connect for me, TransitionProps={{ onEnter }} is setting the TransitionProps prop, so I think that we should expect the onExit property to be reset. For the same reason why when overriding Fade's timeout prop https://mui.com/material-ui/api/fade/, it erases the default prop.
3. How do we document this behavior?
4. The current approach seems to be on a case-by-case basis. Does it mean that we would update all the objects to support this? How much JavaScript to the bundle would be added for everyone? vs. some developers creating a wrapper.
5. At a higher level, I think that it shouldn't be easy to make heavy customization from the theme because it's a singleton. We wouldn't want people to turn their theme into a 50 kB gzipped JS file because they could. Why not make customizations easier, but to be mindful about this.

Removing the nested slots makes the most sense to me but I'm not sure if it can be done in complex components like DatePickers.

[flaviendelangle]

Personally, the test doesn't connect for me, TransitionProps={{ onEnter }} is setting the TransitionProps prop, so I think that we should expect the onExit property to be reset. For the same reason why when overriding Fade's timeout prop https://mui.com/material-ui/api/fade/, it erases the default prop.

For me the problem is that depending on the use case, people might want to override the whole object or they might want to just add one property. On the inputProps / InputProps use case on the fields / pickers, it is clear to me that most users do not want to override the whole object, because it contains props needed for the editing to work, and almost impossible to override.

Removing the nested slots makes the most sense to me but I'm not sure if it can be done in complex components like DatePickers.

I clearly not a fan of having nested slots like this, but I don't see a decent alternative.

We can indeed advise people to use wrappers more often for complex use-case. But right now I feel like we are loosing a lot of people around these customization topics

[oliviertassinari]

On the inputProps / InputProps use case on the fields / pickers, it is clear to me that most users do not want to override the whole object, because it contains props needed for the editing to work, and almost impossible to override.

@flaviendelangle Is this solvable at the component implementation level rather than theme level? I have tried to reproduce https://github.com/mui/mui-x/issues/8345 but couldn't: https://github.com/mui/mui-x/issues/8345#issuecomment-1557268589.

Also, I haven't seen too much struggle with this on the Autocomplete. There were, but maybe now people have a good first result when they search on Google so don't complain anymore. Or maybe not using slots helped.

[flaviendelangle]

The bug is not longer reproducible because I added this to the code

But it's super error-prone to have to define it on every usage. The issue here is not the merge of the theme but the merge of the slot.

For the merge of the theme, it would cause bug as well if you used theme.components.MuiDateTimeField.defaultProps.InputProps see this example

For me, we have to put some boundaries on what people can do. I fully agree that overriding theme.components.{componentName}.slotProps.{slotName} is very risky and probably not something we want to do most of the time.

But I think the use case I displayed in the link above is valid and should work.