-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: added disable future dates #3528
base: acceptance
Are you sure you want to change the base?
Conversation
Pandabweer
commented
Nov 11, 2024
•
edited
Loading
edited
- Added option to disable future dates to dateTimePicker & datePicker component
- Disabled future & past dates for timePicker
@@ -253,6 +254,7 @@ | |||
helperText={currentHelperText} | |||
disableToolbar={disableToolbar} | |||
disablePast={disablePastDates} | |||
disableFuture={disableFutureDates} |
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.
Is this feature really necessary? It feels too restricted. What if I want to restrict not dates after today, but rather dates after next week?
Maybe a better solution is to make better use of the MaxDate options.
E.g. maxDate = $today
Is that a possible solution?
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 could be a solution, but most of the time it's either the full future or the full past. I also wanted to follow the same style as the past. maxDate is also only if you'd want relative max date, if you'd want the complete future it would still be needed. This could also be applied for the past being too restricted as it is now.
How I see it, it would be an addition to the checkboxes.
@@ -0,0 +1,109 @@ | |||
import { component, PrefabReference } from '@betty-blocks/component-sdk'; |
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.
Why is this whole TimePicker structure added?
The DateTimePicker is capable to be used for: date, datetime and time selection.
There is also not a DatePicker structure, just a DateTimePicker structure. So I think this TimePicker structure is not necessary, and should be removed.
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.
Also, why is a TimePicker in the same change as maxDate? A time does not contain a date, so how it time-picker related to a maxDate?
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.
Why is this whole TimePicker structure added?
The DateTimePicker is capable to be used for: date, datetime and time selection.
There is also not a DatePicker structure, just a DateTimePicker structure. So I think this TimePicker structure is not necessary, and should be removed.
This is due to it not having disableFuture or the disablePast option. In the current prefab, timepicker has disablePast, but it doesn't apply anything. Due to it not doing anything, it shouldn't be included in the same prefab as datetime or date. The code can be the same because it ignores the flags.
Why this is in the same PR is because adding the checkbox disableFuture also adds it to the timepicker, which also doesn't work in this way.
condition: showIf('type', 'EQ', 'date'), | ||
}, | ||
}), | ||
datetimeFormat: text('Format', { |
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.
Do we need this option in the timepicker? since it says dateTimeFormat
condition: showIf('type', 'EQ', 'time'), | ||
}, | ||
}), | ||
dateFormat: text('Format', { |
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.
Do we need this option in the timepicker? since it says dateFormat
placeholder: variable('Placeholder', { value: [] }), | ||
helperText: variable('Helper text', { value: [] }), | ||
|
||
timeFormat: text('Format', { |
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.
Do we need this option in the timepicker? since the value contains MM/dd/yyyy which is a date value
}), | ||
|
||
type: text('Type', { | ||
value: 'datetime', |
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.
dateTime should be time here right?
Also should we even have a type if its only a time picker?
value: '', | ||
showInReconfigure: true, | ||
configuration: { | ||
allowedKinds: ['DATE'], |
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 should be TIME instead of DATE I think?
validationAfterMaxValue: variable('Value after maximum message', { | ||
value: ['Date should not be after maximal date'], | ||
configuration: { | ||
condition: showIf('type', 'EQ', 'date'), |
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 should never be the case in a time picker right?
value: [''], | ||
configuration: { | ||
allowFormatting: false, | ||
condition: showIf('type', 'EQ', 'date'), |
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 should never be the case in a time picker right?
validationInvalidValue: variable('Value invalid message', { | ||
value: ['Invalid date'], | ||
configuration: { | ||
condition: showIf('type', 'EQ', 'date'), |
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 should never be the case in a time picker right?
value: [''], | ||
configuration: { | ||
allowFormatting: false, | ||
condition: showIf('type', 'EQ', 'date'), |
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 should never be the case in a time picker right?
import { Configuration } from '../Configuration'; | ||
import { options as defaults } from './options'; | ||
|
||
export enum DateInputTypes { |
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 input types are not necessary, because the type is determined by the file right?