mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.96k stars 32.27k forks source link

[material-ui][Button] Variants don't consume the size prop #32427

Open cheesestringer opened 2 years ago

cheesestringer commented 2 years ago

Duplicates

Latest version

Current behavior 😯

Creating a button variant doesn't seem to use the size prop. When a button and a variant button are placed next to each other they look misaligned.

export const components = {
  MuiButton: {
    defaultProps: {
      variant: 'contained'
    },
    variants: [
      {
        props: { variant: 'action' },
        style: {
          background: 'red'
        }
      }
    ]
  }
} as Components;
<div>
  <Button color="primary" size="large">
    Normal Button
  </Button>
  <Button color="secondary" size="large" variant="action">
    Variant Button
  </Button>
</div>

image

Expected behavior 🤔

I assumed size would be consumed so buttons and their variants could still be grouped together to have the same font sizes and padding etc.

Steps to reproduce 🕹

Steps:

  1. Create a button variant
  2. Try and use different sizes on the variant while comparing font size and padding to the normal button

Context 🔦

Create a button variant with different styles while still being able to consume the existing props like size. I'd like to be able to position the buttons next to each other and have them using the same font sizes and padding without looking misaligned.

Your environment 🌎

`npx @mui/envinfo` System: OS: Windows 10 10.0.19044 Binaries: Node: 16.13.2 - C:\Program Files\nodejs\node.EXE Yarn: 1.22.15 - C:\Program Files\nodejs\yarn.CMD npm: 8.1.2 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: Not Found Edge: Spartan (44.19041.1266.0), Chromium (100.0.1185.44) npmPackages: @emotion/react: ^11.9.0 => 11.9.0 @emotion/styled: ^11.8.1 => 11.8.1 @mui/base: 5.0.0-alpha.77 @mui/material: ^5.6.2 => 5.6.2 @mui/private-theming: 5.6.2 @mui/styled-engine: 5.6.1 @mui/system: 5.6.2 @mui/types: 7.1.3 @mui/utils: 5.6.1 @types/react: ^18.0.6 => 18.0.6 react: ^18.0.0 => 18.0.0 react-dom: ^18.0.0 => 18.0.0 typescript: ^4.6.3 => 4.6.3
ZeeshanTamboli commented 2 years ago

Hi @cheesestringer ,

I assumed size would be consumed so buttons and their variants could still be grouped together to have the same font sizes and padding etc.

I don't think this is possible with the variants key in the theme. Creating new component variants means you will have to define completely new styles to that component when that specific variant prop value is applied. Only the default styles are applied. That means for custom size: large with variant: action, you will have to separately define the padding and fontsize styles you need.

It will not pick the Button's size: large styles. It only applies the default styles.

For more details see the docs: https://mui.com/material-ui/customization/theme-components/#creating-new-component-variants

As you can see for Large buttons, if you inspect, the padding applied are the default values 6px 16px, not of size: large having padding: '8px 11px'.

As for your use case, if you want to just have red background, you can instead of introducing new variant: action, simply apply it with the sx prop:

<Button color="secondary" size="large" sx={{ background: 'red' }}>
   ABC
</Button>
cheesestringer commented 2 years ago

Thanks for the response @ZeeshanTamboli! The red background was just a small repro to make the issue more clear. I wanted to use a variant so it could be easily reusable.

It would be awesome if variants could use the same color and size props of the base component if the variant doesn't specify them. It seems a bit weird that I can specify size=large but nothing is actually happening. Maybe the user should see an error if it's up to them to define the missing sizes? Only found this issue accidentally when I was comparing it to another button inline.

Just to clarify in case I'm doing something wrong:

ZeeshanTamboli commented 2 years ago

Are variants the recommended approach when creating reusable components with small styling changes?

We recommend to use the sx prop for small styling changes. This API is used to apply one-off style.

Is there an easy way to access the small, medium, large sizes from the base button so I can apply them to the variant?

If having new variant introduced via variants key in theme, you will have to define the styles for different Button sizes.

cheesestringer commented 2 years ago

Using the dashed button example as the variant, it seems weird that by extending the variants and adding a new one that it doesn't get to consume the existing font size. Is it just an accident that the existing variants all have the same font size?

image

I can add the font size so it matches the existing variants, but then if the font size of the built in variants changes in future updates it will get out of sync.

export const components = {
  MuiButton: {
    variants: [
      {
        props: { variant: 'dashed' },
        style: {
          border: '1px dashed purple'
        }
      },
      {
        props: { variant: 'dashed', size: 'large' },
        style: ({ theme }) => ({
          border: '1px dashed purple',
          fontSize: theme.typography.pxToRem(15)
        })
      }
    ]
  }
} as Components;

I guess I should use a wrapper component to automatically consume updated sizes in the future? In what situation would a variant be used? For the dashed button example it looks like a component that doesn't conform to the existing contract.

ZeeshanTamboli commented 2 years ago

@cheesestringer

Now looking at your code, it can simple be done as follows.

<Button size="large" sx={{ border: '1px dashed purple' }}>
    Variant
</Button>

No need to have a new "dashed" variant if you want to apply it to one button.

I can add the font size so it matches the existing variants, but then if the font size of the built in variants changes in future updates it will get out of sync.

Yes, so if you want to just have dashed border buttons with size=large applied, you can use the sx prop (shown above) or override style globally (for multiple dashed buttons).

cheesestringer commented 2 years ago

Is it an accident that all the current button variants have the same font size? It was throwing me that I could create a variant that doesn't conform to the existing font sizes unless I add it myself. If those font sizes change in the future I'd have to look out for that change to also consume it.

No need to have a new "dashed" variant if you want to apply it to one button.

Was using the demo from the docs, the goal is a reusable component.

or override style globally (for multiple dashed buttons).

Do you have a small demo of what that might look like?

siriwatknp commented 2 years ago

@ZeeshanTamboli was right from the implementation perspective:

// Button.js
// ...button styles
    ...(ownerState.size === 'small' &&
      ownerState.variant === 'text' && {
        padding: '4px 5px',
        fontSize: theme.typography.pxToRem(13),
      }),
    ...(ownerState.size === 'large' &&
      ownerState.variant === 'text' && {
        padding: '8px 11px',
        fontSize: theme.typography.pxToRem(15),
      }),

The variant and size are combined to create a style. However, from DX perspective, at first I thought what @cheesestringer tried to do should work.

For me, sizes and colors should be independent. That would make customization a lot more intuitive and easy. Anyway, I don't suggest doing anything at this point but it is something that we should consider for v6.

MonstraG commented 1 year ago

We have several (like about 6) custom variants, all of them are used several times, and all of them consist of only changing colors.

We've decided to make it a variant as that would make it extremely easy to write - <Button variant="gray" />, although we would prefer to do <Button color="gray"/> as that would make more sense, but that did not work.

Suggested

<Button color="secondary" size="large" sx={{ background: 'red' }}>
   ABC
</Button>

doesn't quite work as you also need different colors for hover/active/disabled states (and contrasting but different colors for text), so the variants aren't a 1-line affair.

We've discovered that sizes do not work, so we had to do this:

const createSizedButtonVariants = (props: Record<string, string>, style: Record<string, any>) => {
    return ["small", "medium", "large"].map((size) => ({
        props: { ...props, size: size },
        style: {
            ...style,
            ...(size === "small" && {
                padding: "4px 5px",
                fontSize: theme.typography.pxToRem(13)
            }),
            ...(size === "large" && {
                padding: "8px 11px",
                fontSize: theme.typography.pxToRem(15)
            })
        }
    }));
};

to still be able to do sizes.

genepaul commented 8 hours ago

@ZeeshanTamboli, @mnajdova, is there any plan to get this fixed? I see the PR to fix it has been open since June, and doesn't actually seem to do much to fix the issue. This makes it very difficult to use MUI for a custom design system where we might only want to use one size of buttons while providing our own custom variants. Is there any workaround to this other than just reapplying all the sizing styles to our variants? I was hoping I could set defaultProps.size to small, then just customize the colors for my custom variants.