Separate date and datetime components #9161
Conversation
![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 separates the date and datetime form field components into distinct files, improving component organization and API clarity while maintaining existing functionality.
- Added new
FormDateFieldInputcomponent in/packages/twenty-front/src/modules/object-record/record-field/form-types/components/FormDateFieldInput.tsxthat wrapsFormDateTimeFieldInputwithdateOnlyprop - Replaced
FormDateTimeFieldInputBasewithFormDateTimeFieldInputin/packages/twenty-front/src/modules/object-record/record-field/form-types/components/FormDateTimeFieldInput.tsx, simplifying API by removing mode prop - Added comprehensive test coverage in new story files
/packages/twenty-front/src/modules/object-record/record-field/form-types/components/__stories__/FormDateFieldInput.stories.tsxandFormDateTimeFieldInput.stories.tsx - Updated
FormFieldInputcomponent to use separate date/datetime components instead of base component with mode
6 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| type FormDateFieldInputProps = { | ||
| label?: string; | ||
| defaultValue: string | undefined; |
There was a problem hiding this comment.
style: Consider making defaultValue nullable (string | undefined | null) to match onPersist's return type
| <FormDateFieldInput | ||
| label={field.label} | ||
| defaultValue={defaultValue as string | undefined} | ||
| onPersist={onPersist} | ||
| VariablePicker={VariablePicker} | ||
| /> |
There was a problem hiding this comment.
style: FormDateFieldInput wraps FormDateTimeFieldInput with dateOnly=true. Consider passing the onPersist type as (value: string | null) => void to match the child component's type signature.
| expect(args.onPersist).toHaveBeenCalledWith( | ||
| expect.stringMatching(/^2024-12-07/), | ||
| ); |
There was a problem hiding this comment.
logic: this regex pattern is too loose - could match invalid dates like '2024-12-07999'. Use exact ISO string match instead
| await Promise.all([ | ||
| userEvent.type(input, '02/02/1500{Enter}'), | ||
|
|
There was a problem hiding this comment.
style: consider testing multiple invalid date formats to ensure robust validation
| const defaultValueAsDisplayString = parseDateToString({ | ||
| date: new Date(args.defaultValue!), | ||
| isDateTimeInput: false, | ||
| userTimezone: undefined, | ||
| }); |
There was a problem hiding this comment.
logic: parseDateToString called with undefined userTimezone - could cause inconsistent behavior across different timezones
| const variableTag = await canvas.findByText('test'); | ||
| expect(variableTag).toBeVisible(); | ||
|
|
||
| const removeVariableButton = canvas.getByTestId(/^remove-icon/); |
There was a problem hiding this comment.
style: using regex for test ID matching could be fragile if IDs change
| await userEvent.type(input, '12/08/2024 12:10{enter}'); | ||
|
|
||
| await waitFor(() => { | ||
| expect(args.onPersist).toHaveBeenCalledWith( | ||
| expect.stringMatching(/2024-12-08T\d{2}:10:00.000Z/), | ||
| ); |
There was a problem hiding this comment.
logic: timezone mismatch potential - the regex pattern allows any hour value, which could cause flaky tests across different timezones
| @@ -143,6 +142,8 @@ export const FormDateTimeFieldInputBase = ({ | |||
| const displayDatePicker = | |||
| draftValue.type === 'static' && draftValue.mode === 'edit'; | |||
|
|
|||
| const placeholder = dateOnly ? 'mm/dd/yyyy' : 'mm/dd/yyyy hh:mm'; | |||
|
|
|||
| useListenClickOutside({ | |||
| refs: [datePickerWrapperRef], | |||
| listenerId: 'FormDateTimeFieldInputBase', | |||
There was a problem hiding this comment.
style: listenerId still references old component name 'FormDateTimeFieldInputBase'
| label?: string; | ||
| placeholder?: string; |
There was a problem hiding this comment.
style: placeholder prop is defined but never used since placeholder is defined internally on line 145
Add a 2 components solution for FormDate and FormDateTime
Add a 2 components solution for FormDate and FormDateTime
Add a 2 components solution for FormDate and FormDateTime