-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pickers] Refactor slots
and slotProps
propagation strategy
#18867
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
[pickers] Refactor slots
and slotProps
propagation strategy
#18867
Conversation
Deploy preview: https://deploy-preview-18867--material-ui-x.netlify.app/ Bundle size report
|
{...(isSingleInput && { | ||
inputRef, | ||
})} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multi Input fields do not support inputRef
prop
</InputAdornment> | ||
); | ||
} | ||
// handle the case of showing custom `inputAdornment` for Field components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to handle a case when a field component would have an inputAdornment
slot.
Technically, the API contract declares support for it... 🤷
Basically, to handle a DateField
component instead of DatePicker
in this demo.
* Overridable component slots. | ||
* @default {} | ||
*/ | ||
slots?: PickerFieldUISlots; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change, PickerFieldUi
no longer supports slots
and slotProps
AND the same props in the context.
In every case, these props are passed via context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look at it!
</Layout> | ||
</PickersModalDialog> | ||
</PickerFieldUIContextProvider> | ||
<Field slots={slots} slotProps={slotProps} inputRef={inputRef} {...fieldProps} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this remove the ability to use props.slotProps.field.slotProps.xxx
at the picker level?
For example:
<DatePicker
slotProps={{
field: {
slotProps: {
clearIcon: { sx: { opacity: 0 } },
}
}
}}
/>
Since it's part of fieldProps
but now it would override the props we pass and potentially break stuff (remove the toolbar title id for example).
In most cases it's probably not something we want to encourage (since the slots also exist at the picker level).
But it might be required for some edge cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I did found the scenario that do not work 😆
import * as React from 'react';
import dayjs from 'dayjs';
import { DemoContainer } from '@mui/x-date-pickers/internals/demo';
import { AdapterDayjs } from '@mui/x-date-pickers/AdapterDayjs';
import { LocalizationProvider } from '@mui/x-date-pickers/LocalizationProvider';
import { DatePicker } from '@mui/x-date-pickers/DatePicker';
export default function BasicDatePicker() {
return (
<LocalizationProvider dateAdapter={AdapterDayjs}>
<DemoContainer components={['DatePicker']}>
<DatePicker
defaultValue={dayjs()}
slotProps={{
openPickerButton: {
sx: { color: 'green' },
},
field: {
slotProps: {
openPickerButton: { 'data-testid': 'open-picker-button' },
},
},
}}
/>
</DemoContainer>
</LocalizationProvider>
);
}
From what I can see, on master
it's slotProps.openPickerButton
that has the highest priority.
And on your PR it's slotProps.field.slotProps.openPickerButton
(because of the spread I commented on I think).
Both fail to correctly merge the 2 objects though.
Feel free to ignore it if you feel it's too much of an edge case.
It's problematic if we set one of the two internally (because it would prevent the user to user the other one without bugs), but I don't think we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flaviendelangle I have updated the solution.
Added a few tests to ensure the fixed Field.slots.inputAdornment
behavior stays intact.
Also inverted the order of props to keep the existing behavior you noticed in your comment.
I will just add a note, that based on TS, your example is invalid, field
slotProp does not have it's own slotProps
. 🙈
28b5b0d
to
dce13e3
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
slots
and slotProps
are propagated to internal componentslots
and slotProps
propagation strategy
render( | ||
<DateField | ||
enableAccessibleFieldDOMStructure | ||
slots={{ inputAdornment: CustomInputAdornment }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests to ensure this slot behaves as expected on Field components - this is the main bug-fix in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't find any problematic edge case 👌
Noticed a problem with

inputAdornment
resolution while exploring #17738 and checking https://mui.com/x/react-date-pickers/custom-opening-button/#add-an-icon-next-to-the-opening-buttonThe problem is on this line:
mui-x/packages/x-date-pickers/src/internals/components/PickerFieldUI.tsx
Lines 143 to 145 in be7b78c
A forgotten case to handle
?? pickerFieldUIContext.slots.inputAdornment
.This felt too fragile to me, given that we've already made a mistake here without noticing. 🙈
This particular issue has been recently resolved with a single line targeted fix: #19399, but I feel like the general approach could still be improved.
Besides, this PR also fixes an additional issue.
I refactored how
slots
andslotProps
are propagated to avoid the need to targetslotProps ?? context.slotProps
.