Add date time form field #9133
Add date time form field #9133
Conversation
Devessier
commented
Dec 18, 2024
•
Devessier
commented
- Create a really simple abstraction to unify the date and date time fields. We might dissociate them sooner than expected.
- The relative setting is ignored
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a new FormDateTimeFieldInput component and refactors date handling by extracting shared functionality into FormDateishFieldInputBase, enabling unified handling of both date and datetime fields.
- Incomplete Escape key handling in FormDateishFieldInputBase needs fixing (noted in code comment)
- InternalDatePicker's isDateTimeInput prop is hardcoded to false in FormDateishFieldInputBase, potentially affecting datetime functionality
- No error state handling for invalid date inputs in FormDateishFieldInputBase
- Timezone handling in FormDateTimeFieldInput.stories.tsx may need additional test coverage
- The relative setting is intentionally ignored as noted in PR description, which could affect date display functionality
5 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| return ( | ||
| <FormDateishFieldInputBase | ||
| mode="datetime" | ||
| placeholder="mm/dd/yyyy hh:mm" |
There was a problem hiding this comment.
style: placeholder format 'mm/dd/yyyy hh:mm' doesn't match ISO-8601 format used in defaultValue ('2024-12-09T13:20:19.631Z'). Consider documenting the format conversion or using consistent formats.
| ) : isFieldDateTime(field) ? ( | ||
| <FormDateTimeFieldInput | ||
| label={field.label} | ||
| defaultValue={defaultValue as string | undefined} |
There was a problem hiding this comment.
style: consider handling timezone conversion here since datetime values often need timezone awareness
| <FormDateishFieldInputBase | ||
| mode="date" | ||
| placeholder="mm/dd/yyyy" | ||
| {...{ label, defaultValue, onPersist, VariablePicker }} | ||
| /> |
There was a problem hiding this comment.
style: Props spreading could hide future prop changes in FormDateishFieldInputBase. Consider explicitly passing each prop.
| <OverlayContainer> | ||
| <InternalDatePicker | ||
| date={pickerDate ?? new Date()} | ||
| isDateTimeInput={false} |
There was a problem hiding this comment.
logic: isDateTimeInput is hardcoded to false but mode prop indicates this component handles both date and datetime
| persistDate(newDate); | ||
| }; | ||
|
|
||
| const handlePickerEnter = () => {}; |
There was a problem hiding this comment.
logic: empty handlePickerEnter function could lead to unexpected behavior when user presses Enter key in picker
| if (!isDefined(parsedInputDateTime)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
style: no feedback to user when invalid date is entered
| setDraftValue({ | ||
| type: 'static', | ||
| mode: 'edit', | ||
| value: newDate?.toDateString() ?? null, |
There was a problem hiding this comment.
logic: using toDateString() loses time information for datetime mode
| await waitFor(() => { | ||
| expect(args.onPersist).toHaveBeenCalledWith( | ||
| expect.stringMatching(/2024-12-08T\d{2}:10:00.000Z/), | ||
| ); |
There was a problem hiding this comment.
logic: The regex pattern \d{2}:10:00.000Z may be too specific and could fail in different timezones. Consider using a more flexible pattern that accounts for timezone variations.
