Skip to content
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

Merged
merged 6 commits into from
May 23, 2024

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented May 22, 2024

Fixes #11618.

  • Avoid correlation between the disableOpenPicker prop intent and the fact that it forced the UI not to be rendered.
  • Add a test asserting that:
    • a button is hidden (if it even should be present) when disableOpenPicker="true"
    • the MuiPickersLayout-contentWrapper element has children - is not empty

@LukasTy LukasTy added regression A bug, but worse component: pickers This is the name of the generic UI component, not the React module! cherry-pick The PR was cherry-picked from the next branch labels May 22, 2024
@LukasTy LukasTy self-assigned this May 22, 2024
@@ -40,7 +40,7 @@ function DesktopDateTimePickerLayout<
{isLandscape ? shortcuts : toolbar}
{isLandscape ? toolbar : shortcuts}
<PickersLayoutContentWrapper
className={pickersLayoutClasses.contentWrapper}
className={clsx(pickersLayoutClasses.contentWrapper, classes?.contentWrapper)}
Copy link
Member Author

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. 🙈

@mui-bot
Copy link

mui-bot commented May 22, 2024

Deploy preview: https://deploy-preview-13212--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 4c3da18

@LukasTy LukasTy requested review from a team and flaviendelangle May 22, 2024 14:51
Copy link
Member

@flaviendelangle flaviendelangle left a 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;
Copy link
Member

@flaviendelangle flaviendelangle May 23, 2024

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

Copy link
Member Author

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? 🤔

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.

@LukasTy LukasTy enabled auto-merge (squash) May 23, 2024 10:50
@LukasTy LukasTy merged commit 5f6ceb4 into mui:master May 23, 2024
15 checks passed
@LukasTy LukasTy deleted the fix-disableOpenPicker-prop-behavior branch May 23, 2024 10:58
@LukasTy LukasTy added needs cherry-pick The PR should be cherry-picked to master after merge and removed cherry-pick The PR was cherry-picked from the next branch labels May 23, 2024
LukasTy added a commit to LukasTy/mui-x that referenced this pull request May 23, 2024
arthurbalduini pushed a commit to arthurbalduini/mui-x that referenced this pull request May 23, 2024
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pickers] using disableOpenPicker prop with responsive DatePicker shows no dates in mobile calendar dialog
3 participants