[Slider] Customize disabled state

[mtr1990]

I have overridden the disabled state of Slider.

but I don't know which syntax is correct:

const theme = createMuiTheme({
  components: {
    MuiSlider: {
      styleOverrides: {
        root: {
           // case 1: Working
          // "&.MuiSlider-root.Mui-disabled": {
          //   color: "red"
          // }

          // case 2: Not Working
          "&.Mui-disabled": {
            color: "red"
          }
        }
      }
    }
  }
});

Usually, I use method 2. but it doesn't seem to work in this case

More details:

https://codesandbox.io/s/continuousslider-material-demo-forked-3qphg

[povilass]

You need only but style overrides not working I think it is a bug.

components: {
    MuiSlider: {
      styleOverrides: {
        disabled: {
          color: "red"
        }
      }
    }
  }

[povilass]

@oliviertassinari I can look into it and make changes later.

[oliviertassinari]

@povilass Ok, great, it would help. #25075 (comment) should throw a warning, we don't support it.

[povilass]

@oliviertassinari I think we supported this in the previous version it just broken in v5 after migration into emotion

[oliviertassinari]

@povilass We throw a warning against it. It was added the warning a year or two ago. The warning can be seen in the reproduction:

https://codesandbox.io/s/continuousslider-material-demo-forked-pv2ut?file=/index.js:479-565

[oliviertassinari]

Regarding this very issue. I have stopped looking at this root cause:

  deepmerge({
    '&.Mui-disabled': {
      color: 'red',
    },
  }, {
    '&.Mui-disabled': undefined,
  });
  
// KO, no deep merge

A workaround: https://codesandbox.io/s/continuousslider-material-demo-forked-f8rie?file=/index.js, but it's not OK. We need to fix this. I recall we had a discussion with @mnajdova around how to best merge the different styles.

[povilass]

Yes just looked into it got the same warning.

[povilass]

Ok, I understand what is happening now gonna think for a while about what can we do with it and how to resolve the problem in all components.

[oliviertassinari]

I would propose this fix:

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 6eebcfece1..fb7d25179b 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -1,7 +1,7 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
-import { chainPropTypes, deepmerge } from '@material-ui/utils';
+import { chainPropTypes } from '@material-ui/utils';
 import { generateUtilityClasses, isHostComponent } from '@material-ui/unstyled';
 import SliderUnstyled, {
   SliderValueLabelUnstyled,
@@ -37,7 +37,7 @@ const overridesResolver = (props, styles) => {

   const marked = marks.length > 0 && marks.some((mark) => mark.label);

-  return deepmerge(styles.root || {}, {
+  return {
     ...styles[`color${capitalize(styleProps.color)}`],
     [`&.${sliderClasses.disabled}`]: styles.disabled,
     ...(marked && styles.marked),
@@ -54,7 +54,8 @@ const overridesResolver = (props, styles) => {
       ...styles[`thumbColor${capitalize(styleProps.color)}`],
       [`&.${sliderClasses.disabled}`]: styles.disabled,
     },
-  });
+    ...styles.root
+  };
 };

 export const SliderRoot = experimentalStyled(

It's also faster not to have to run deep merge. The downside of the above is that you can't do:

const theme = createMuiTheme({
  components: {
    MuiSlider: {
      styleOverrides: {
        root: {
          '& .MuiSlider-rail': {
            color: "green",
          },
        },
        rail: { // no effect
          height: 4,
        }
      }
    }
  }
});

IMHO, this downside is fine as it encourages the direction we are trying to promote.

Now, to make it clear that the current deepmerge behavior is correct, I would add a test case:

diff --git a/packages/material-ui-utils/src/deepmerge.test.ts b/packages/material-ui-utils/src/deepmerge.test.ts
index 8440abe055..2df7797f8d 100644
--- a/packages/material-ui-utils/src/deepmerge.test.ts
+++ b/packages/material-ui-utils/src/deepmerge.test.ts
@@ -20,4 +20,21 @@ describe('deepmerge', () => {

     expect(result.element).to.equal(element2);
   });
+
+  // https://github.com/mui-org/material-ui/issues/25075
+  it('should reset source when target is undefined', () => {
+    const result = deepmerge(
+      {
+        '&.Mui-disabled': {
+          color: 'red',
+        },
+      },
+      {
+        '&.Mui-disabled': undefined,
+      },
+    );
+    expect(result).to.deep.equal({
+      '&.Mui-disabled': undefined,
+    });
+  });
 });

The alternative solution, that matches a bit closer to what @mnajdova wanted to optimize for in the beginning is:

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index 6eebcfece1..b4e142941b 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -37,7 +37,7 @@ const overridesResolver = (props, styles) => {

   const marked = marks.length > 0 && marks.some((mark) => mark.label);

-  return deepmerge(styles.root || {}, {
+  return deepmerge({
     ...styles[`color${capitalize(styleProps.color)}`],
     [`&.${sliderClasses.disabled}`]: styles.disabled,
     ...(marked && styles.marked),
@@ -54,7 +54,7 @@ const overridesResolver = (props, styles) => {
       ...styles[`thumbColor${capitalize(styleProps.color)}`],
       [`&.${sliderClasses.disabled}`]: styles.disabled,
     },
-  });
+  }, styles.root || {});
 };

 export const SliderRoot = experimentalStyled(

I personally think that we don't need to do the extra mile with a deepmerge but happy to mention that it's also an option.

[mnajdova]

Coming a bit late to this, but seems like changing the order of the root and the other overrides is creating a problems. See https://codesandbox.io/s/wandering-cloud-pvpwc?file=/src/App.js:0-592 Currently if we do:

const theme = createMuiTheme({
  components: {
    MuiButton: {
      styleOverrides: {
        root: {
          border: "2px dashed grey"
        },
        contained: {
          // shouldn't this win?
          border: "none"
        }
      }
    }
  }
});

The styles defined under root win which is not intuitive at all. I know we came again to this multiple time, but it's important to get it right.

So we should do one of these options:

Option 1:

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index cf52016d61..51807c69b0 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -13,7 +13,7 @@ import buttonClasses, { getButtonUtilityClass } from './buttonClasses';
 const overridesResolver = (props, styles) => {
   const { styleProps } = props;

-  return deepmerge(
+  return deepmerge(styles.root || {},
     {
       ...styles[styleProps.variant],
       ...styles[`${styleProps.variant}${capitalize(styleProps.color)}`],
@@ -22,7 +22,9 @@ const overridesResolver = (props, styles) => {
       ...(styleProps.color === 'inherit' && styles.colorInherit),
       ...(styleProps.disableElevation && styles.disableElevation),
       ...(styleProps.fullWidth && styles.fullWidth),
-      [`& .${buttonClasses.label}`]: styles.label,
+      [`& .${buttonClasses.label}`]: {
+        ...styles.label
+      },
       [`& .${buttonClasses.startIcon}`]: {
         ...styles.startIcon,
         ...styles[`iconSize${capitalize(styleProps.size)}`],
@@ -32,7 +34,6 @@ const overridesResolver = (props, styles) => {
         ...styles[`iconSize${capitalize(styleProps.size)}`],
       },
     },
-    styles.root || {},
:                        

Or completely drop the deepmerge, which I am starting to lean more forward seeing the different issues we may stumble apon.

Option 2:

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index cf52016d61..50cf4d1077 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -1,7 +1,6 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
-import { deepmerge } from '@material-ui/utils';
 import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
 import experimentalStyled, { shouldForwardProp } from '../styles/experimentalStyled';
 import useThemeProps from '../styles/useThemeProps';
@@ -13,27 +12,27 @@ import buttonClasses, { getButtonUtilityClass } from './buttonClasses';
 const overridesResolver = (props, styles) => {
   const { styleProps } = props;

-  return deepmerge(
-    {
-      ...styles[styleProps.variant],
-      ...styles[`${styleProps.variant}${capitalize(styleProps.color)}`],
-      ...styles[`size${capitalize(styleProps.size)}`],
-      ...styles[`${styleProps.variant}Size${capitalize(styleProps.size)}`],
-      ...(styleProps.color === 'inherit' && styles.colorInherit),
-      ...(styleProps.disableElevation && styles.disableElevation),
-      ...(styleProps.fullWidth && styles.fullWidth),
-      [`& .${buttonClasses.label}`]: styles.label,
-      [`& .${buttonClasses.startIcon}`]: {
-        ...styles.startIcon,
-        ...styles[`iconSize${capitalize(styleProps.size)}`],
-      },
-      [`& .${buttonClasses.endIcon}`]: {
-        ...styles.endIcon,
-        ...styles[`iconSize${capitalize(styleProps.size)}`],
-      },
+  return {
+    ...styles.root,
+    ...styles[styleProps.variant],
+    ...styles[`${styleProps.variant}${capitalize(styleProps.color)}`],
+    ...styles[`size${capitalize(styleProps.size)}`],
+    ...styles[`${styleProps.variant}Size${capitalize(styleProps.size)}`],
+    ...(styleProps.color === 'inherit' && styles.colorInherit),
+    ...(styleProps.disableElevation && styles.disableElevation),
+    ...(styleProps.fullWidth && styles.fullWidth),
+    [`& .${buttonClasses.label}`]: {
+      ...styles.label,
     },
-    styles.root || {},
-  );
+    [`& .${buttonClasses.startIcon}`]: {
+      ...styles.startIcon,
+      ...styles[`iconSize${capitalize(styleProps.size)}`],
+    },
+    [`& .${buttonClasses.endIcon}`]: {
+      ...styles.endIcon,
+      ...styles[`iconSize${capitalize(styleProps.size)}`],
+    },
+  };
 };

 const useUtilityClasses = (styleProps) => {

[oliviertassinari]

@mnajdova Oh my bad. I didn't realize this constraint existed. Are you sure about the diff in option 1. ? It doesn't solve the issue, on my end.

[mnajdova]

@mnajdova Oh my bad. I didn't realize this constraint existed. Are you sure about the diff in option 1. ? It doesn't solve the issue, on my end.

Can you share a scenario that doesn't work? I was able to override both the root as well as the styles for the label slot via:

const theme = createMuiTheme({
  components: {
    MuiButton: {
      styleOverrides: {
        root: {
          border: "2px dashed grey"
        },
        contained: {
          // shouldn't this win?
          border: "none"
        },
        label: {
          width: '50%',
        }
      }
    }
  }
});

The only thing we need to keep an eye on is to default to an empty object if there isn't any override, hence I changed it to new object and spread. Again, I think this is easy to be overlooked..

[oliviertassinari]

The problem I see with option 1. is the reproduction provided at the top of this issue. I believe it's breaking it. Only option 2. seems to satisfies both constraints (I didn't test). Try:

const theme = createMuiTheme({
  components: {
    MuiButton: {
      styleOverrides: {
        root: {
          border: "2px dashed grey"
          "&.Mui-disabled": {
            color: "red"
          }
        },
        contained: {
          border: "none"
        },
        label: {
          width: '50%',
        }
      }
    }
  }
});

[mnajdova]

@oliviertassinari I've created https://codesandbox.io/s/optimistic-feynman-y5teq?file=/src/App.js based on #25803 which applies Option 1. I don't see any issues (or maybe I am too sleepy 😃) Should we maybe test on the Slider component? The important change on the PR is the change on the label overrides

[oliviertassinari]

@mnajdova It works, you are right. So it's not only an issue with the Button but with all the components. e.g. in the slider so the color wins over the root.

diff --git a/packages/material-ui/src/Slider/Slider.js b/packages/material-ui/src/Slider/Slider.js
index f2fd1ff942..7ae62e11bc 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -37,10 +37,10 @@ const overridesResolver = (props, styles) => {

   const marked = marks.length > 0 && marks.some((mark) => mark.label);

-  return deepmerge(
+  return deepmerge(styles.root || {},
     {
       ...styles[`color${capitalize(styleProps.color)}`],
-      [`&.${sliderClasses.disabled}`]: styles.disabled,
+      [`&.${sliderClasses.disabled}`]: {...styles.disabled},
       ...(marked && styles.marked),
       ...(styleProps.orientation === 'vertical' && styles.vertical),
       ...(styleProps.track === 'inverted' && styles.trackInverted),
@@ -56,7 +56,6 @@ const overridesResolver = (props, styles) => {
         [`&.${sliderClasses.disabled}`]: styles.disabled,
       },
     },
-    styles.root || {},
   );
 };

I would personally prefer option 3. It seems simpler, maybe a bit faster at runtime and prevent developer to mix nested selectors in the root with a slot. They have to pick one approach, which should avoid footguns:

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index cf52016d61..a112f3548b 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -13,27 +13,25 @@ import buttonClasses, { getButtonUtilityClass } from './buttonClasses';
 const overridesResolver = (props, styles) => {
   const { styleProps } = props;

-  return deepmerge(
-    {
-      ...styles[styleProps.variant],
-      ...styles[`${styleProps.variant}${capitalize(styleProps.color)}`],
-      ...styles[`size${capitalize(styleProps.size)}`],
-      ...styles[`${styleProps.variant}Size${capitalize(styleProps.size)}`],
-      ...(styleProps.color === 'inherit' && styles.colorInherit),
-      ...(styleProps.disableElevation && styles.disableElevation),
-      ...(styleProps.fullWidth && styles.fullWidth),
-      [`& .${buttonClasses.label}`]: styles.label,
-      [`& .${buttonClasses.startIcon}`]: {
-        ...styles.startIcon,
-        ...styles[`iconSize${capitalize(styleProps.size)}`],
-      },
-      [`& .${buttonClasses.endIcon}`]: {
-        ...styles.endIcon,
-        ...styles[`iconSize${capitalize(styleProps.size)}`],
-      },
+  return ({
+    [`& .${buttonClasses.label}`]: styles.label,
+    [`& .${buttonClasses.startIcon}`]: {
+      ...styles.startIcon,
+      ...styles[`iconSize${capitalize(styleProps.size)}`],
     },
-    styles.root || {},
-  );
+    [`& .${buttonClasses.endIcon}`]: {
+      ...styles.endIcon,
+      ...styles[`iconSize${capitalize(styleProps.size)}`],
+    },
+    ...styles.root,
+    ...styles[styleProps.variant],
+    ...styles[`${styleProps.variant}${capitalize(styleProps.color)}`],
+    ...styles[`size${capitalize(styleProps.size)}`],
+    ...styles[`${styleProps.variant}Size${capitalize(styleProps.size)}`],
+    ...(styleProps.color === 'inherit' && styles.colorInherit),
+    ...(styleProps.disableElevation && styles.disableElevation),
+    ...(styleProps.fullWidth && styles.fullWidth),
+  });
 };

[mnajdova]

Agree, I’ll handle it next week

[mnajdova]

Another potential solution to this problem is to define the styles for the slots independently as done in #25809 not sure what the cost of that would be. cc @siriwatknp

const TablePaginationToolbar = experimentalStyled(
  Toolbar,
  {},
  {
    name: 'MuiTablePagination',
    slot: 'Toolbar',
    overridesResolver: (props, styles) => styles.toolbar,
  },
)(({ theme }) => ({
  minHeight: 52,
  paddingRight: 2,
  [`${theme.breakpoints.up('xs')} and (orientation: landscape)`]: {
    minHeight: 52,
  },
  [theme.breakpoints.up('sm')]: {
    minHeight: 52,
    paddingRight: 2,
  },
  [`& .${tablePaginationClasses.actions}`]: {
    flexShrink: 0,
    marginLeft: 20,
  },
}));