mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

[theme] Allow custom color variants #13875

Closed carlgunderson closed 3 years ago

carlgunderson commented 5 years ago

Expected Behavior 🤔

The ability to have success in the color palette for Button, or any component w/ a color prop.

Current Behavior 😯

Only error palette option is available.

Context 🔦

Similar to the need for error styling, a success color palette option for any component that has a color prop would be equally valuable. The current functionality to get a green button option, for instance, requires lots of custom styling on the Button component which isn't ideal. If error is warranted in the palette, why not success as well?

Another idea is for some kind of dynamic mapping, so if you created a manual key of success and passed it in as a color prop to a Button, the button would just try to find the theme override with that key in the palette.

Thanks!

oliviertassinari commented 5 years ago

Vuetify has 4 different colors (success, danger, warning, info) by default on top of the primary and secondary colors in the theme: https://vuetifyjs.com/en/components/buttons#usage. Exactly like Bootstrap. I see some value in that. We should at the minimum make it easy for anybody to have them.

Another idea is for some kind of dynamic mapping

Some effort in this direction: https://deploy-preview-13632--material-ui.netlify.com/system/basics/.

@mui-org/core-contributors thoughts?

mbrookes commented 5 years ago

Where are we with support of style values from props (dynamic styles)? That would potentially allow any color to be passed in, either arbitrarily, or from the theme.

Users can easily add colour keys to the theme for these sorts of things in their own components, but not so easily in MUI components.

Otherwise, I have no objection to adding more colors. Even though the spec doesn’t explicitly provide for such color options, it’s a common UI pattern.

carlgunderson commented 5 years ago

Users can easily add colour keys to the time for these sorts of things.

Are you saying this is already possible with the current theme API? I tried creating a custom color palette object with a key of success but when I tried to access it from a UI component it was undefined.

oliviertassinari commented 5 years ago

@carlgunderson No, it's not possible the way you have in mind. But I think that it should, in an ideal world. Right now, you have to use a wrapper component.

We have some options:

  1. We support these 6 color variants by using static style rules.
  2. We rely on dynamic style rules.
  3. We use a mix of the static and dynamic style rules.

I would personally go with option 3.

katerlouis commented 4 years ago

what's the status here? I'd also like to do the following

<Button color="customPaletteColor">Awesome!</Button>
oliviertassinari commented 4 years ago

@katerlouis It works but you will get a prop-type warning. The next step is to, somehow, loosen the prop-types from a static check to a dynamic check.

katerlouis commented 4 years ago

this doesn't work for me:

// colors.js
export const blue = {
  main: "#00CDE8",
}

export const red = {
  main: "#FF495A",
  light: "#F76E7B",
}

export const purple = {
  main: "#5D2BFF",
}
...
// theme.js
...
palette: {
    primary: blue,
    secondary: purple,
    buy: blue,
    sell: red,
},
...
// SomeComponent.jsx
...
<Button color="sell">This is not red :'(</Button>

What am I doing wrong?

oliviertassinari commented 4 years ago

@katerlouis Ok, my bad, the current style structure doesn't allow it. We would need to perform the following diff:

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 958cb4d74..ad737d575 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -199,34 +199,20 @@ const Button = React.forwardRef(function Button(props, ref) {
     ...other
   } = props;

-  const text = variant === 'text';
-  const outlined = variant === 'outlined';
-  const contained = variant === 'contained';
-  const primary = color === 'primary';
-  const secondary = color === 'secondary';
-  const className = clsx(
-    classes.root,
-    {
-      [classes.text]: text,
-      [classes.textPrimary]: text && primary,
-      [classes.textSecondary]: text && secondary,
-      [classes.outlined]: outlined,
-      [classes.outlinedPrimary]: outlined && primary,
-      [classes.outlinedSecondary]: outlined && secondary,
-      [classes.contained]: contained,
-      [classes.containedPrimary]: contained && primary,
-      [classes.containedSecondary]: contained && secondary,
-      [classes[`size${capitalize(size)}`]]: size !== 'medium',
-      [classes.disabled]: disabled,
-      [classes.fullWidth]: fullWidth,
-      [classes.colorInherit]: color === 'inherit',
-    },
-    classNameProp,
-  );
-
   return (
     <ButtonBase
-      className={className}
+      className={clsx(
+        classes.root,
+        classes[variant],
+        {
+          [classes[`size${capitalize(size)}`]]: size !== 'medium',
+          [classes.disabled]: disabled,
+          [classes.fullWidth]: fullWidth,
+          [classes[`${variant}${capitalize(color)}`]]: color !== 'inherit',
+          [classes.colorInherit]: color === 'inherit',
+        },
+        classNameProp,
+      )}
       component={component}
       disabled={disabled}
       focusRipple={!disableFocusRipple}

From a bundle size perspective, it's already something worth doing :). Is this something you would like to contribute?

Then, you could do:

import React from 'react';
import { createMuiTheme } from '@material-ui/core/styles';
import { ThemeProvider } from '@material-ui/styles';
import Button from '@material-ui/core/Button';

const theme = createMuiTheme({
  overrides: {
    // Style sheet name ⚛️
    MuiButton: {
      // Name of the rule
      textSell: {
        color: 'red',
      },
    },
  },
});

function OverridesCss() {
  return (
    <ThemeProvider theme={theme}>
      <Button color="sell">Overrides CSS</Button>
    </ThemeProvider>
  );
}

export default OverridesCss;

We would need to move #6115 forward or #16180 first to support theme.palette.x directly.

katerlouis commented 4 years ago

NOW I'm confused :D

I'm new to material-ui so I remmeber seeing it, but don't remember exactly why you have to say MuiButton in overrides, when you want to overwrite the Button component; –

And then: why do you say textSell in the overrides, but in jsx u say color="sell"?

And theeen: why would you need to tell the button specifically what to do with the new color in overirdes? Right now the <Button> can react to color="secondary" without me telling it in overrides what it has to do with secondary.

The thing I'm (and I guess we all) asking for is, that once we assigned custom colors in the palette the button just grabs this palette and does with it exactly the same as with primary and secondary

oliviertassinari commented 4 years ago

The thing I'm (and I guess we all) asking for is, that once we assigned custom colors in the palette the button just grabs this palette and does with it exactly the same as with primary and secondary

@katerlouis Yes, I agree, this will be the resolution of the issue.

I was covering the partial solution we could so in the near future. text comes from the default variant. So you would get the same with outlinedSell and containedSell.

pkhodaveissi commented 4 years ago

any nice workarounds guys till v5?

sakulstra commented 4 years ago

We currently use a custom Button component which reexports Button with some style overwrites.

const useStyles = makeStyles(theme =>
  createStyles({
    text: props => {
      const color = theme.palette[props.color].main;
      return {
        color: color,
        backgroundColor: lighten(color, 0.935),
        "&:hover": {
          backgroundColor: fade(color, theme.palette.action.hoverOpacity),
          // Reset on touch devices, it doesn't add specificity
          "@media (hover: none)": {
            backgroundColor: "transparent"
          }
        }
      };
    }
  })
);

function Button(props) {
  const classes = useStyles(props);
  return <MuiButton classes={classes} {...props} />;
}

We do a lot of weird non spec-conform custom styling, which is why i stripped out most of the styles, but I hope this gives you an idea.

@oliviertassinari I'd love to contribute a spec-conform dynamic solution, but after looking at the codebase i'm not sure how to archive that. It seems like in the mui-codebase itself we use clsx and don't have access to props inside the styles. Would it be okay to use createStyles internally/what's the reason for not using it?

I guess with createStyles and access to props it would be relatively easy to: 1) allow custom palette colors 2) while reducing the amount of code I created a small (non-working poc) here: https://github.com/mui-org/material-ui/pull/17233/files but wasn't sure if it's worth keeping on, as I don't know if anything in that direction would be accepted.

oliviertassinari commented 4 years ago

@sakulstra The only reason why this is not supported yet it's because it's too slow to be used I the core components. At least, given how we are using JSS right now. It should be OK with styled-components, and could be OK with react-jss.

oliviertassinari commented 4 years ago

We can chat about it on gitter if you are interested.

sakulstra commented 4 years ago

@oliviertassinari i wrote you on spectrum - couldn't find any gitter link :thinking:

oliviertassinari commented 4 years ago

@sakulstra Avoid Spectrum, it's really slow. You don't need any link for gitter, you can chat with anyone on GitHub with gitter.

bradwray commented 4 years ago

any nice workarounds guys till v5?

My workaround is to name my accent color "error"... and tweak its hex code in the createMuiTheme... it feels wrong.

Luckily I can just bring whatever color I want in with the theme and not need to rely on the color prop.

clearwaterstream commented 4 years ago

Vuetify has 4 different colors (success, danger, warning, info) by default on top of the primary and secondary colors in the theme: https://vuetifyjs.com/en/components/buttons#usage. Exactly like Bootstrap. I see some value in that. We should at the minimum make it easy for anybody to have them.

Another idea is for some kind of dynamic mapping

Some effort in this direction: https://deploy-preview-13632--material-ui.netlify.com/system/basics/.

@mui-org/core-contributors thoughts?

For typical "forms" applications, info and warnings colors are needed. Good thing error is provided. For now i have a work around (not pretty), but it will do.

michaeldaineka commented 4 years ago

I made step by step but it doesn't seem to work

Representation: image

And another question: If I add custom containedSell value to Button component and add backgroundColor: 'red' to it, all owned classes will interact backgroundColor and will count the ripple RGBA color and so on? Or I have to add colors values (borders, ripples) with opacity manually?

bluefire2121 commented 4 years ago

While we wait for full customization to be added, can we go ahead and push success, info, and warning? Those 3 additions would help a ton.

oliviertassinari commented 4 years ago

@bluefire2121 agree, I think that it would be great.

danielbronder commented 4 years ago

I am really looking forward to the option to add custom colours, because overwriting the Button multiple times just to show a different colour is a little bit obnoxious. A great option would be to use the colours from '@material-ui/styles/colors'

When will the option be released for success, info and warning (this would be a great start)?

Alecell commented 4 years ago

So, is there any solution? The allow for success, info and etc was made? I mean, on the package I get that it exists, but the Typescript of the package dont allow to use anything beyond 'inherit' | 'primary' | 'secondary' | 'default'. I lost something or it isnt fixed yet?

AntoineKleinmannIbiza commented 4 years ago

I have the same error with TypeScript:

Type '"success"' is not assignable to type '"inherit" | "primary" | "secondary" | "default" | undefined'

When I look in the node_modules, the types don't reflect the annonced updates regarding the new color variants.

Does anyone know if this will be fixed soon / how to work around failing tests regarding the color prop?

iMakedonsky commented 4 years ago

How this still isn't released yet shocks me.

pskfry commented 4 years ago

to me, this use case is fundamental to any style library. looking forward to this addition

KendoJaaa commented 4 years ago

please

malyzeli commented 4 years ago

I made a simple utility which allows you to easily generate stylesheets for any color intention defined in your theme.

You just feed it with array of variants and colors you want to support, add stylesheet template and pass theme in.

It produces stylesheets which you can use as an input to makeStyles or withStyles and then you can apply generated classNames to your component as usual.

const variants = ["text", "contained"];
const colors = ["error", "warning"];

const useStyles = makeStyles(theme => ({
  ...createColorStyles(colors, variants, template, theme),
  // other stylesheets...
}));

/* output of createColorStyles */
const output = {
  textError: {
    color: "#b00020",
    "&:hover": {
      backgroundColor: "rgba(176, 0, 32, 0.04)",
    },
  },
  textWarning: {
    color: "#cc6600",
    "&:hover": {
      backgroundColor: "rgba(204, 102, 0, 0.04)",
    },
  },
  containedError: {
    color: "#fff",
    backgroundColor: "#b00020",
    "&:hover": {
      backgroundColor: "rgb(123, 0, 22)",
    },
  },
  containedWarning: {
    color: "#fff",
    backgroundColor: "#cc6600",
    "&:hover": {
      backgroundColor: "rgb(142, 71, 0)",
    },
  },
};

The only "hard" stuff is the template function, which transforms given variant, color, and theme into actual stylesheet. That's obviously the only part which you have to write from scratch. You can start with copying stylesheets from material component sources.

const template = (variant, color, theme) =>
  mergeAll([
    {
      color: theme.palette[color].main,
    },
    variant === "contained" && {
      color: theme.palette[color].contrastText,
      backgroundColor: theme.palette[color].main,
    },
    {
      "&:hover": mergeAll([
        {
          backgroundColor: fade(
            theme.palette[color].main,
            theme.palette.action.hoverOpacity
          ),
        },
        variant === "contained" && {
          backgroundColor: theme.palette[color].dark,
        },
      ]),
    },
  ]);

I'm using utils from ramda, but you can replace it with whatever functions doing the same job. You don't even need to use mergeAll in template function, but I find it convenient that way.

Here is the code with example usage and output - supporting all color intentions from default theme on the button component.

Enjoy!

image

oliviertassinari commented 4 years ago

@malyzeli Nice! If you could put it into a codesandbox, it would be awesome.

mehmetor commented 4 years ago

@malyzeli very nice! Thank you. Would you please put it in codesandbox?

CarlosOrozc commented 4 years ago

https://codesandbox.io/s/elated-hawking-6cz49?file=/src/App.js

malyzeli commented 4 years ago

@oliviertassinari @simetri8 sorry guys, never used CodeSandbox before and had priorities somewhere else recently, so I didn't want to spend much time with that...

Thanks @CarlosOrozc for setting it up!

I suggest creating your own wrapper component for those components (eg. Button) so you have consistent API and can use variant and color for all combinations (as opposed to @CarlosOrozc sandbox, where you have to use className for custom colors).

Here is such example from my code, with extracted Button component supporting all colors via default props API.

haluvibe commented 3 years ago

Guys, you're really missing the point. This NEEDS to be dynamic. NOT a set of arbitrary keywords!!!

dvzrd commented 3 years ago

Guys, you're really missing the point. This NEEDS to be dynamic. NOT a set of arbitrary keywords!!!

Do you mean something like a makePalette hook that lets you add custom props to the theme palette?

I think adding more than three props for primary and secondary colors would be a good start as it's probably easier to achieve - similar to how the grey palette has more than 3 variations using a number scale. I think this would give designers more options when choosing their color palette.

spmsupun commented 3 years ago

Any updates on this ?

lucasbasquerotto commented 3 years ago

I think the main thing that needs to be done is just make the PropTypes allow the additional values (or at least a way to disable it, because it's bad to develop and see lots of errors due to a wrong property name). Something must be done with Typescript too, but for now I made a workaround (for Typescript), like in the following case:

import { PropTypes } from '@material-ui/core';
import { grey, red } from '@material-ui/core/colors';
import { createMuiTheme, ThemeOptions } from '@material-ui/core/styles';

declare module '@material-ui/core/styles/createMuiTheme' {
    interface Theme {
        status: {
            danger: React.CSSProperties['color'];
        };
    }
    interface ThemeOptions {
        status?: {
            danger: React.CSSProperties['color'];
        };
    }
}

declare module '@material-ui/core/styles/createPalette' {
    interface Palette {
        neutral: Palette['primary'];
    }
    interface PaletteOptions {
        neutral?: PaletteOptions['primary'];
    }
}

export const createMyTheme = (options: ThemeOptions) =>
    createMuiTheme({
        ...options,
        palette: {
            neutral: {
                main: grey[200],
                light: grey[50],
                dark: grey[900],
            },
            ...options.palette,
        },
        status: {
            danger: red[900],
            ...options.status,
        },
    });

export const ThemeColor = {
    INHERIT: 'inherit' as PropTypes.Color,
    PRIMARY: 'primary' as PropTypes.Color,
    SECONDARY: 'secondary' as PropTypes.Color,
    DEFAULT: 'default' as PropTypes.Color,
    NEUTRAL: 'neutral' as PropTypes.Color,
};

Then I just use it like:

const Hello: FunctionComponent<{}> = () => (
    <>
        <Button variant='contained' color={ThemeColor.PRIMARY}>
            Hello World
        </Button>
        <Button variant='contained' color={ThemeColor.SECONDARY}>
            Hello World 2
        </Button>
        <Button variant='contained' color={ThemeColor.NEUTRAL}>
            Hello World 3
        </Button>
    </>
);

But I still get the PropType errors:

Warning: Failed prop type: Invalid prop color of value neutral supplied to ForwardRef(Button), expected one of ["default","inherit","primary","secondary"].

If I could disable the PropType validation it would already be fine for me (Typescript already catch most of such errors).

oliviertassinari commented 3 years ago

An update, the support for custom colors from the theme (theme.palette.x) technically works in v5.0.0-alpha.22, for instance:

import * as React from "react";
import Button from "@material-ui/core/Button";

export default function DisableElevation() {
  return (
    <Button variant="contained" color="error" disableElevation>
      Delete
    </Button>
  );
}

https://codesandbox.io/s/disableelevation-material-demo-forked-jlpk9?file=/package.json

Capture d’écran 2021-01-09 à 20 20 59

However, we still need to update the TypeScript types to allow module augmentation and to widen the propTypes (as we did for the variant prop):

Capture d’écran 2021-01-09 à 20 21 32 Capture d’écran 2021-01-09 à 20 22 19
pskfry commented 3 years ago

awesome news! looks great - can we help in any way?

oliviertassinari commented 3 years ago

@pskfry Thanks for proposing your help #24408 brings the full support for custom colors and sizes for the Button and Badge components. It can be used as a template for the other components:

If somebody wants to lead the effort and submit pull requests, help is welcome 🙌

jrhager84 commented 3 years ago

Is there a particular reason it hasn't been designed that you can add custom colors to the palette that are accessible through the color props? Something likepalette: { custom1: { main: #example } } where you can do <Button color="custom1">?

oliviertassinari commented 3 years ago

@jrhager84 This is working in v5.

jrhager84 commented 3 years ago

@jrhager84 This is working in v5.

Any timeline for stable release?

oliviertassinari commented 3 years ago

Any timeline for stable release?

@jrhager84 we aim for 2021, maybe beta in 6 months and stable in Q4.

mngu commented 3 years ago

Willing to try to update TextField :)

oliviertassinari commented 3 years ago

@mngu Note that the TextField is the most complex one to handle. I would recommend starting simpler, to gain more confidence and derisk the time invested.

mngu commented 3 years ago

@oliviertassinari Alright then, here's what I have so far => https://github.com/mngu/material-ui/compare/next...mngu:textfield-custom-props Custom color is working fine. But size has no effect for now... If it is what was expected, I can continue on this, or switch to another component as suggested.

oliviertassinari commented 3 years ago

@mngu It seems to go in the right direction. We should also likely update the Input, FilledInput, OutlinedInput were relevant.

oliviertassinari commented 3 years ago

While this issue is about making it simpler to add extra colors, in #24778, we talk about extending the default colors supported. For instance to make success available by default in the Button. We have also surfaced how we could maybe improve the types to automatically accept any values that are in the theme's palette.

mngu commented 3 years ago

We have also surfaced how we could maybe improve the types to automatically accept any values that are in the theme's palette.

Yeah, looks like it would be an easier way than augmenting each component individually.

oliviertassinari commented 3 years ago

@mngu if you want to contribute to this discussion, feel free to do it on the linked issue. For instance, how can it be technically done?