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

slots overrides do not merge with the theme #34214

Closed
flaviendelangle opened this issue Sep 7, 2022 · 6 comments · Fixed by #35477
Closed

slots overrides do not merge with the theme #34214

flaviendelangle opened this issue Sep 7, 2022 · 6 comments · Fixed by #35477
Assignees
Labels
bug 🐛 Something doesn't work customization: theme Centered around the theming features

Comments

@flaviendelangle
Copy link
Member

Problem

How does the theme default props work

Users can define default props for there components using the theme as follow:

const theme = createTheme({
  components: {
    MuiDatePicker: {
      defaultProps: {
        closeOnSelect: true,
      },
    },
  },
});

The props passed to the theme, will then be merged with the props passed to the component with a shallow merge.

Object.keys(defaultProps).forEach((propName: keyof T) => {
if (output[propName] === undefined) {
output[propName] = defaultProps[propName];
}
});

What is the problem with the slots ?

The way the merge currently works, makes it very difficult, or even impossible for users to override a slot component (or some slot component props) directly from the theme.

For instance, if a user wants to customize the left and right arrow icons on all the date pickers of its application, he will not be able to use the theme.

Since the merge is a shallow merge, the props.components passed to DatePicker overrides the one stored in the theme default props instead of being merged with it.

Example with the props: working

Example with the theme: not working

When is this problem blocking ?

I did not find any actual use case on the core and I don't think users should always override the slots through the theme.
But on bigger components like the one provided by X, the ability of overriding some slot once and for all can make some customization a lot easier.

Here are a few example I have in mind:

DataGrid + Joy

@siriwatknp did a POC during our retreat (https://github.com/mui/mui-x/pull/5360/files).
The behavior could be even better if users could just import a method to populate the theme with joy slots.
This could look something like:

import { addJoyDataGrid } from '@mui/x-data-grid/joy';

const theme = createTheme({ /** Some theme customization not related to the data grid **/ });
const themeWithJoyDataGrid = addJoyDataGrid(theme);

// Each data grid usage does not have to care about the theme
const UserList = () => {
  const users = useUsers();
  
  return (
    <DataGrid {...users} />
  );
};

export default function App() {
  return (
    <ThemeProvider theme={themeWithJoyDataGrid}>
      <UserList />
    </ThemeProvider>
  );
}

Custom DatePicker with custom input

The current version of the pickers requires a renderInput prop.
This prop can be defined in the theme to avoid defining it on each picker (with @ts-ignore to avoid having TS complain that the prop is required)
Working example

In v6, we are discussing replacing this prop with a component slot to be coherent with the other component customization.
But it would prevent users from defining it in the theme, which would be a clean solution I would like to encourage.

Proposal

For the components prop, increase the depth of the merge of one.

--- a/packages/mui-utils/src/resolveProps.ts
+++ b/packages/mui-utils/src/resolveProps.ts
@@ -4,14 +4,20 @@
  * @param {object} props
  * @returns {object} resolved props
  */
-export default function resolveProps<T extends { className?: string } & Record<string, unknown>>(
-  defaultProps: T,
-  props: T,
-) {
+export default function resolveProps<
+  T extends { className?: string; components?: Record<string, unknown> } & Record<string, unknown>,
+>(defaultProps: T, props: T) {
   const output = { ...props };
 
   Object.keys(defaultProps).forEach((propName: keyof T) => {
-    if (output[propName] === undefined) {
+    if (propName === 'components') {
+      if (defaultProps[propName] !== undefined) {
+        output[propName] =
+          output[propName] === undefined
+            ? defaultProps[propName]
+            : resolveProps(defaultProps[propName] as any, output[propName]);
+      }
+    } else if (output[propName] === undefined) {
       output[propName] = defaultProps[propName];
     }
   });

Advantages

  • Users will be able to define the component slot on the theme
  • We will be able to provide methods to enrich the theme and customize our X components for Joy

Disadvantages

  • It adds some additional complexity
  • How can user override there default props on a specific component to fallback on the built-in default value ?

Follow up

I focused this RFC on the components prop, but the same can be discussed for the componentsProps prop.

@flaviendelangle flaviendelangle added customization: theme Centered around the theming features RFC Request For Comments labels Sep 7, 2022
@flaviendelangle flaviendelangle self-assigned this Sep 7, 2022
@siriwatknp
Copy link
Member

siriwatknp commented Sep 12, 2022

In general, this makes sense to me. We can do a POC with Joy UI in the next quarter together with the Data Grid integration.

@flaviendelangle
Copy link
Member Author

Would you consider it a breaking change ? Technically it's a behavior change but I don't know who would currently use the theme to define components and expect the current behavior.

@siriwatknp
Copy link
Member

Would you consider it a breaking change ?

I don't think so. If my understanding is correct, the change is basically merging components prop from the theme and on the instance together, right?

Technically it's a behavior change but I don't know who would currently use the theme to define components and expect the current behavior.

Yeah, I feel that it will benefit the X components more than in general usage. I agree with the proposal that the components should be merged. It is just a matter of when we should create a PR, may be let's wait for @mnajdova's opinion first.

@siriwatknp siriwatknp added the waiting for 👍 Waiting for upvotes label Sep 15, 2022
@flaviendelangle
Copy link
Member Author

the change is basically merging components prop from the theme and on the instance together, right?

Yes

@mnajdova
Copy link
Member

Agree with the proposal. I would generally consider this as a bug fix, not a breaking change. I would assume this is what people would expect, but I also think this is not widely used so far, so it may even go unnoticed.

@siriwatknp siriwatknp added bug 🐛 Something doesn't work and removed waiting for 👍 Waiting for upvotes RFC Request For Comments labels Sep 20, 2022
@siriwatknp siriwatknp changed the title [RFC] Allow component slot override through theme components overrides do not merge with the theme Sep 20, 2022
@siriwatknp siriwatknp changed the title components overrides do not merge with the theme slots overrides do not merge with the theme Sep 20, 2022
@ZeeshanTamboli
Copy link
Member

A PR was opened for it in #35088

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work customization: theme Centered around the theming features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants