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

[DateRangePicker] disable day on date range #5773

Merged
merged 11 commits into from
Aug 26, 2022

Conversation

alexfauquette
Copy link
Member

Fix #5769

I tried a POC for #5769 to add a parameter 'start' and 'end' for now only on DesktopDateRangePicker. It will need some cleaning but seems to work.

@mui-bot
Copy link

mui-bot commented Aug 12, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 326.3 673.3 450 465.18 124.34
Sort 100k rows ms 470.7 895.3 774.2 718.46 150.812
Select 100k rows ms 198.9 295.1 227.6 242 41.165
Deselect 100k rows ms 127.9 273.6 210.9 215.88 50.772

Generated by 🚫 dangerJS against 7ac942a

@LukasTy
Copy link
Member

LukasTy commented Aug 15, 2022

The POC looks superb 🙌
I do believe that this should easily solve the original issue.
But as Flavian mentioned in it, it would be best to also add this option to shouldDisableMonth and shouldDisableYear.

@alexfauquette alexfauquette added the component: DateRangePicker The React component. label Aug 16, 2022
@github-actions
Copy link

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

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 24, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 24, 2022
@alexfauquette
Copy link
Member Author

But as @flaviendelangle mentioned in it, it would be best to also add this option to shouldDisableMonth and shouldDisableYear. -1

Why not, but shouldDisableMonth and shouldDisableYear are only used in month and year pickers which are not used in range picker

The helper validateDate only use shouldDisableDate as follow.

case Boolean(props.shouldDisableDate && props.shouldDisableDate(date)):
  return 'shouldDisableDate';

@flaviendelangle
Copy link
Member

For now indeed
That will come with #4331 at some point

@alexfauquette alexfauquette changed the title [POC] disable day on date range [DateRangePicker] disable day on date range Aug 25, 2022
* @returns {boolean} Returns `true` if the date should be disabled.
*/
shouldDisableDate?: (day: TDate) => boolean;
shouldDisableDate?: (day: TDate, position?: 'start' | 'end') => boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to only add it to the range interfaces
But that may add a lot of complexity :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wanted to comment on this as well. 🤔
Seems a bit counter intuitive to provide optional field for every picker, when only range ones have this field present.
I'll look into whether it would be feasible to have separate interfaces for range and other pickers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because this is propagated nearly everywhere. But I can give a second try to override it properly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a reason I ignore, it seems to work without any complex modification 🤯

I'm happy and suspicious at the same time because docs:API generate the expected output (one parameter for basic input, and two for range inputs) but I did not had to Omit shouldDisableDate anywhere 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to omit if you want to make this param non-nullable for range pickers

diff --git a/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerView.tsx b/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerView.tsx
index 3ec1c82fa..f80b83938 100644
--- a/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerView.tsx
+++ b/packages/x-date-pickers-pro/src/DateRangePicker/DateRangePickerView.tsx
@@ -40,7 +40,7 @@ export interface ExportedDateRangePickerViewProps<TDate>
   extends ExportedDesktopDateRangeCalendarProps<TDate>,
     Omit<
       ExportedCalendarPickerProps<TDate>,
-      'onYearChange' | 'renderDay' | keyof BaseDateValidationProps<TDate>
+      'onYearChange' | 'renderDay' | keyof BaseDateValidationProps<TDate> | 'shouldDisableDate'
     > {
   /**
    * Overrideable components.
@@ -81,7 +81,7 @@ export interface ExportedDateRangePickerViewProps<TDate>
    * @param {string} position The date to test, 'start' or 'end'.
    * @returns {boolean} Returns `true` if the date should be disabled.
    */
-  shouldDisableDate?: (day: TDate, position?: 'start' | 'end') => boolean;
+  shouldDisableDate?: (day: TDate, position: 'start' | 'end') => boolean;
 }
 
 interface DateRangePickerViewProps<TInputDate, TDate>
diff --git a/packages/x-date-pickers-pro/src/internal/hooks/validation/useDateRangeValidation.ts b/packages/x-date-pickers-pro/src/internal/hooks/validation/useDateRangeValidation.ts
index 969eb1f59..c61bbcce3 100644
--- a/packages/x-date-pickers-pro/src/internal/hooks/validation/useDateRangeValidation.ts
+++ b/packages/x-date-pickers-pro/src/internal/hooks/validation/useDateRangeValidation.ts
@@ -5,14 +5,12 @@ import {
   DateValidationError,
   validateDate,
   BaseDateValidationProps,
-  DayValidationProps,
 } from '@mui/x-date-pickers/internals';
 import { isRangeValid, parseRangeInputValue } from '../../utils/date-utils';
 import { DateRange } from '../../models';
 
 export interface DateRangeValidationProps<TInputDate, TDate>
-  extends DayValidationProps<TDate>,
-    Required<BaseDateValidationProps<TDate>>,
+  extends Required<BaseDateValidationProps<TDate>>,
     ValidationProps<DateRangeValidationError, DateRange<TInputDate>> {
   /**
    * Disable specific date. @DateIOType
@@ -21,7 +19,7 @@ export interface DateRangeValidationProps<TInputDate, TDate>
    * @param {string} position The date to test, 'start' or 'end'.
    * @returns {boolean} Returns `true` if the date should be disabled.
    */
-  shouldDisableDate?: (day: TDate, position?: 'start' | 'end') => boolean;
+  shouldDisableDate?: (day: TDate, position: 'start' | 'end') => boolean;
 }
 
 export const validateDateRange: Validator<any, DateRangeValidationProps<any, any>> = ({

I just have one issue which is that the doc generation looses the enum.
Most likely during the proptype generation phase:

@param {string} position The date to test, 'start' or 'end'.

But that's a different topic that probably impact other parts of our codebase and should be adressed

Copy link
Member

@flaviendelangle flaviendelangle Aug 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I would be in favor of having a DayRangeValidationProps containing only shouldDisableDate
And have the two interface with the range shouldDisableDate extend it.

To be consistent with DayValidationProps

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just have one issue which is that the doc generation looses the enum.
Most likely during the proptype generation phase:
@param {string} position The date to test, 'start' or 'end'.

The string does not come from the typescript definition but from the comment line

/**
 * @param {string} position The date to test, 'start' or 'end'.
 * /

I did that because using @param {'start' | 'end'} or

type RangePosition = 'start' | 'end'
/**
 * @param {RangePosition}
 */

ends up with an error in the doc generation saying the Template Literal Types are not supported by the documentation build, so instead of fixing the script I put string and listed the values.

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Very nice final result! 🚀 💯 ❤️

@alexfauquette alexfauquette merged commit 412d8db into mui:master Aug 26, 2022
@LukasTy LukasTy mentioned this pull request Sep 1, 2022
14 tasks
alexfauquette added a commit to alexfauquette/mui-x that referenced this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: DateRangePicker The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DateRangePicker] Separate "shouldDisableDate" for start date and end date
4 participants