[Button] Custom variant

[mnajdova]

This PR is a proposal for solving the problem of adding custom variants to the components, defined inside the theme. See #15573 for more details.

Currently users can do this by creating wrapper components:

import React from "react";
import { makeStyles } from "@material-ui/core/styles";
import { deepmerge } from "@material-ui/utils";
import MuiButton, {
  ButtonProps as MuiButtonProps
} from "@material-ui/core/Button";

type ButtonProps = Omit<MuiButtonProps, "variant"> & {
  variant: "text" | "outlined" | "contained" | "dashed";
};

const useStyles = makeStyles(theme => ({
  root: ({ variant }: ButtonProps) => ({
    ...(variant === "dashed" && {
      border: "2px dashed grey"
    })
  })
}));

const Button: React.FC<ButtonProps> = props => {
  const variantClassses = useStyles(props);
  const { classes: propsClasses, variant: propsVariant, ...rest } = props;

  const classes = deepmerge(variantClassses, propsClasses);
  const variant = propsVariant !== "dashed" ? propsVariant : undefined;

  return <MuiButton classes={classes} variant={variant} {...rest} />;
};

export default function App() {
  return (
    <Button variant="dashed" color="secondary">
      Custom variant
    </Button>
  );
}

From @oliviertassinari #21648 (review)

The purpose of this PR is to remove the effort and what I believe is the underlying pain point from our users. It's about not having to create wrapper components. The issues I see with the wrapper component direction:

• For every new wrapper component you create, you have to put constraints in place in the codebase to make sure your organization will discover it and use it.
  You have to be very strict about it. I have never found a good solution for it.
  When working at @doctolib, we had many components for the same thing (I had seemed 5 slightly different switches build in the matter of a year). When working at @onepixel, we were mixing wrapped components and unwrapped.
  We were losing track of which one to use and when. We didn't invest in an eslint rule.
• Wrapper components don't allow to swap different themes like it used to be the case in the past with CSS based approach. Older libraries like datatables.net handles the multi-theming issue by enforcing the same class names (equivalent to component for React).

I've looked into several API, and decided to go with the following one, mainly for flexibility on defining the props for a combination of props, and having type safety.

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

declare module '@material-ui/core/Button/Button' {
  interface ButtonPropsVariantOverrides {
    dashed: true;
  }
}

const theme = createMuiTheme({
  variants: {
    MuiButton: [
      {
        props: { variant: 'dashed' }, // for what combination of props the styles will be applied
        styles: {
          padding: '5px 15px',
          border: '5px dashed grey',
        },
      },
    ],
  },
});

export default function App() {
  return (
    <ThemeProvider theme={theme}>
      <Button variant="dashed" color="secondary">
        Custom variant
      </Button>
    <ThemeProvider>
  );
}

Benchmarks

Here is a list of ideas on how other frameworks solve similar issues:

1. Bootstrap - the variant prop is just a string, and the value is used for generating a className (client's are responsible for associating styles with that className as far as I understood)
2. styled-system - they allow clients so specify variants + props which will be used, but they are creating new component - in their case they are always creating a new component so this is not something different - probably we can allow something similar with some factory function, but ideally I would like to avoid creating new components. https://styled-system.com/variants
3. chakra ui - don't have custom variants, plan to add it in next major release https://github.com/chakra-ui/chakra-ui/blob/a828a0dbd451729fcb3d94c006af3cf338342d62/packages/variant/src/variant.ts#L54 https://github.com/chakra-ui/chakra-ui/blob/ea75ca1086f5dfb12607ccb56a462763bb941cb9/packages/system/src/component/get-modifier-style.ts#L103
4. antd - they use the type prop, based on which they generate a className which will be applied on the element, in similar way as Bootstrap - haven't seen API for adding the styles associated with the variant
5. react-ui - https://react-ui.dev/components/Element#variant.
6. Modulz - https://www.kickstarter.com/projects/stephenhaney/modulzthe-next-step-in-visual-coding

[mui-pr-bot]

@material-ui/core: parsed: +0.30% , gzip: +0.29%
@material-ui/lab: parsed: +0.48% , gzip: +0.67%
@material-ui/styles: parsed: +2.50% , gzip: +2.72%

Details of bundle changes

Generated by 🚫 dangerJS against 704e2f2

[oliviertassinari]

A quick initial review

• How will the developers know which prop they can extend in the theme?
• Do you have a list of prior-art on this problem? I think that it would be really great to check out how other UI libraries have attempts to solve the problem in the past. I wanted to put a list together but I'm running out of time, time for a 🏃‍♂️

[mnajdova]

How will the developers know which prop they can extend in the theme?

My initial thought was that we should provide a way for clients to add custom variants/colors, for which they can use the classes key for defining the additional styles

Do you have a list of prior-art on this problem? I think that it would be really great to check out how other UI libraries have attempts to solve the problem in the past. I wanted to put a list together but I'm running out of time, time for a 🏃‍♂️

Updated the PR description with my first round of bench marking 👍

[oliviertassinari]

I gave this problem more thought. It feels that if we want to have style engine independent, we will need to abstract away from the prop to class name mapping logic. A wild idea:

theme.variants = {
  MuiButton: [
    {
      prop: 'variant',
      trigger: 'red',
      styles: `
        color: red;
      `
    },
    {
      prop: 'size',
      trigger: 'xxsmall',
      styles: `
        color: red;
      `
    },
    {
      prop: 'toggle',
      trigger: true,
      styles: `
        color: red;
      `
    },
    {
      prop: 'toggle',
      trigger: (props, state) => props.toggle === 'red',
      styles: `
        color: red;
      `
    }
  ],
}

[mnajdova]

I like this idea, although if we have a trigger I would argue why we need the prop.. Maybe a slightly better structure can be prop + value, or just trigger. Although, if we can define anything with the trigger fn I am not sure why we need to expose different APIs... So maybe something like this:

theme.variants = {
  MuiButton: [
    {
      trigger: (props, state) => props.variant === 'red',
      styles: `
        color: red;
      `
    },
    {
      trigger: (props, state) => props.size === 'xxsmall',
      styles: `
        color: red;
      `
    },
    {
      trigger: (props, state) => props.toggle,
      styles: `
        color: red;
      `
    },
    {
      trigger: (props, state) => props.toggle === 'red',
      styles: `
        color: red;
      `
    }
  ],
}

[oliviertassinari]

@mnajdova To be honest, I was making a wild guess. A couple of limitations I see with my previous exploration:

• Should we be worried about the runtime cost of calling each of these functions are each render to know if the style should be applied?
• Is the API intuitive, do we have alternatives? For instance, do we even need to have a function?
• Would this structure be easy to statically analyze and automatically generate documentation for the different states? (for a future tool)
• What about TypeScript safety?
• What about styling for a combination of two props, like variant, and size?

if we have a trigger I would argue why we need the prop key

The idea was to use it to avoid forwarding extra props into the DOM nodes, but maybe is-prop-valid could serve the same purpose.

[mnajdova]

@oliviertassinari some thoughts

Should we be worried about the runtime cost of calling each of these functions are each render to know if the style should be applied?

I believe we will need some custom caching if we decide to go with this approach. As far as I know, for example emotion is pretty good with caching this kind of definitions.

Is the API intuitive, do we have alternatives? For instance, do we even need to have a function?

I would propose maybe we should have only function so that there is one way to do it.. On the other hand, allowing users to use only classes key is better in my opinion as it is more restrictive

Would this structure be easy to statically analyze and automatically generate documentation for the different states? (for a future tool)

This may be hard...

What about TypeScript safety?

If we have typings for the names, it should be doable..

What about styling for a combination of two props, like variant, and size?

This shouldn't be problem with the function

is-prop-valid is a good idea 👍 , we should see the perf on it as well

Anyway I would like to try more different approaches, and have them all on the PR, so we can eventually pick one of them...

[mnajdova]

Slightly different idea. How about instead of trigger, we allow matcher, something like:

theme.variants = {
  MuiButton: [
    {
      matcher: { variant: 'red' },
      styles: `
        color: red;
      `
    },
    {
      matcher: { size: 'xxsmall' },
      styles: `
        color: red;
      `
    },
    {
      matcher: { toggle: true },
      styles: `
        color: red;
      `
    },
    {
      matcher: { toggle: true, variant: 'red' },
      styles: `
        color: red;
      `
    }
  ],
}

This should solve the issues we had with the previous proposal, and it still more user friendly than just specifying the class key directly, as the initial proposal with the additions.

[oliviertassinari]

Considering this failing test case:

    it.only('should map the variant classkey to the component', function test() {
      if (/jsdom/.test(window.navigator.userAgent)) {
        // see https://github.com/jsdom/jsdom/issues/2953
        this.skip();
      }

      const theme = createMuiTheme({
        variants: {
          MuiButton: [
            {
              props: { variant: 'test', color: 'primary', size: 'large' },
              styles: { backgroundColor: 'rgb(255, 0, 0)' },
            },
          ],
        },
      });

      render(<WrappedComponent theme={theme} variant="test" size="large" />);

      const style = window.getComputedStyle(screen.getByTestId('component'));
      expect(style.getPropertyValue('background-color')).to.equal('rgb(255, 0, 0)');
    });

Should we move the logic outside of withStyles, after the resolution of the default props?

[eps1lon]

string shouldn't be allowed by default. Will take a look later. Nothing that holds back this PR since it would also affect breakpoints.

[alli5623]

Content deleted

[eps1lon]

This is very brittle to maintain. Do we have any automated test that would verify that a newly added prop would be passed to useThemeVariants with its default value?

Not important for this PR. I already have an idea for a lint rule and since it's a hook it should be pretty straight forward to track.

[mnajdova]

Lint rule would be perfect in my opinion 👍 There isn't automated tests for this, but I am open for ideas on this one. Although if we have lint rules, won't be necessary..

[eps1lon]

Yeah lint rule looks promising.

[eps1lon]

https://astexplorer.net/#/gist/d094e92b26f3c9217cd78fa62cbd7306/02a10fbd4af430267ab603c7400303b8cda068dc should cover most cases. Should resolve #21648 (comment) first before we finalize the lint rule.

[oliviertassinari]

@mnajdova Great job 🙏!