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

[material] Add variant prop type in all components #33510

Open
2 tasks done
emlai opened this issue Jul 14, 2022 · 13 comments
Open
2 tasks done

[material] Add variant prop type in all components #33510

emlai opened this issue Jul 14, 2022 · 13 comments
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process discussion new feature New feature or request package: material-ui Specific to @mui/material

Comments

@emlai
Copy link
Contributor

emlai commented Jul 14, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

Creating a List component variant causes TypeScript errors when using the variant prop in the variants declaration or at the component usage site.

Suppressing the TypeScript errors allows the variant to be declared and used as expected.

Screen Shot 2022-07-14 at 14 52 39

Expected behavior 🤔

Creating a List component variant is supported in TypeScript.

ListProps should have a declaration for variant, like we have in ButtonProps:

    /**
     * The variant to use.
     * @default 'text'
     */
    variant?: OverridableStringUnion<
      'text' | 'outlined' | 'contained',
      ButtonPropsVariantOverrides
    >;

Steps to reproduce 🕹

Steps:

  1. Create a component variant for List using TypeScript.

CodeSandbox

Context 🔦

We're maintaining a UI library built on top of MUI, and we want to provide a "bordered" List variant for our users. This is is blocking us from doing that.

Your environment 🌎

No response

@emlai emlai added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 14, 2022
@ZeeshanTamboli
Copy link
Member

The List component does not have a variant prop in it's API.

@ZeeshanTamboli ZeeshanTamboli added component: list 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 Jul 18, 2022
@ZeeshanTamboli ZeeshanTamboli closed this as not planned Won't fix, can't repro, duplicate, stale Jul 18, 2022
@ZeeshanTamboli ZeeshanTamboli added the status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it label Jul 18, 2022
@jussirantala
Copy link

jussirantala commented Jul 18, 2022

The List component does not have a variant prop in it's API.

Isn't that because the API docs are generated from typings? So there would be an issue in API also because an existing prop is missing from the docs. Though it wouldn't be nice to have it in the API docs since there seems to be no ready-made variants for it by default 🤔 But now the typings are blocking everyone from using the prop...

@jussirantala
Copy link

I started to work on a PR for this.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Jul 18, 2022

@jussirantala Can you provide a CodeSandbox showing a List component using a variant prop, and that the type is missing for it? The issue template maybe a good starting point. Thanks.

@jussirantala
Copy link

@jussirantala Can you provide a CodeSandbox showing a List component using a variant prop, and that the type is missing for it? The issue template maybe a good starting point. Thanks.

Here you go: https://codesandbox.io/s/list-variant-bug-5fqdfp?file=/src/App.tsx

@ZeeshanTamboli
Copy link
Member

Thanks for the reproduction. Looks like a bug indeed.

Not sure how does the style (background: red) gets applied if there is no support of variant prop in the List component implementation.

Looks like this would be the case for many other components in general.

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work and removed status: expected behavior Does not imply the behavior is intended. Just that we know about it and can't work around it labels Jul 18, 2022
@jussirantala
Copy link

jussirantala commented Jul 18, 2022

Seems it also complains about missing component prop if variant prop is used. I don't know the reason for that. Component prop should be optional.

@DinhBaoTran
Copy link
Contributor

Hi guys, I have similar issue with FormHelperText -> #33564

@mnajdova mnajdova changed the title [List] variant prop is missing in typings [material] Add variant prop type in all components Sep 30, 2022
@mnajdova mnajdova added new feature New feature or request design: material This is about Material Design, please involve a visual or UX designer in the process package: material-ui Specific to @mui/material and removed bug 🐛 Something doesn't work component: list This is the name of the generic UI component, not the React module! labels Sep 30, 2022
@mnajdova mnajdova mentioned this issue Oct 11, 2022
2 tasks
@mnajdova
Copy link
Member

Adding here some of the conversations we had on the open pull request. These are some open questions that would arise if we decide to support this in all components:

  • the component initially didn't supported the prop - this would mean that the component does not handle in anyway the prop, which would mean that it would be spreader on the inner DOM element. Should we always not propagate the prop in all components?
  • the component handles the prop, but the tree rendered depends on the variant prop - this means that we can't simply change it as we would break the internal logic of the component
  • what if this discussions gets created for a different prop next, like the size should we then handle this prop in all components too?

@CoennW
Copy link

CoennW commented May 31, 2024

Hi @mnajdova,

Wondering if there is any progress on this? We assumed variants to be supported in all component as mentioned in the docs. Our designer put time and effort in creating variants, now this does not seem to be supported. I would at least expect something like a notice in the docs, then we could have been aware of this months ago.

@matthew-h-wang
Copy link

Hi @mnajdova,

Wondering if there is any progress on this? We assumed variants to be supported in all component as mentioned in the docs. Our designer put time and effort in creating variants, now this does not seem to be supported. I would at least expect something like a notice in the docs, then we could have been aware of this months ago.

Similar situation here, without the variant prop in List/ListItem/ListButtonItem, I am not able to implement my designer's variants the way we we implement new variants to other components like Button or Typography.

@matthew-h-wang
Copy link

Hi @mnajdova,
Wondering if there is any progress on this? We assumed variants to be supported in all component as mentioned in the docs. Our designer put time and effort in creating variants, now this does not seem to be supported. I would at least expect something like a notice in the docs, then we could have been aware of this months ago.

Similar situation here, without the variant prop in List/ListItem/ListButtonItem, I am not able to implement my designer's variants the way we we implement new variants to other components like Button or Typography.

Actually, I learned that you can add your own variant prop (or any other custom prop) via the OwnProps interfaces. For example:

/* eslint-disable @typescript-eslint/no-empty-interface */

export type CustomListItemButtonOwnProps = {
  variant?: "standard" | "condensed" | "comfort";
  otherCustomProp?: boolean;
};

declare module "@mui/material/ListItemButton" {
  interface ListItemButtonOwnProps extends CustomListItemButtonOwnProps {}
}

or just

declare module "@mui/material/ListItemButton" {
  interface ListItemButtonOwnProps {
    variant?: "standard" | "condensed" | "comfort";
    otherCustomProp?: boolean;
  }
}

@CoennW
Copy link

CoennW commented Jul 4, 2024

Hi @mnajdova,
Wondering if there is any progress on this? We assumed variants to be supported in all component as mentioned in the docs. Our designer put time and effort in creating variants, now this does not seem to be supported. I would at least expect something like a notice in the docs, then we could have been aware of this months ago.

Similar situation here, without the variant prop in List/ListItem/ListButtonItem, I am not able to implement my designer's variants the way we we implement new variants to other components like Button or Typography.

Actually, I learned that you can add your own variant prop (or any other custom prop) via the OwnProps interfaces. For example:

/* eslint-disable @typescript-eslint/no-empty-interface */

export type CustomListItemButtonOwnProps = {
  variant?: "standard" | "condensed" | "comfort";
  otherCustomProp?: boolean;
};

declare module "@mui/material/ListItemButton" {
  interface ListItemButtonOwnProps extends CustomListItemButtonOwnProps {}
}

or just

declare module "@mui/material/ListItemButton" {
  interface ListItemButtonOwnProps {
    variant?: "standard" | "condensed" | "comfort";
    otherCustomProp?: boolean;
  }
}

Yes you can extend the props or find other ways to create variants. But the variant functionality, so using variant in the theme, that is described in the docs of MUI, will not work. This means we have different ways of adding variants, which is not really desirable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process discussion new feature New feature or request package: material-ui Specific to @mui/material
Projects
None yet
Development

No branches or pull requests

7 participants