Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Button | Typescript | Add TS overrides for top level button props (in addition to variant, color, and size) #28754

Open
1 task done
MikeRawding opened this issue Oct 1, 2021 · 12 comments
Labels
component: button This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@MikeRawding
Copy link

MikeRawding commented Oct 1, 2021

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

theme.components.MuiButton.variants allows you to apply styles based on matching props. This works with arbitrary props, but the TS is only set up to let you add values to variant, color, and size. I believe adding new props here should be valid.

Examples 🌈

https://codesandbox.io/s/mui-5-custom-button-props-sd9pp?file=/src/App.js

Motivation 🔦

My app uses a standard and rounded version of each MUI Button variant. In v4 this was accomplished with a RoundedButton component which wrapped Button. I've been been looking forward to dropping RoundedButton in favor of v5 custom variants.

I found that adding additional variants is not really what I needed. I would need to add contained-rounded, outlined-rounded, and text-rounded, then I would need to duplicate styles from each of the default variants. Using a rounded prop handles this very naturally without needing to duplicate styles or have a wrapping component.

const theme = createTheme({
  components: {
    MuiButton: {
      variants: [
        {
          props: { rounded: true }, // this works great! but there is no way to update the TS
          style: {
            borderRadius: 50
          }
        }
      ]
    }
  }
});

Button.d.ts already has ButtonPropsVariantOverrides, ButtonPropsColorOverrides, and ButtonPropsSizeOverrides. I'm proposing adding a top level ButtonPropsOverrides.

If you think this is a useful pattern, I'd be happy to make a PR updating the TS and adding an example to the docs.

@MikeRawding MikeRawding added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 1, 2021
@siriwatknp siriwatknp added new feature New feature or request component: button This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Oct 3, 2021
@siriwatknp
Copy link
Member

ButtonPropsOverrides is one way to do this but I think we need to take a step back and look at higher level because other components will use the same approach. One question is does rounded appear in the DOM? I guess you use it like this:

<Button rounded />

@MikeRawding
Copy link
Author

That's an excellent point! My codesandbox example is throwing a warning about just that, so I guess we would also need a way to specify additional props for Button.js to strip off before spreading to the DOM.

I really wasn't sure how the community would feel about this pattern. I don't know of anywhere else where MUI lets you add props like this. It just seems to fit my use case so naturally I thought it was worth discussion.

@klausbreyer
Copy link

klausbreyer commented Oct 6, 2021

We also came across this issue. Also I would like to point out: There is also no possibility to introduce custom variants/props for a lot of other components. We would like to have this also for ToggleButton(Group), Tab(s), etc.

Having a high Level Prop Override like ButtonPropsOverride for every component would be a good thing to have and would solve all of our current issues we are facing while implementing a complete styleguide in components.

@siriwatknp
Copy link
Member

siriwatknp commented Oct 7, 2021

The only way (at this moment) to extend props on a component is to wrap the component with styled api which I also think it is not a good experience.

// ButtonWithRounded.tsx
import { shouldForwardProp as baseShouldForwardProp } from '@mui/system';

const shouldForwardProp = prop => baseShouldForwardProp(prop) && prop !== 'rounded'; // to prevent the prop from spreading to DOM
const ButtonWithRounded = styled(Button, { shouldForwardProp })<{ rounded?: boolean }>(...)

export default ButtonWithRounded

and now you have to always import Button from this path instead of @mui/material/Button.


My proposed solution is to make props extendable at the global config level (cannot do it via theme), something like this:

import { NonForwardedProps } from '@mui/material';

// to not spread `rounded` prop to DOM
NonForwardedProps.set('Button', ['rounded']);

// to have typed safe, Button need to provide extra type that can be augmented
declare module '@mui/material/Button' {
  interface ButtonExtraProps {
    rounded?: boolean
  }
}

createTheme({
  components: {
    MuiButton: {
      variants: [
        {
          props: { rounded: true },
          style: {
            borderRadius: 50
          }
        }
      ]
    }
  }
})

Now, I can use the Button as usual.

import Button from '@mui/material/Button';

<Button rounded>Rounded</Button>

POC branch: siriwatknp@5d4a951

Screen Shot 2564-10-07 at 13 31 47

@MikeRawding
Copy link
Author

Thanks for your work on this. It looks like you tried to add nonForwardedProps to the theme first. What prevented that from working?

@klausbreyer
Copy link

Thanks a lot, that you work on this, @siriwatknp !!

What I do not understand: You defined rounded as a top level prop for Button in your module declaration. (if I am right)
But in createTheme Definition it is below variants:. Was this intentional?

Sidenote:
In general, a createTheme Definition with more flexible variants for every single component would something we are also looking for. This is something discussed here: #27267 and #27130 But I think this needs also other types of overridable Props for all of the components.

@duterte
Copy link

duterte commented Oct 23, 2021

rounded can be done with something like this `<Button sx={{ borderRadius: 50 }}>Rounded

lol

@JonKrone
Copy link
Contributor

JonKrone commented Dec 15, 2021

Just to add another voice of support here, my team would also love the addition of interfaces we could extend for adding custom props to any component through the variants theme configuration. The javascript is there, we just need type support. The discussion in these other issues also shows more support #19466, #27267, #27130

@javier-zabala-lenio
Copy link

javier-zabala-lenio commented Feb 12, 2022

Any update on this? It would be great if we could add custom props to the top level interface, this way we could just use overrides to customize all those components that don't need custom logic. We can do this already with a few components like the Select but most don't use this approach.

@siriwatknp
Copy link
Member

siriwatknp commented Feb 23, 2022

Another option I can think of is to leverage the ownerState because we have exposed the ownerState to the public in styleOverrides for theming.

Developers won't have to deal with forwarded props because ownerState is not passed to DOM by default.

What does it look like with this approach?

Developers have to extends the OwnerState interface of the component for adding new props

declare module '@mui/material/Button' {
  interface ButtonOwnerState {
    rounded?: boolean;
  }
}

<Button ownerState={{ rounded: true }} /> // ✅ typed-safe

// theming should also work with no afford
theme={{
  components: {
    MuiButton: {
      styleOverrides: {
        root: ({ ownerState }) => ({
          ...ownerState.rounded && {
            borderRadius: '20px',
          }
        })
      }
    }
  }
}}

What's needs to be done?

  • expose all components ownerState interface. eg. ButtonOwnerState, ToggleButtonOwnerState
  • all components must receive ownerState prop.

cc @oliviertassinari @mnajdova @michaldudak

@mnajdova
Copy link
Member

mnajdova commented Mar 3, 2022

@siriwatknp it could work, but I am not sure that is the way we should teach people to use going forward. If we want to support this feature we should make it official and allow adding new props as first class prop, make sure we whitelist which props are going on the DOM elements etc. From semantics point of view, ownerState is not a place where you add new styling tokens using the component prop.

@mr-FastWalker
Copy link

@siriwatknp are there any updates for this issue? (Add TS overrides)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants