[Button] Remove default color, use primary by default #17530

wysird · 2019-09-22T17:51:01Z

It'd be useful to be able to override the 'default' palette value and it actually take effect.

e.g.

const theme = createMuiTheme({
    palette: {
        ...
        default: {
            dark: '#000000',
            light: '#111111',
            main: '#100008',
            contrastText: '#ffffff',
        },
        ...
    },
    typography: {
        useNextVariants: true
    },
});

At the moment some components (like Button) use specific colours (like theme.palette.grey[300]) instead of theme.palette.default. Switching this round would allow us to make use of the default them in the palette.

e.g.

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 4e1304ff1..cbfef3994 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -107,8 +107,8 @@ export const styles = theme => ({
   },
   /* Styles applied to the root element if `variant="contained"`. */
   contained: {
-    color: theme.palette.getContrastText(theme.palette.grey[300]),
-    backgroundColor: theme.palette.grey[300],
+    color: theme.palette.default.contrastText,
+    backgroundColor: theme.palette.default.main,
     boxShadow: theme.shadows[2],
     '&$focusVisible': {
       boxShadow: theme.shadows[6],
@@ -122,10 +122,10 @@ export const styles = theme => ({
       backgroundColor: theme.palette.action.disabledBackground,
     },
     '&:hover': {
-      backgroundColor: theme.palette.grey.A100,
+      backgroundColor: theme.palette.default.dark,
       // Reset on touch devices, it doesn't add specificity
       '@media (hover: none)': {
-        backgroundColor: theme.palette.grey[300],
+        backgroundColor: theme.palette.default.main,
       },
       '&$disabled': {
         backgroundColor: theme.palette.action.disabledBackground,

The text was updated successfully, but these errors were encountered:

mbrookes · 2019-09-22T21:53:33Z

The reason that the components use grey from the theme, rather than from core/colors (and the reason grey is in the theme) is for just this reason - so that you can override it.

Perhaps grey isn't the best name (what if you want to turn grey into pink?) but it at least describes the default.

wysird · 2019-09-22T23:46:18Z

I understand that you can 'override' the default value, however as 'default' is part of the Enum-type definitions, I thought it might be more useful to instead define it in the palette.

mbrookes · 2019-09-23T13:39:05Z

It is already defined in the palette – theme.palette.grey.

wysird · 2019-09-23T19:43:00Z

I see, so you suggest that I remap grey to be the "default", replacing 'default' with 'grey' in the example I gave?

mbrookes · 2019-09-23T23:38:39Z

I'm suggesting that if you want to redefine the color of components that use grey from the theme, you can.

CoericK · 2020-05-02T04:48:33Z

+1 Im also facing the same issue, @wysird, I love this library, but for instance, I have a simple UI where i want to display a simple button but using theme colors:

<Button variant="contained">Button contained</Button>

To look like this:

In order to it would be nice to have something like :

const theme = createMuiTheme({
  palette: {
    default: {
      main: blue[500],
      light: blue[500],
      dark: blue[500],
      contrastText: "#ffffff",
    },
  },
});

But instead we have to do this:

<Button variant="contained" color="primary">Button contained primary</Button>

const theme = createMuiTheme({
  palette: {
    primary: {
      main: blue[500],
      light: blue[500],
      dark: blue[500],
      contrastText: "#ffffff",
    },
  },
});

And finally if you want to override the default button you would need to override with:

{
overrides: {
 MuiButton:{
contained:{
//...do override of colors and background here.
}
}
}
}

CoericK · 2020-05-02T04:49:57Z

@mbrookes how did you workaround this?

wysird · 2020-05-02T12:39:33Z

@CoericK so according to @mbrookes you're supposed to do something like this:

const theme = createMuiTheme({
  palette: {
    grey: {
      main: blue[500],
      light: blue[500],
      dark: blue[500],
      contrastText: "#ffffff",
    },
  },
});

wysird · 2020-05-30T11:07:37Z

@CoericK I do totally agree though, I feel like overriding the MuiButtons (and everything else) is not really right, given that you refer to theme.palette.primary and theme.palette.secondary (as you pass in the value to the color="primary"/color="secondary", respectively).

For this reason, it would still make sense to match color="default" with:

palette: {
  default: {
    main: blue[500],
    light: blue[500],
    dark: blue[500],
    contrastText: '#ffffff',
  },
},

Either that, or if (as I suggested) the name provided to color changes from color="default" to color="grey", to match up with:

palette: {
    grey: {
      main: blue[500],
      light: blue[500],
      dark: blue[500],
      contrastText: "#ffffff",
  },
},

Thoughts, @mbrookes?

oliviertassinari · 2020-05-30T19:00:51Z

What about we drop this default color in v5?

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 7a780777a..bf1c7c19c 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -264,7 +264,7 @@ const Button = React.forwardRef(function Button(props, ref) {
     children,
     classes,
     className,
-    color = 'default',
+    color = 'primary',
     component = 'button',
     disabled = false,
     disableElevation = false,
@@ -346,7 +346,7 @@ Button.propTypes = {
   /**
    * The color of the component. It supports those theme colors that make sense for this component.
    */
-  color: PropTypes.oneOf(['default', 'inherit', 'primary', 'secondary']),
+  color: PropTypes.oneOf(['inherit', 'primary', 'secondary']),
   /**
    * The component used for the root node.
    * Either a string to use a HTML element or a component.

Pros:

The valuedefault can feel inconsistent: [RFC] color prop API #13028 by @eps1lon. This issue.
I think that it was added for an older version of the specification. There is no more mention to is anymore. Also, having looked at more design systems, it seems that we are an exception.
Do people even use this default value (without overrides)? Personally, I have avoided as much as possible, it looks strange.

wysird · 2020-06-07T18:14:33Z

What about we drop this default color in v5?
diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 7a780777a..bf1c7c19c 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -264,7 +264,7 @@ const Button = React.forwardRef(function Button(props, ref) {
     children,
     classes,
     className,
-    color = 'default',
+    color = 'primary',
     component = 'button',
     disabled = false,
     disableElevation = false,
@@ -346,7 +346,7 @@ Button.propTypes = {
   /**
    * The color of the component. It supports those theme colors that make sense for this component.
    */
-  color: PropTypes.oneOf(['default', 'inherit', 'primary', 'secondary']),
+  color: PropTypes.oneOf(['inherit', 'primary', 'secondary']),
   /**
    * The component used for the root node.
    * Either a string to use a HTML element or a component.
Pros:

The valuedefault can feel inconsistent: [RFC] color prop API #13028 by @eps1lon. This issue.

I think that it was added for an older version of the specification. There is no more mention to is anymore. Also, having looked at more design systems, it seems that we are an exception.

Do people even use this default value (without overrides)? Personally, I have avoided as much as possible, it looks strange.

I think inheriting from above is probably a better idea; the real problem is when trying to style the colours to work with your new backgrounds and the default just doesn't change; it sticks with whatever it has. I'm totally behind just removing the default as a compromise to needing to override it.

wysird · 2020-06-08T16:36:02Z

Actually as an extension to what you suggested @oliviertassinari, it might be worth changing all 'root' styles (e.g. MuiButton-root, FormControl-root, etc) for the color variable to be inherit, as this is usually a better solution (meaning you can set the color at the top level in a single place, rather than overriding literally every MUI element.

oliviertassinari · 2020-06-08T17:11:29Z

@wysird I fear the full support of color="inherit" with CSS inheritance is not possible. How would you style the hover, focused, etc styles?

wysird · 2020-06-08T19:06:45Z

Sorry, I don't mean to use color="inherit" I mean the default styling provided by the theme provider should be setting the CSS style of color to inherit. At the moment there are values like: color: rgba(0, 0, 0, 0.54); which means that if you change (for instance) the background color to a dark color, anything that has this value will have to be overridden. So you overrides ends up looking like this:

{
  ...
  overrides: {
    MuiButton: {
      color: 'inherit',
    },
    MuiSvgIcon: {
      color: 'inherit',
    },
    MuiPaper: {
      color: 'inherit',
    },
  },
  ...
}

etc.

I'm suggesting that the default for all these is 'inherit', meaning there should not really be any reason to override, and makeStyles is utilised in situations that a change is required.

Does this make sense, or am I muddying the water?

oliviertassinari · 2020-06-09T12:41:12Z

@wysird The color CSS property inherits by default. We override it, sometimes, to use the values from the theme; it doesn't seem there is any change required here;

wysird · 2020-06-09T13:41:21Z

Oh yeah, my bad, I see what you mean.

mbrookes added the discussion label Sep 22, 2019

oliviertassinari added component: button This is the name of the generic UI component, not the React module! breaking change and removed discussion labels May 30, 2020

oliviertassinari added this to the v5 milestone May 30, 2020

oliviertassinari changed the title ~~Palette Default override~~ [Button] Remove default color, use primary by default May 30, 2020

oliviertassinari mentioned this issue Jun 5, 2020

[ToggleButton] Bring component closer to MUI Button #20919

Closed

1 task

wysird closed this as completed Jun 7, 2020

oliviertassinari reopened this Jun 7, 2020

mbrookes mentioned this issue Jun 27, 2020

[Button] Make primary the default color #21594

Merged

1 task

oliviertassinari closed this as completed in #21594 Jun 30, 2020

oliviertassinari mentioned this issue Dec 9, 2020

[Button] 'default' color not supported in v5 #23907

Closed

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Button] Remove default color, use primary by default #17530

[Button] Remove default color, use primary by default #17530

wysird commented Sep 22, 2019

mbrookes commented Sep 22, 2019

wysird commented Sep 22, 2019 •

edited

Loading

mbrookes commented Sep 23, 2019

wysird commented Sep 23, 2019

mbrookes commented Sep 23, 2019

CoericK commented May 2, 2020 •

edited

Loading

CoericK commented May 2, 2020

wysird commented May 2, 2020

wysird commented May 30, 2020

oliviertassinari commented May 30, 2020 •

edited

Loading

wysird commented Jun 7, 2020

wysird commented Jun 8, 2020

oliviertassinari commented Jun 8, 2020 •

edited

Loading

wysird commented Jun 8, 2020

oliviertassinari commented Jun 9, 2020

wysird commented Jun 9, 2020

This comment has been minimized.

[Button] Remove default color, use primary by default #17530

[Button] Remove default color, use primary by default #17530

Comments

wysird commented Sep 22, 2019

mbrookes commented Sep 22, 2019

wysird commented Sep 22, 2019 • edited Loading

mbrookes commented Sep 23, 2019

wysird commented Sep 23, 2019

mbrookes commented Sep 23, 2019

CoericK commented May 2, 2020 • edited Loading

CoericK commented May 2, 2020

wysird commented May 2, 2020

wysird commented May 30, 2020

oliviertassinari commented May 30, 2020 • edited Loading

wysird commented Jun 7, 2020

wysird commented Jun 8, 2020

oliviertassinari commented Jun 8, 2020 • edited Loading

wysird commented Jun 8, 2020

oliviertassinari commented Jun 9, 2020

wysird commented Jun 9, 2020

This comment has been minimized.

wysird commented Sep 22, 2019 •

edited

Loading

CoericK commented May 2, 2020 •

edited

Loading

oliviertassinari commented May 30, 2020 •

edited

Loading

oliviertassinari commented Jun 8, 2020 •

edited

Loading