[material-ui][Autocomplete] Fix PaperComponent prop type #41837
Conversation
hectortav
commented
Apr 10, 2024
hectortav
commented
- I have followed (at least) the PR section of the contributing guide.
Netlify deploy previewhttps://deploy-preview-41837--material-ui.netlify.app/ Bundle size report |
zannager
added
the
component: autocomplete
There was a problem hiding this comment.
@hectortav Thanks for the PR. Are you encountering any issues with it? If so, could you provide a link to the corresponding GitHub issue in the description? Or is it simply an enhancement?
ZeeshanTamboli
added
enhancement
|
Hello! |
Thanks for the example. This should be mentioned in the PR description when opening a PR. Could you add a TypeScript test related to this in |
| type PaperComponentProps = { title?: string }; | ||
| declare module '@mui/material/Paper' { | ||
| export interface PaperBaseProps extends PaperComponentProps {} | ||
| } | ||
|
|
||
| function PaperComponent({ title, children, ...props }: PaperProps) { | ||
| return ( | ||
| <Paper {...props}> | ||
| {title} | ||
| {children} | ||
| </Paper> | ||
| ); | ||
| } | ||
|
|
||
| function AutocompleteCustomPaper() { | ||
| return ( | ||
| <Autocomplete | ||
| options={['one', 'two', 'three']} | ||
| renderInput={(params) => <TextField {...params} />} | ||
| PaperComponent={PaperComponent} | ||
| componentsProps={{ | ||
| paper: { | ||
| title: 'Title', | ||
| variant: 'outlined', | ||
| }, | ||
| }} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
This isn't testing what's needed. Because if I revert the PaperComponent type back to React.JSXElementConstructor<React.HTMLAttributes<HTMLElement>>, the test still passes.
There was a problem hiding this comment.
Hello again! Sorry for the late response. It should be fixed now
… extended props [material-ui][Autocomplete] Fix typescript tests
hectortav
force-pushed
the
autocomplete-paper-component-type
branch
from
3501b67
to
f039ff2
Compare
There was a problem hiding this comment.
Ultimately, once the slots prop is introduced and utilized for the Autocomplete component, this PR won't serve much purpose as the PaperComponent will become deprecated. What are your thoughts on this, @DiegoAndai? (I suggest reading all the PR comments)
| // eslint-disable-next-line react/no-unstable-nested-components | ||
| PaperComponent={({ elevation, variant, ...props }) => ( | ||
| <Paper {...{ elevation, variant }} {...props} /> | ||
| )} |
There was a problem hiding this comment.
Rendering the component inline isn't recommended due to unnecessary re-renders which is mentioned in the eslint rule react/no-unstable-nested-components. However, it seems we can't test it any other way. Let's keep the PaperComponent type modification but remove this test. Developers can and should render a custom Paper component like this:
function CustomPaper(props: PaperProps) {
return <Paper square {...props} />;
}
function AutocompleteCustomPaper() {
return (
<Autocomplete
options={['one', 'two', 'three']}
renderInput={(params) => <TextField {...params} />}
PaperComponent={CustomPaper}
componentsProps={{
paper: {
elevation: 2,
variant: 'outlined',
},
}}
/>
);
}It seems okay not to have a test for the modification of the PaperComponent prop type because, apart from inlining a component (not recommended), we can't test it, with it having React.JSXElementConstructor type. In any case, we plan to deprecate this approach and use the slots prop instead (slots.paper).
hectortav
force-pushed
the
autocomplete-paper-component-type
branch
from
6ae1290
to
befda08
Compare
|
Thanks for the callout @ZeeshanTamboli. We shouldn't merge this change as We currently have a PR open to deprecate |
|
Alright, I'll close this PR. Thanks for trying to fix it, @hectortav. |
