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

[system] Support new props with variants #19466

Open
galechus opened this issue Jan 29, 2020 · 4 comments
Open

[system] Support new props with variants #19466

galechus opened this issue Jan 29, 2020 · 4 comments
Labels
new feature New feature or request package: system Specific to @mui/system waiting for 👍 Waiting for upvotes

Comments

@galechus
Copy link
Contributor

galechus commented Jan 29, 2020

Hi,
is this possible extend PaperClassKey and PaperProps with custom values?

I would like to get something like this:

export const theme = createMuiTheme({
    overrides: {
        MuiPaper: {
            dark: {
                backgroundColor: COLORS.BLACK,
            },
            light: {
                backgroundColor: COLORS.WHITE,
            },
        },
    },
});

And using the component would look like this:

<Paper filled="dark" />
@eps1lon eps1lon added component: Paper This is the name of the generic UI component, not the React module! new feature New feature or request labels Jan 29, 2020
@oliviertassinari oliviertassinari changed the title Extend PaperClassKey and PaperProps [theme] Support new props with variants Apr 17, 2021
@oliviertassinari oliviertassinari added design This is about UI or UX design, please involve a designer and removed component: Paper This is the name of the generic UI component, not the React module! labels Apr 17, 2021
@oliviertassinari
Copy link
Member

@galechus We have worked on this problem in v5 with #15573. We have solved the problem for existing props as well as for JavaScript with new props:

import * as React from "react";
import { ThemeProvider, createMuiTheme } from "@material-ui/core/styles";
import Paper from "@material-ui/core/Paper";

const theme = createMuiTheme({
  components: {
    MuiPaper: {
      variants: [
        {
          props: { filled: "dark" },
          style: {
            backgroundColor: "#000",
            color: "#fff"
          }
        }
      ]
    }
  }
});

export default function Demo() {
  return (
    <ThemeProvider theme={theme}>
      <Paper filled="dark">filled="dark"</Paper>
    </ThemeProvider>
  );
}

Screenshot 2021-04-18 at 00 22 08

https://codesandbox.io/s/continuousslider-material-demo-forked-nxq1s?file=/demo.tsx:0-562

However, it doesn't work with TypeScript:

Screenshot 2021-04-18 at 00 39 54

For benchmark, I could get it working with Stiches: https://codesandbox.io/s/stitches-demo-with-stitches-reset-forked-yedfi?file=/src/App.tsx, well done @peduarte.

import * as React from "react";
import { styled } from "@stitches/react";

const Paper = styled("div", {
  variants: {
    filled: {
      dark: {
        backgroundColor: "#000",
        color: "#fff"
      }
    }
  }
});

export default function App() {
  return <Paper filled="dark">hello</Paper>;
}

Regarding the next steps, I think that:

  • We can use this issue to keep track of our progress on the support of new props, as well as collect developers' interest.
  • Maybe it would make sense to add in the styled() method the support for a variants key with the same API, as in the theme. This will make it easier for developers to switch between the two. Not sure, we can wait for requests. I have shared the API Stiches above, Chakra has something similar.

cc @mnajdova

@shripadk
Copy link

shripadk commented Jul 19, 2021

@oliviertassinari I stumbled upon this issue as well. Could something like this be possible?

Assuming I want to add a new prop to say the Avatar component: prop is "size" with values "small", "medium" and "large". I can do something like this to get rid off the first error:

import type {
  AvatarProps,
} from "@material-ui/core/Avatar"

// Create new sizes for Avatar
declare module "@material-ui/core/Avatar" {
  interface AvatarProps {
    size: "small" | "medium" | "large"
  }
}

Even though it works in the theme overrides, it still throws an missing prop Error while actually using the component. In this case, it would seem prudent to also provide a way to override Props in the same vein as Variant overrides. Something like this: AvatarPropsOverrides. So in the Avatar example, the type definitions would be:

export interface AvatarPropsVariantOverrides {}

export interface AvatarPropsOverrides {}

export interface AvatarTypeMap<P = {}, D extends React.ElementType = 'div'> {
  props: P & {
    ...
  } & AvatarPropsOverrides;
  defaultComponent: D;
}

Then the type module augmentation would be:

import type {
  AvatarPropsOverrides,
} from "@material-ui/core/Avatar"

// Create new sizes for Avatar
declare module "@material-ui/core/Avatar" {
  interface AvatarPropsOverrides {
    size?: "small" | "medium" | "large"
  }
}

This should solve the issue IMHO. Would be happy to send a pull request if this is acceptable.

@mnajdova
Copy link
Member

@shripadk although this looks good, there are other aspects that we need to solve before this is possible.

One I could think of immediately is, how will the component know which props to forward to the root element? Currently, all unhandled props are passed, now we will need to have a more complex solution here, for example forward only valid HTML props.

@oliviertassinari oliviertassinari changed the title [theme] Support new props with variants [system] Support new props with variants Jul 20, 2021
@oliviertassinari oliviertassinari added package: system Specific to @mui/system waiting for 👍 Waiting for upvotes and removed design This is about UI or UX design, please involve a designer labels Jul 20, 2021
@shripadk
Copy link

@mnajdova I did not realise that custom props are being passed down to the native DOM element. I thought the default behaviour was not to pass down any prop unless it is whitelisted in the implementation of the component. So I assumed that custom props were utilized only for generating styles for the component. Thanks for providing clarity on that. Yes I agree with you that it does require a more complex solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: system Specific to @mui/system waiting for 👍 Waiting for upvotes
Projects
None yet
Development

No branches or pull requests

5 participants