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.38k stars 32.14k forks source link

[Base] Accept callbacks in `componentsProps.*` #32144

Closed michaldudak closed 2 years ago

michaldudak commented 2 years ago

The componentsProps fields currently accept plain objects, that are spread to corresponding slots' props:

<InputUnstyled
  componentsProps={{
    root: { className: 'custom-root' },
    input: { className: 'custom-input' },
  }}
/>

This approach makes it harder to apply slots' props conditionally, based on the state of the unstyled component. The only way it's possible right now, is by providing a custom slot component with the logic that determines the props to set.

This is unnecessarily verbose. A much simpler way (originally proposed in https://github.com/mui/material-ui/pull/30700#discussion_r788279436) would be to accept functions in componentsProps' fields. Such a function would be called with ownerState as the parameter:

<InputUnstyled
  componentsProps={{
    root: ({ focused }) => ({ className: focused ? 'custom-focus' : '' }),
    input: ({ error }) => ({ 'data-error': error }),
  }}
/>

cc @flaviendelangle

oliviertassinari commented 2 years ago

The only way it's possible right now, is by providing a custom slot component with the logic that determines the props to set.

For context, the assumption in https://github.com/mui/material-ui/issues/21453 is that it would be enough, that it was not worth the overhead (e.g. the extra code bundled, larger API surface to teach, the opportunity cost for our time) to support it. It also assumed that the extra boilerplate also comes with an advantage: it's more declarative, there is less mystery about what's the priority resolution orders between different props, since it's a component you control. Maybe it can avoid a potential foot gun 😁.

For benchmark, in https://github.com/straw-hat-team/adr/blob/master/adrs/1358452048/README.md, they don't have the notion of componentsProps, which would lead to more boilerplate.

But I agree that using the current API with utility classes (e.g. Tailwind CSS), it becomes very challenging. In https://headlessui.com/react/tabs they use a render function to solve this problem:

        <Tab as={Fragment}>
          {({ selected }) => (
            <button
              className={
                selected ? 'bg-blue-500 text-white' : 'bg-white text-black'
              }
            >
              Tab 1
            </button>
          )}
        </Tab>

it sounds like we have a more generic/predictable API, so easier to learn and use 👍 (with some downsides, the ones I have listed above).


I almost wonder if the API shouldn't be:

<InputUnstyled
  classes={{
    root: ({ focused }) => (focused ? 'custom-focus' : ''),
  }}
/>

less code than:

<InputUnstyled
  componentsProps={{
    root: ({ focused }) => ({ className: focused ? 'custom-focus' : '' }),
  }}
/>

I wonder because the problem we solve seems to be all about class names. I mean, the concrete problems we refer to in this GitHub issue so far are https://github.com/mui/material-ui/pull/30700#pullrequestreview-857544962 and https://headlessui.com/react/tabs, all about the class attribute. (I have assumed that we should heavily discount the value of hypothetical use cases because it's too hard to reason through them)

flaviendelangle commented 2 years ago

We have on different problem solved by the callbacks in componentsProps.* which is the mobile / desktop props on the pickers. By the way @alexfauquette we did not implement the callback at the same level

<DatePicker
  componentsProps={{
    actionBar: {
      // The actions will be the same between desktop and mobile
      actions: ['clear'],

      // The actions will be different between desktop and mobile
      actions: (variant) => (variant === 'desktop' ? [] : ['clear']),
    },
  }}
/>
michaldudak commented 2 years ago

In https://headlessui.com/react/tabs they use a render function to solve this problem

It's not possible to modify the Tab's props this way. This could only work for composite components, so it's not really the same problem we're solving with componentsProps

I wonder because the problem we solve seems to be all about class names. I mean, the concrete problems we refer to in this GitHub issue so far are https://github.com/mui/material-ui/pull/30700#pullrequestreview-857544962 and https://headlessui.com/react/tabs, all about the class attribute.

I've seen requests from the community to enable componentsProps on Material UI components and the motivation was not customization of classes (#33018). I wouldn't drop componentsProps altogether. I agree that using classes, as you proposed, would be simpler and require less code. I would not replace componentsProps with classes, though. We could think of adding the classes prop if the community thinks it would be beneficial. The downside of this is that we'd have yet another way of customization that we'll need to support.

michaldudak commented 2 years ago

I'm closing this issue as we've got all the existing Base components covered. Let's discuss other changes in a new issue (or in #28189) if needed.