[pickers] Set closeOnSelect prop default value to false on pickers containing time views

[arthurbalduini]

Closes #9310

This PR introduces a breaking change because it changes the default value of the closeOnSelect prop, as mentioned here

[mui-bot]

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

Updated pages:

• docs/data/date-pickers/lifecycle/lifecycle.md

Generated by 🚫 dangerJS against 2a2e1f9

[LukasTy]

Thank you for taking care of it! 👍

I think that given this change of behavior, we should update the logic for default actions to also include the extended logic for closeOnSelect and have ['cancel', 'accept'] actions by default.
This would allow to remove the extra logic for this in:

mui-x/packages/x-date-pickers/src/DesktopTimePicker/DesktopTimePicker.tsx

Lines 80 to 82 in dd98382

	const actionBarActions: PickersActionBarAction[] = shouldRenderTimeInASingleColumn
	? []
	: ['accept'];

mui-x/packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePicker.tsx

Lines 176 to 178 in dd98382

	const actionBarActions: PickersActionBarAction[] = shouldRenderTimeInASingleColumn
	? []
	: ['accept'];

Regarding the change itself—I think that we could deliver it on a minor release given a good changelog entry description and gotchas that people can expect when updating (i.e. the need to change tests), but I'd love to hear input from @flaviendelangle and @joserodolfofreitas.
Maybe you vote for keeping this change for v8 to avoid any potential backlash? 🤔

[LukasTy]

Suggested change

	* @default `true` for desktop, `false` for mobile variants (based on the chosen wrapper and `desktopModeMediaQuery` prop).
	* @default `true` for desktop, `false` for mobile variant (based on the chosen wrapper and `desktopModeMediaQuery` prop).

[LukasTy]

If we are not deciding between variant usage.
WDYT about dropping this addition and keeping the description as is? 🤔

Suggested change

	* @default `true` for desktop, `false` for mobile variants (based on the chosen wrapper and `desktopModeMediaQuery` prop).
	* @default `true` for desktop, `false` for mobile (based on the chosen wrapper and `desktopModeMediaQuery` prop).

[samuelsycamore]

I think it reads more clearly without "variant" so unless it's absolutely necessary for clarity then I'd leave it out.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[LukasTy]

Leaving a round of technical feedback.
Will check the changes on DigitalClock separately. 😉

[LukasTy]

These are the same values as in usePickerLayout.
I think that we need to decide where we are putting these values and then go with it.
Maybe the default can reside in usePickerLayout, and in cases where we have closeOnSelect=true, we provide a different default.

This is a diff that I've played around with and seems to make sense. 🤔

diff --git a/packages/x-date-pickers-pro/src/DesktopDateRangePicker/DesktopDateRangePicker.tsx b/packages/x-date-pickers-pro/src/DesktopDateRangePicker/DesktopDateRangePicker.tsx
index 7fd3b8544f..134038e9df 100644
--- a/packages/x-date-pickers-pro/src/DesktopDateRangePicker/DesktopDateRangePicker.tsx
+++ b/packages/x-date-pickers-pro/src/DesktopDateRangePicker/DesktopDateRangePicker.tsx
@@ -75,6 +75,10 @@ const DesktopDateRangePicker = React.forwardRef(function DesktopDateRangePicker<
         hidden: true,
         ...defaultizedProps.slotProps?.toolbar,
       },
+      actionBar: (ownerState: any) => ({
+        actions: [],
+        ...resolveComponentProps(defaultizedProps.slotProps?.actionBar, ownerState),
+      }),
     },
   };
 
diff --git a/packages/x-date-pickers-pro/src/DesktopDateTimeRangePicker/DesktopDateTimeRangePicker.tsx b/packages/x-date-pickers-pro/src/DesktopDateTimeRangePicker/DesktopDateTimeRangePicker.tsx
index 0b3049cf97..23caca5e0e 100644
--- a/packages/x-date-pickers-pro/src/DesktopDateTimeRangePicker/DesktopDateTimeRangePicker.tsx
+++ b/packages/x-date-pickers-pro/src/DesktopDateTimeRangePicker/DesktopDateTimeRangePicker.tsx
@@ -12,7 +12,6 @@ import {
 } from '@mui/x-date-pickers/internals';
 import { extractValidationProps } from '@mui/x-date-pickers/validation';
 import { PickerOwnerState, PickerValidDate } from '@mui/x-date-pickers/models';
-import { PickersLayoutOwnerState } from '@mui/x-date-pickers/PickersLayout';
 import resolveComponentProps from '@mui/utils/resolveComponentProps';
 import { refType } from '@mui/utils';
 import {
@@ -25,7 +24,6 @@ import {
 } from '@mui/x-date-pickers/MultiSectionDigitalClock';
 import Divider from '@mui/material/Divider';
 import { digitalClockClasses } from '@mui/x-date-pickers/DigitalClock';
-import type { PickersActionBarAction } from '@mui/x-date-pickers/PickersActionBar';
 import { DesktopDateTimePickerLayout } from '@mui/x-date-pickers/DesktopDateTimePicker';
 import { rangeValueManager } from '../internals/utils/valueManagers';
 import { DesktopDateTimeRangePickerProps } from './DesktopDateTimeRangePicker.types';
@@ -168,7 +166,6 @@ const DesktopDateTimeRangePicker = React.forwardRef(function DesktopDateTimeRang
   const views = !shouldHoursRendererContainMeridiemView
     ? defaultizedProps.views.filter((view) => view !== 'meridiem')
     : defaultizedProps.views;
-  const actionBarActions: PickersActionBarAction[] = ['cancel', 'accept'];
 
   const props = {
     ...defaultizedProps,
@@ -199,10 +196,6 @@ const DesktopDateTimeRangePicker = React.forwardRef(function DesktopDateTimeRang
         toolbarVariant: 'desktop',
         ...defaultizedProps.slotProps?.toolbar,
       },
-      actionBar: (ownerState: PickersLayoutOwnerState) => ({
-        actions: actionBarActions,
-        ...resolveComponentProps(defaultizedProps.slotProps?.actionBar, ownerState),
-      }),
     },
   };
 
diff --git a/packages/x-date-pickers/src/DesktopDatePicker/DesktopDatePicker.tsx b/packages/x-date-pickers/src/DesktopDatePicker/DesktopDatePicker.tsx
index da90de5c5c..0a626407b8 100644
--- a/packages/x-date-pickers/src/DesktopDatePicker/DesktopDatePicker.tsx
+++ b/packages/x-date-pickers/src/DesktopDatePicker/DesktopDatePicker.tsx
@@ -80,6 +80,10 @@ const DesktopDatePicker = React.forwardRef(function DesktopDatePicker<
         hidden: true,
         ...defaultizedProps.slotProps?.toolbar,
       },
+      actionBar: (ownerState: any) => ({
+        actions: [],
+        ...resolveComponentProps(defaultizedProps.slotProps?.actionBar, ownerState),
+      }),
     },
   };
 
diff --git a/packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePicker.tsx b/packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePicker.ts
x
index 37a96e87ec..34a1f8c2e8 100644
--- a/packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePicker.tsx
+++ b/packages/x-date-pickers/src/DesktopDateTimePicker/DesktopDateTimePicker.tsx
@@ -23,7 +23,6 @@ import {
   resolveDateTimeFormat,
   resolveTimeViewsResponse,
 } from '../internals/utils/date-time-utils';
-import { PickersActionBarAction } from '../PickersActionBar';
 import { PickerOwnerState, PickerValidDate } from '../models';
 import {
   renderDigitalClockTimeView,
@@ -42,7 +41,6 @@ import { UsePickerViewsProps } from '../internals/hooks/usePicker/usePickerViews
 import { isInternalTimeView } from '../internals/utils/time-utils';
 import { isDatePickerView } from '../internals/utils/date-utils';
 import { buildGetOpenDialogAriaText } from '../locales/utils/getPickersLocalization';
-import { PickersLayoutOwnerState } from '../PickersLayout';
 
 const rendererInterceptor = function rendererInterceptor<
   TDate extends PickerValidDate,
@@ -174,7 +172,6 @@ const DesktopDateTimePicker = React.forwardRef(function DesktopDateTimePicker<
   const views = !shouldHoursRendererContainMeridiemView
     ? resolvedViews.filter((view) => view !== 'meridiem')
     : resolvedViews;
-  const actionBarActions: PickersActionBarAction[] = ['cancel', 'accept'];
 
   // Props with the default values specific to the desktop variant
   const props = {
@@ -210,10 +207,6 @@ const DesktopDateTimePicker = React.forwardRef(function DesktopDateTimePicker<
         hidden: true,
         ...defaultizedProps.slotProps?.tabs,
       },
-      actionBar: (ownerState: PickersLayoutOwnerState) => ({
-        actions: actionBarActions,
-        ...resolveComponentProps(defaultizedProps.slotProps?.actionBar, ownerState),
-      }),
     },
   };
 
diff --git a/packages/x-date-pickers/src/DesktopTimePicker/DesktopTimePicker.tsx b/packages/x-date-pickers/src/DesktopTimePicker/DesktopTimePicker.tsx
index 3516ece316..6ecbeb70ef 100644
--- a/packages/x-date-pickers/src/DesktopTimePicker/DesktopTimePicker.tsx
+++ b/packages/x-date-pickers/src/DesktopTimePicker/DesktopTimePicker.tsx
@@ -16,7 +16,6 @@ import {
   renderDigitalClockTimeView,
   renderMultiSectionDigitalClockTimeView,
 } from '../timeViewRenderers';
-import { PickersActionBarAction } from '../PickersActionBar';
 import { TimeViewWithMeridiem } from '../internals/models';
 import { resolveTimeFormat } from '../internals/utils/time-utils';
 import { resolveTimeViewsResponse } from '../internals/utils/date-time-utils';
@@ -77,7 +76,6 @@ const DesktopTimePicker = React.forwardRef(function DesktopTimePicker<
   };
 
   const ampmInClock = defaultizedProps.ampmInClock ?? true;
-  const actionBarActions: PickersActionBarAction[] = ['cancel', 'accept'];
 
   // Need to avoid adding the `meridiem` view when unexpected renderer is specified
   const shouldHoursRendererContainMeridiemView =
@@ -113,10 +111,6 @@ const DesktopTimePicker = React.forwardRef(function DesktopTimePicker<
         ampmInClock,
         ...defaultizedProps.slotProps?.toolbar,
       },
-      actionBar: {
-        actions: actionBarActions,
-        ...defaultizedProps.slotProps?.actionBar,
-      },
     },
   };
 
diff --git a/packages/x-date-pickers/src/PickersActionBar/PickersActionBar.tsx b/packages/x-date-pickers/src/PickersActionBar/PickersActionBar.tsx
index 59725fcb88..bf792eb530 100644
--- a/packages/x-date-pickers/src/PickersActionBar/PickersActionBar.tsx
+++ b/packages/x-date-pickers/src/PickersActionBar/PickersActionBar.tsx
@@ -11,7 +11,7 @@ export interface PickersActionBarProps extends DialogActionsProps {
   /**
    * Ordered array of actions to display.
    * If empty, does not display that action bar.
-   * @default `['cancel', 'accept']` for mobile and `[]` for desktop
+   * @default `['cancel', 'accept']`
    */
   actions?: PickersActionBarAction[];
   onAccept: () => void;

[arthurbalduini]

Great vision! I did a change inspired by your proposal which I think allowed to remove extra lines. Could you check 34c1cce and give your opinion ?

[LukasTy]

I don't have the strictest opinion on this one, but given that we will have only 2 components without actions, I thought it would make the most sense to simplify this management and put it where it makes the most sense. 🤔
If we look at the problem like this: should the layout logic care about or manage the necessary actions? 🤔
WDYT?

[flaviendelangle]

Suggested change

	The default value of `closeOnSelect` varies among the components:
	The default value of `closeOnSelect` depends on the component used:

But I'll let @samuelsycamore check the wordings 🙏

[samuelsycamore]

I'm happy with your version @flaviendelangle 👍

[flaviendelangle]

Suggested change

	- `DatePicker` and `DateRangePicker`: `true` on desktop and `false` on mobile variants;
	- Date Picker and Date Range Picker: `true` on desktop and `false` on mobile variants.

[flaviendelangle]

Suggested change

	- `TimePicker`, `DateTimePicker` and `DateTimeRangePicker`: `false` on both desktop and mobile variants.
	- Time Picker, Date Time Picker and Date Time Range Picker: `false` on both desktop and mobile variants.

[flaviendelangle]

Same changes as proposed above

[flaviendelangle]

Not fan of just adding variant after mobile here, I would stick with the old wording or use variants for both:

Suggested change

	* @default `true` for desktop, `false` for mobile variant (based on the chosen wrapper and `desktopModeMediaQuery` prop).
	* @default `true` for the desktop variant, `false` for the mobile variant (based on the chosen wrapper and `desktopModeMediaQuery` prop).

or

Suggested change

	* @default `true` for desktop, `false` for mobile variant (based on the chosen wrapper and `desktopModeMediaQuery` prop).
	* @default `true` for desktop, `false` for mobile (based on the chosen wrapper and `desktopModeMediaQuery` prop).

[LukasTy]

We currently call them "variants" in the doc as well. 🙈
P.S. Both of your suggestions seem to be the same. 🙈

[flaviendelangle]

Suggestion updated 😆
My problem is not using the word "variant", it's the phrasing "true for desktoop, false for mobile variant" which seems inconsistent to me.

[flaviendelangle]

I would prefix this one in case we want to add it to the main ownerState later on.

Suggested change

	})<{ ownerState?: { valueType: ValueType } }>({
	})<{ ownerState?: { pickersValueType: ValueType } }>({

I would add this property in usePickerLayout and return the ownerState from there, that way we use the exact same object in everything layout-related.
Note that this change is a breaking change since people are using this DOM element when building custom layout.

---

With that being said, I find the change quite fragile, not sure how we could improve it...
I think we should be able to use something like grid-column: 1/-1; to avoid hardcoding the number of columns on items that should spread from left to right.

[arthurbalduini]

I fully gree with the ownerState approach. It seems indeed a smoother path !

---

Regarding the fragility of the change, I tried your suggestion and I'm afraid it would create other layout issues, since the number of columns is not explicitly defined by the LayoutRoot, using grid-column: 1/-1; would not be correctly interpreted

unless the number of columns is explicted in the parent with something like grid-template-columns: 'auto auto auto', for example. But in this case wouldn't we, in a certain way, also be hardcoding it? Plus, by the approach I tried, it breaks the layout of DateCalendar with shortcuts since the shortcuts bar is by default on the first column

Do you see another way to refactor the grid parameters ? 

I assume that since the original value was 2, which could be argued as hardcoded since there's an assumption about the number of columns behind the scene, maintaining it for the Date components and adding a new one to Time components seemed to make sense, also because currently there's already opportunity for an UI improvement on these components when they have a complete ActionBar, as mentioned in this comment

[flaviendelangle]

We should export the new type and treat that as a breaking change.

[LukasTy]

Agreed.
WDYT about PickerValueType?
Would that be too confusing beside PickerValue? 🤔

[LukasTy]

On another note, @flaviendelangle do we have a clear use-case for why we are exporting this type? 🤔

[LukasTy]

Agreed.
WDYT about PickerValueType?
Would that be too confusing beside PickerValue? 🤔

[LukasTy]

I don't have the strictest opinion on this one, but given that we will have only 2 components without actions, I thought it would make the most sense to simplify this management and put it where it makes the most sense. 🤔
If we look at the problem like this: should the layout logic care about or manage the necessary actions? 🤔
WDYT?

[LukasTy]

We currently call them "variants" in the doc as well. 🙈
P.S. Both of your suggestions seem to be the same. 🙈

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[LukasTy]

Another round of ideas. 😉

[LukasTy]

If we are not deciding between variant usage.
WDYT about dropping this addition and keeping the description as is? 🤔

Suggested change

	* @default `true` for desktop, `false` for mobile variants (based on the chosen wrapper and `desktopModeMediaQuery` prop).
	* @default `true` for desktop, `false` for mobile (based on the chosen wrapper and `desktopModeMediaQuery` prop).

[LukasTy]

If we have a specific definition per component, we know its default behavior.

Suggested change

	* If `true`, the popover or modal will close after submitting the full date.
	* If `true`, the popover will close after submitting the full date.

Otherwise, we could go the more abstract route to avoid the need to update the JSDoc when we move range pickers away from the tooltip to dialog/popover. 🤔

Suggested change

	* If `true`, the popover or modal will close after submitting the full date.
	* If `true`, the Picker will close after submitting the full date.

[LukasTy]

Maybe we could update the JSDoc of this prop on UsePickerValueNonStaticProps type definition since it is now the most common default.
Then we can keep the overrides only for DateRangePicker and DesktopDateRangePicker. 🤔
WDYT?

Suggested change

	/**
	* If `true`, the popover or modal will close after submitting the full date.
	* @default false
	*/
	closeOnSelect?: boolean;

[samuelsycamore]

I'm happy with your version @flaviendelangle 👍

[samuelsycamore]

is this supposed to be singular here but plural in the next one?

[samuelsycamore]

same as @flaviendelangle 's suggestion above: Date Picker, Date Range Picker, etc.

[samuelsycamore]

I think it reads more clearly without "variant" so unless it's absolutely necessary for clarity then I'd leave it out.