[List] Add `variant` prop

[jussirantala]

• [x] I have followed (at least) the PR section of the contributing guide.

Fixes issue: https://github.com/mui/material-ui/issues/33510

Codesandboxes: Not working: https://codesandbox.io/s/list-variant-bug-5fqdfp?file=/src/App.tsx Working (my latest commit in this PR): https://codesandbox.io/s/list-variant-bug-fixed-h7p0kk?file=/src/App.tsx

[mui-bot]

Details of bundle changes

Generated by :no_entry_sign: dangerJS against 806b8ac193883532a0253661cea78baa0e50bcb3

[jussirantala]

Could somebody help me to understand why all the checks are failing? The logs are not very useful, all of them seem to say just "exit code 1".

[emillaine]

I think you need to run at least yarn proptypes && yarn docs:api: see CONTRIBUTING.md.

[hbjORbj]

Please provide 2 Codesandboxes: one that shows the original error and another that shows that this PR has fixed the issue.

[jussirantala]

Please provide 2 Codesandboxes: one that shows the original error and another that shows that this PR has fixed the issue.

Added sandbox links to the description

[jussirantala]

@hbjORbj Hey, do you think the PR is now good to go?

[siriwatknp]

@jussirantala 👍 Thanks for initiating the PR. I have added an exception for the variant prop, so need another review from @mnajdova

[siriwatknp]

Why are we adding this property in the first place, if we don't handle it anyhow in the component? It is adding unnecessary bundle size for all users of the component. The variants key in the theme in it's name is misleading which probably caused the motivation for this PR. It is not related to the variant prop, it allows you to specify props combination for some component and the styles associated by it. I would recommend creating a wrapper around the List/ListItem component if you want to support variants in your design system.

I understand that you may have tried to add it and it works, as it is handled in the theme, but you may have noticed that the prop is being added to the inner HTML element, which is an indication that it is not handled.

If we want to go the other way and add the variant prop to all components, that is a different discussion, but overall the List should not be any exception from the guidelines we have set at this moment.

Styling the component directly from the theme sounds better than creating new components to me but I do understand that it comes with a cost that we haven't discussed before so happy to pause this PR.

[mnajdova]

Styling the component directly from the theme sounds better than creating new components to me but I do understand that it comes with a cost that we haven't discussed before so happy to pause this PR.

Agree, it may also raise things like, should the size be also added in all components. Should we support some default variants in all components? More over, in some components, like the TextField, the variant prop decides which component is being rendered, so developers can't just add custom variants, as it would break the component.

This is why I am being careful with adding this new API, when there is an already established React way of doing it - using a wrapper component.

[siriwatknp]

Agree, it may also raise things like, should the size be also added in all components. Should we support some default variants in all components? More over, in some components, like the TextField, the variant prop decides which component is being rendered, so developers can't just add custom variants, as it would break the component.

Thanks for the clarification.

[emillaine]

@mnajdova If that is the case, then the Creating new component variants section in the documentation is very misleading. Currently that documentation gives the impression that you can create custom variants for any component, and that the variant prop can be used to give names for those custom variants (for when the variant is not just a combination of existing props).

In @jussirantala's and my case, we would like to add variant='bordered' support to List, which based on that documentation seems like a supported use case. A wrapper component on the other hand feels like a workaround. IMO adding this support would be worth it regardless of whatever minimal bundle size impact it will have, as MUI is not a lightweight library anyway, but a library that allows you to customize it deeply.

[anied]

@mnajdova If that is the case, then the Creating new component variants section in the documentation is very misleading. Currently that documentation gives the impression that you can create custom variants for any component, and that the variant prop can be used to give names for those custom variants (for when the variant is not just a combination of existing props).

In @jussirantala's and my case, we would like to add variant='bordered' support to List, which based on that documentation seems like a supported use case. A wrapper component on the other hand feels like a workaround. IMO adding this support would be worth it regardless of whatever minimal bundle size impact it will have, as MUI is not a lightweight library anyway, but a library that allows you to customize it deeply.

I agree with this sentiment. Furthermore, from what I can see with my similar scenario with the <Dialog/> component, it builds and functions correctly with a custom, themed variant, but it makes TypeScript angry because it thinks the types don't match. Quite frustrating as what was a nice, clean abstraction via the theme instead will have to be done in a much more manual manner solely to avoid running afoul of the type system.

[mnajdova]

Thanks for the feedback. I propose we continue the discussion in https://github.com/mui/material-ui/issues/33510 and close the PR for now. We can re-open or create a new one once we have decided on the solution for it.