-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Clean definition of validation props #15198
Conversation
6dd284a
to
3b73a0b
Compare
Deploy preview: https://deploy-preview-15198--material-ui-x.netlify.app/ |
3b73a0b
to
101427d
Compare
* 12h/24h view for hour selection clock. | ||
* @default utils.is12HourCycleInCurrentLocale() | ||
*/ | ||
ampm?: boolean; |
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 not strictly related to validation but I took the opportunity to also remove duplication on this prop.
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.
If I resume, you're introducing the following types to enforce consistency.
// Pro lib
ExportedValidateDateRangeProps
ExportedValidateTimeRangeProps
ExportedValidateDateTimeRangeProps
// MIT lib
ExportedValidateDateProps
ExportedValidateTimeProps
ExportedValidateDateTimeProps
// bonus
AmPmProps
I just left a comment about the notion of required props. But the rest looks good
@@ -62,8 +62,7 @@ export interface DateRangeCalendarSlotProps<TDate extends PickerValidDate> | |||
|
|||
export interface ExportedDateRangeCalendarProps<TDate extends PickerValidDate> | |||
extends ExportedDayCalendarProps<TDate>, | |||
BaseDateValidationProps<TDate>, |
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 BaseDateValidationProps
is still imported in this file because it's used to define DateCalendarDefaultizedProps
export type DateRangeCalendarDefaultizedProps<TDate extends PickerValidDate> = DefaultizedProps<
DateRangeCalendarProps<TDate>,
| 'views'
| 'openTo'
| 'reduceAnimations'
| 'calendars'
| 'disableDragEditing'
| 'availableRangePositions'
| keyof BaseDateValidationProps<TDate>
>;
Would it make sense to have type ExportedValidateDateRangePropsDefaultizeKey = keyof BaseDateValidationProps<TDate>
. to completely remove links with those notion of BaseDateValidationProps
, BaseTimeValidationProps
, ...
Seems their is already something similar with ExportedValidateTimeRangeProps
and ValidateTimeRangeProps
. The second being the defaultized version of the first
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.
That's an interesting point.
In #14679 I'm reworking how we apply the default values (and it will remove interfaces like DateRangeCalendarDefaultizedProps
), but I'm just moving the logic elsewhere (see https://github.com/mui/mui-x/pull/14679/files#diff-9b39de78b5e067c04513d22bde87e071d6adc77dc03839437ca392b977fc859dR42).
We could indeed introduce new interfaces next to the validator, or maybe even compare ExportedXXX
and XXX
and automatically list those which are optional in ExportedXXX
but required in XXX
🤔
I'll have a look in a follow up 👍
Exactly |
Extracted from #15194
We had some duplication of the props needed to validate a date, to validate a time etc...
In #15194, I want to stop using interfaces like
UseDateFieldProps
to generate interfaces likeDatePickerFieldProps
, and I don't want to duplicate yet another time those interfaces.So I added one interface per validator (e.g:
ExportedValidateDateProps
) that contains the props as they should be exposed by the component (picker, view, built-in field, custom field etc...). The next step would be to colocate the method to apply the default values, which are currently duplicated between fields and pickers (I did it in #14679 but I'll extract it of course).