-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[RFC] What's the best API to override deep/nested elements? #21453
Comments
While in my initial comment was trying to draw an accurate presentation of what we have explored so far, I'm going to try to explore the pros & cons of each approach we have learned. Component APIHistorically, we went with the API because it was minimizing the complexity of the source. It was requiring less boilerplate to implement. It was making the source easier to read. I suspect there are a lot of people looking at the source, either for learning best practices, how to apply customization, to debug something, to contribute, etc. The second advantage is that it allows using the hook API directly. render prop APIHowever, we have quickly realized the limitations of the component API. The limitation is twofold:
I think that it's this latter reason that leads us to introduce more
Multiple props vs one propI'm switching gears a little bit here on a different concern: scalability. Should we have one prop per customization point or should we group the customization points under a single prop? So far, we have had different answer to this problem:
|
Sharing my experience and thoughts on this matter:
|
I think we can split the problem in 3 categories
|
FYI I have refactored the approach on the grid for the footer and header, which were the only ones using children. |
@dtassone Thanks for sharing your exploration, once we settle on one approach we can update all the components. Proposal number 1
components?: {
Root?: React.ElementType; // equivalent to classes.root
Label?: React.ElementType; // equivalent to classes.label
}; This also means moving the existing
cc @mui-org/core-team |
I definitely support moving to this: // notice slots
slots?: {
// notice the functions
Root?: (props)=> React.ElementType; // equivalent to classes.root
Label?: (props)=> React.ElementType; // equivalent to classes.label
}; Matches I have documented this pattern for our internal React components in https://github.com/straw-hat-team/adr/blob/master/adrs/1358452048/README.md. |
@yordis Could you provide an example to demonstrate the benefit of a function here rather than just With the following:
users can do things like:
It seems cumbersome and confusing to instead do
|
@ryancogswell hey there, let me know if the following example makes sense. I think your example is valid, but there are some caveats to it since that example is for trivial cases, sometimes you will need to remap or access hoisted values so you can't simply do that. Thinking out loud, the differences between a Function and React Component is technically none for the end-users, except for Mui internals and React itself, so I guess you can take a "Component" rather than a function anyway. import ExternalComponent from 'whatever-thing-i-dont-control';
// static, it doesn't require runtime.
const SomethingNoHoisted = (props)=> {
return <ExternalComponent remapped={props.label} title={props.title}/>
}
function MyComponent(props) {
const title = props.title ?? 'Unknown';
// I guess you co do this, the only thing you are missing is that Mui
// will use `createElement` or <Something/> over Something() in the internals (or you change to createSomething), so no a big
// deal I guess.
// The caveat is that you lose some static declaration of the component
// and require runtime for it, meh.
// So personally speaking, I don't see these functions as component but factories.
function Something(props) {
return (
// Has access to anything internal from MuiComponent
// passing using props in case that some computations from inside
/// needs to be exposed to the outside scope
<ExternalComponent remapped={props.label}
// Remapping in case you need it
remapped={props.label}
// Access to hoisted variables,
title={title}/>
)
}
return (
<div>
<MuiComponent
slots={{
// Something: Something,
// I can't do this due to prop hoisting values, and props that needs
// to be remapped.
// Something: ExternalComponent
// I don't control the rendering of the component, therefore, I can't
// pass `title` from the hoisted variable.
// Something: SomethingNoHoisted
}}
/>
</div>
)
} And I can't agree that it is cumbersome and confusing due to my personal experience, I can only speak from my perspective. And probably you are right, I am thinking in FP practices for function composition that we don't need in some cases and will create harder to understand code for some people. I take back my suggestion, doesn't matter to me. |
@yordis It seems that the main problem you are trying to solve (or at least a different way to frame the problem) is how to pass additional props to your custom component. One way this has been solved in Material-UI is a corresponding For instance,
or if we take the complicated case in your example (the one requiring a function) it could look like:
In this scenario MUI would merge the props specified via As a side note, I prefer the name |
<Modal components={Backdrop: CustomBackdropComponent} BackdropProps: { customProp1: props.valueFromParentScope } ...>...</Modal> That example is exactly what I am trying to prevent since I been there. This is why either to use a factory function which I personally prefer or a dynamic component in the scope. I have been bitten by that example multiple times and the problems that bring in the future. Reasons
|
@yordis To take your example, using props from the outside would look like this, with the first proposal. We would use the context: import ExternalComponent from 'whatever-thing-i-dont-control';
const Context = React.createContext();
function Something(props) {
const { title } = React.useContext(Context);
return (
<ExternalComponent remapped={props.label}
remapped={props.label}
title={title} />
)
}
function MyComponent(props) {
const title = props.title ?? 'Unknown';
return (
<div>
<Context.Provider value={{ title }}>
<MuiComponent
components={{
Something,
}}
/>
</Context.Provider>
</div>
)
} It comes with two drawbacks compared to the 2. A second possible proposalThe same one as the first, however, we introduce a prop to match the current use cases for the components?: {
Root?: React.ElementType<RootProps>;
Label?: React.ElementType<LabelProp>;
};
componentsProps?: {
root?: RootProps;
label?: LabelProp;
}; This option has the advantage of allowing to customize nested components without even needing to import them. 3. A third possible proposalThis time with render props, all in. For the cases where the render prop is mandatory, like renders?: {
root?: (props: RootProps) => React.ReactNode;
label?: (props: LabelProp) => React.ReactNode;
}; I see two downsides with this approach
const Button = React.forwardRef(function Button(props, ref) {
const {
children,
classes,
className,
color = 'default',
component = 'button',
renders = { root = props => <ButtonBase {...props} />, label: props => <span {...props} /> },
disabled = false,
disableElevation = false,
disableFocusRipple = false,
endIcon: endIconProp,
focusVisibleClassName,
fullWidth = false,
size = 'medium',
startIcon: startIconProp,
type = 'button',
variant = 'text',
...other
} = props;
const startIcon = startIconProp && (
<span className={clsx(classes.startIcon, classes[`iconSize${capitalize(size)}`])}>
{startIconProp}
</span>
);
const endIcon = endIconProp && (
<span className={clsx(classes.endIcon, classes[`iconSize${capitalize(size)}`])}>
{endIconProp}
</span>
);
return renders.root({
className: clsx(
classes.root,
classes[variant],
{
[classes[`${variant}${capitalize(color)}`]]: color !== 'default' && color !== 'inherit',
[classes[`${variant}Size${capitalize(size)}`]]: size !== 'medium',
[classes[`size${capitalize(size)}`]]: size !== 'medium',
[classes.disableElevation]: disableElevation,
[classes.disabled]: disabled,
[classes.fullWidth]: fullWidth,
[classes.colorInherit]: color === 'inherit',
},
className,
),
component,
disabled: true,
focusRipple: !disableFocusRipple,
focusVisibleClassName: clsx(classes.focusVisible, focusVisibleClassName),
ref,
type,
...other,
children: render.span({
className: classes.label,
})
})
}); On a related note, during my exploration of the unstyled story, I ended up trying a prop to solve a similar problem: forwardProps?: (slot: 'root' | 'label', state) => props; The prop (or something similar) is required to give the nested styled-components access to the external props and internal state of the component, to style it accordingly. It also assumes that we want to keep the CSS specificity at 1. |
My personal opinion: How to store the props?Here we have only 2 possible options: flat vs deep. I believe that
Components vs functionsPersonally I have no preference on that. There is no performance difference even on the big amount of nodes. Also could say that render functions look more expressive from the reading side. In this example, if you are looking for overrides rendering you could miss the <Something
SelectComponent={Select}
/> And here your editor will shout you that something is rendering over here. <Something
renderSelect={props => <Select {...props} />}
/> |
Reading your examples and opinions, and based on my experience, I definitely will stick to factory/render function to tackle this problem. Something else we could do is to adopt some practices from the Vue community if I am not mistaken they use factory/render functions to tackle this issue. I like that they use // some generics
interface MuiUniversalProps <T = unknown> {
slots?: T;
}
interface Slot<T = unknown> {
(props: T): React.Child;
} // button component
interface ButtonSlots {
root: Slot<{}>;
label: Slot<{}>;
}
interface ButtonProps extends MuiUniversalProps<ButtonSlots> {
...
} I don't think I have much to contribute at this point, I trust your judgment. |
flat vs. deep@dmtrKovalenko While I think that we should encourage flatten props as much as possible for the reasons mentioned in #21453 (comment). (@dtassone Yes, I very much have the DataGrid API in mind the options could be flattened :p), It's not without its limitations. I see a couple of advantages in going deep:
|
Thanks @oliviertassinari :) I think both ways are very discussable If we decide to flatten the props, then, inside the component, we will probably have to restructure some of the props together, so we can process or observe them together. Another kind of drawback is that we mind end up with a very long list of props for some components and it might be difficult to find what we want in the middle of everything. Events, components are quite limited, but some components might have a long list of configuration options. So should we consider to flatten everything except configuration options... |
I would vote for the proposal 2. The 3rd one seems to be a no-go regarding the readability of the source at scale. Readability is still OK for one-off cases. The 1st one is quite limiting for leveraging props from the outside. it's cumbersome to leverage the context. Hence the second as a tradeoff. It's basically the 1st proposal but extended with |
Has anyone taken a look at an approach like import {useButton} from '@react-aria/button';
function Button(props) {
let ref = React.useRef();
let {buttonProps} = useButton(props, ref);
return (
<button {...buttonProps} ref={ref}>
{props.children}
</button>
);
}
<Button onPress={() => alert('Button pressed!')}>Press me</Button> I've used a couple APIs like this (another is That way, if a user wants to override something, they can dig in and find out more about the structure, but if they don't then they don't have to. React Aria's API is like this in most places, so take a look there for more concrete examples of it working in practice. |
@dimitropoulos This RFC doesn't aim to solve the whole problem of customizability. There are different levels of abstraction that are viable, each with pros & cons, I would put them in 3 buckets:
The RFC focuses on improving 3. This is a problem we face with the upcoming DataGrid and DatePicker components. The RFC is hopefully also something I hope we can use to provide unstyled components or different styles engine. For 1. it will require a different effort. But I definitely agree that it would make sense to unbundled the component we have, like the angular CDK. Do you have any interest in helping us with this? |
sure sure, yeah, I get that, I was just saying that whatever solution this RFC lands on - it would be cool to keep the |
An update, proposal 2 has been implemented with the Slider, Badge, and recently the DatePicker. We are pushing further with this approach. |
@oliviertassinari and, to confirm, part of the design (not just happy accident) with proposal 2 is that you can pass <MuiXXX
components={{
Root: SomeRootComponent,
Label: SomeLabelComponent,
}}
componentsProps={{
root: { someProp: 'some value' },
label: { someProp: 'some value' },
}}
/> |
Closing as we agreed on: <MuiXXX
components={{
Root: SomeRootComponent,
Label: SomeLabelComponent,
}}
componentsProps={{
root: { someProp: 'some value' },
label: { someProp: 'some value' },
}}
/> going forward. |
A while back, around the alpha & beta iterations on the v1 version, we had to decide what was the best way to override the nested components. It's meant to help developers customize the rendering. We can find #11204 and #10476 as a legacy.
up until v1
So far, the tradeoff on this problem has been:
XXXComponent
prop when overriding the nested element can be valuable for users. For instance:https://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui/src/Modal/Modal.d.ts#L8
XXXProps
prop when providing a customXXXComponent
component is too cumbersome. For instance:https://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui/src/Modal/Modal.d.ts#L9
However, this original design constraint is increasingly more challenged by the following:
Autocomplete
The Autocomplete mixes the
renderXXX
approach with theXXXComponent
approach. cc @oliviertassinarihttps://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts#L164
https://github.com/mui-org/material-ui/blob/c6b95d2e773088af66823f8995c3e57508c82056/packages/material-ui-lab/src/Autocomplete/Autocomplete.d.ts#L142
Date Picker
The DatePicker mixes the
renderXXX
approach with theXXXComponent
approach. cc @dmtrKovalenkohttps://github.com/mui-org/material-ui-pickers/blob/c834f5cac5930df099aaad806ceb3fe4ec1669db/lib/src/views/Calendar/Calendar.tsx#L43
https://github.com/mui-org/material-ui-pickers/blob/c834f5cac5930df099aaad806ceb3fe4ec1669db/lib/src/typings/BasePicker.tsx#L61
Data Drid
The DataGrid goes with
xXXComponent
approach, however, the name is confusing. Sometimes it's a render prop, sometimes it's a React element, it's never a component as the name suggests cc @dtassonehttps://github.com/mui-org/material-ui-x/blob/59d533642d5837ce6912fe88cbcdfc228f621594/packages/grid/x-grid-modules/src/models/gridOptions.tsx#L93-L97
Styled components
Styled components might request something brand new. cc @mnajdova
As the experimentation of #21104 showcases. If we want to keep the CSS specificity at it's lowest possible level (meaning one level) and expose unstyled components, we will have to expose an API to inject custom component. In older experimentation, I worked around the problem with a
components
prop.https://github.com/oliviertassinari/material-ui/blob/153c1833fb204a28ee6712db1c2ac6a9308a163e/packages/material-ui/src/TableCell/TableCell.unstyled.js#L18
The problem
I think that it would be great to defines which API works best and in which cases. I would hope that by doing so, we can provide a consistent experience for the developers using the library to build applications and websites. I also think that by reducing the number of approaches we can reduce the learning curve for new users.
When should we use a component, when should we use a render prop, etc.?
Prior-arts
The text was updated successfully, but these errors were encountered: