-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
[pickers] Fix disableOpenPicker
prop behavior
#13212
[pickers] Fix disableOpenPicker
prop behavior
#13212
Conversation
@@ -40,7 +40,7 @@ function DesktopDateTimePickerLayout< | |||
{isLandscape ? shortcuts : toolbar} | |||
{isLandscape ? toolbar : shortcuts} | |||
<PickersLayoutContentWrapper | |||
className={pickersLayoutClasses.contentWrapper} | |||
className={clsx(pickersLayoutClasses.contentWrapper, classes?.contentWrapper)} |
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.
The addition of the test allowed me to discover this "bug"—the slotProps.layout.classes
were not used in the case of DesktopDateTimePicker
and DesktopDateTimeRangePicker
. 🙈
Deploy preview: https://deploy-preview-13212--material-ui-x.netlify.app/ |
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 makes a lot of sense!
@@ -185,7 +184,7 @@ export const usePickerViews = < | |||
TAdditionalProps | |||
>): UsePickerViewsResponse<TView> => { | |||
const { onChange, open, onClose } = propsFromPickerValue; | |||
const { views, openTo, onViewChange, disableOpenPicker, viewRenderers, timezone } = props; | |||
const { views, openTo, onViewChange, viewRenderers, timezone } = props; |
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.
You may be able to simplify the typing and get rid of UsePickerViewsNonStaticProps
if disableOpenPicker
is no longer used in this file, and instead have disableOpenPicker
defined directly in UseDesktopPickerProps
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 noting it.
Did you mean this 4c3da18? 🤔
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.
Not exactly
From what I see, disableOpenPicker
is defined in UsePickerViewsNonStaticProps
Which is weird now that this prop is never used in the usePickerViews
hook.
If you move it to UseDesktopPickerProps
, it would be removed from the mobile pickers (which make sense since it does nothing on them).
Or maybe I'm missing something, I did not dive deeper tbh
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.
On Mobile, it currently doesn't change anything, but on range pickers, it prevents the picker from opening on click. 🤔
That's why I didn't move anything more than needed now.
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
The current position of this prop's definition seems odd to me but we can move it later if we find a better interface to put it in.
Fixes #11618.
disableOpenPicker
prop intent and the fact that it forced the UI not to be rendered.disableOpenPicker="true"
MuiPickersLayout-contentWrapper
element has children - is not empty