Add date form field #9021

Devessier · 2024-12-11T14:27:48Z

Add an option to hide the input from the packages/twenty-front/src/modules/ui/input/components/internal/date/components/AbsoluteDatePickerHeader.tsx component
Create a form field for dates
- Integrate the picker
- Create an input without a mask to let the user types a date and validate it by pressing the Enter key
Extract some utils to parse and format dates

greptile-apps

PR Summary

This PR adds comprehensive date form field functionality with both manual text input and date picker UI, including timezone-aware date handling and validation.

Added new FormDateFieldInput component in /packages/twenty-front/src/modules/object-record/record-field/form-types/components/FormDateFieldInput.tsx with unmasked date input validation and picker integration
Extracted date parsing logic into utility functions /packages/twenty-front/src/modules/ui/input/components/internal/date/utils/parseDateToString.ts and parseStringToDate.ts for consistent handling
Added US-centric date format constants (MM/dd/yyyy) in /packages/twenty-front/src/modules/ui/input/components/internal/date/constants/DateParserFormat.ts which may need internationalization
Moved state management up from DateInput to parent components for better control flow
Added comprehensive test coverage in FormDateFieldInput.stories.tsx including edge cases and validation scenarios

_{13 file(s) reviewed, 19 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-12-12T16:42:29Z

packages/twenty-front/src/modules/object-record/record-field/components/FormFieldInput.tsx

+  ) : isFieldDate(field) ? (
+    <FormDateFieldInput
+      label={field.label}
+      defaultValue={defaultValue as string | undefined}


logic: defaultValue should be typed as Date | string | undefined since dates can be passed as Date objects

greptile-apps · 2024-12-12T16:42:39Z

.../object-record/record-field/form-types/components/__stories__/FormDateFieldInput.stories.tsx

+      expect(args.onPersist).toHaveBeenCalledWith('2024-12-08T00:00:00.000Z');
+    });
+
+    expect(dialog).toBeVisible();


logic: Dialog should be removed after successful date entry - currently remains visible

greptile-apps · 2024-12-12T16:42:39Z

.../object-record/record-field/form-types/components/__stories__/FormDateFieldInput.stories.tsx

+    await Promise.all([
+      userEvent.type(input, '{Enter}'),
+


logic: Race condition possible between clear and Enter key - should verify input is actually empty before sending Enter

greptile-apps · 2024-12-12T16:42:44Z

...-front/src/modules/object-record/record-field/meta-types/input/components/DateFieldInput.tsx

+  const [temporaryValue, setTemporaryValue] =
+    useState<Nullable<Date>>(dateValue);


style: temporaryValue state is initialized with dateValue but not updated when fieldValue changes. Add useEffect to sync or document why this is intentional

greptile-apps · 2024-12-12T16:43:30Z

packages/twenty-front/src/modules/ui/field/input/components/DateInput.tsx

+        date={temporaryValue ?? new Date()}
+        onChange={handleChange}
+        onMouseSelect={handleMouseSelect}
+        clearable={clearable ? clearable : false}


style: redundant ternary - can be simplified to clearable || false

greptile-apps · 2024-12-12T16:45:00Z

packages/twenty-front/src/modules/ui/input/components/internal/date/utils/parseDateToString.ts

+    ? DATE_TIME_PARSER_FORMAT
+    : DATE_PARSER_FORMAT;
+
+  const dateParsed = DateTime.fromJSDate(date, { zone: userTimezone });


logic: potential timezone issue - if userTimezone is undefined, Luxon will use local timezone which may cause inconsistencies

greptile-apps · 2024-12-12T16:45:01Z

packages/twenty-front/src/modules/ui/input/components/internal/date/utils/parseDateToString.ts

+  const dateWithoutTime = DateTime.fromJSDate(date)
+    .toLocal()
+    .set({
+      day: date.getUTCDate(),
+      month: date.getUTCMonth() + 1,
+      year: date.getUTCFullYear(),
+      hour: 0,
+      minute: 0,
+      second: 0,
+      millisecond: 0,
+    });


logic: mixing UTC methods with toLocal() can lead to incorrect date conversions - consider using Luxon's built-in UTC handling instead of manual UTC getters

Not sure why we did that. Handling dates is really hard, and I experienced several bugs related to dates in the app. I think we'll have to work on that topic at some point.

greptile-apps · 2024-12-12T16:45:09Z

packages/twenty-front/src/modules/ui/input/components/internal/date/utils/parseStringToDate.ts

+    ? DateTime.fromFormat(dateAsString, parsingFormat, { zone: userTimezone })
+    : DateTime.fromFormat(dateAsString, parsingFormat, { zone: 'utc' });


logic: inconsistent timezone handling between date/datetime could cause issues - dates without times should probably use the user's timezone too to avoid off-by-one errors near midnight

greptile-apps · 2024-12-12T16:45:09Z

packages/twenty-front/src/modules/ui/input/components/internal/date/utils/parseStringToDate.ts

+  if (!isValid) {
+    return null;
+  }


style: should validate dateAsString is not empty/undefined before parsing to avoid unnecessary Luxon operations

greptile-apps · 2024-12-12T16:45:10Z

packages/twenty-front/src/modules/ui/input/components/internal/date/utils/parseStringToDate.ts

+  const parsedDate = isDateTimeInput
+    ? DateTime.fromFormat(dateAsString, parsingFormat, { zone: userTimezone })
+    : DateTime.fromFormat(dateAsString, parsingFormat, { zone: 'utc' });


style: consider adding try/catch around DateTime.fromFormat() as it can throw exceptions for malformed inputs

martmull

Nice code! Some comments

martmull · 2024-12-13T09:20:41Z

packages/twenty-front/src/modules/ui/field/input/components/DateInput.tsx

+  hideHeaderInput?: boolean;
+  temporaryValue: Nullable<Date>;
+  setTemporaryValue: (newValue: Nullable<Date>) => void;
+  wrapperRef: React.RefObject<HTMLElement>;


can you make temporaryValue, setTemporaryValue and wrapperRef arguments optional? Then you can leave the instanciations if props if they are undefined. It will avoid to update packages/twenty-front/src/modules/object-record/record-field/meta-types/input/components/DateTimeFieldInput.tsx

@charlesBochet wasn't satisfied with this code, and I asked him to take a look as he had an idea of the ideal API. Thanks for your suggestion, I know this part can be improved.

I'm not convinced by the name temporary value, but I think it's not a bad idea to let parents provide temporaryValue, setTemporaryValue, and wrapperRef. It makes the component more easily customizable. I find it easier than making the arguments optional; I think it would make the child component less understandable—I don't think having an "optional" useState is common.

I can change my mind if we agree there is no better solution!

...-front/src/modules/ui/input/components/internal/date/components/AbsoluteDatePickerHeader.tsx

martmull · 2024-12-13T09:27:23Z

...ges/twenty-front/src/modules/ui/input/components/internal/date/constants/DateParserFormat.ts

@@ -0,0 +1 @@
+export const DATE_PARSER_FORMAT = 'MM/dd/yyyy';


You can find the information in the database workspaceMember.dateFormat. The info is persisted in the recoil state dateTimeFormatState (see packages/twenty-front/src/modules/users/components/UserProviderEffect.tsx)

martmull · 2024-12-13T09:28:32Z

...twenty-front/src/modules/ui/input/components/internal/date/constants/DateTimeParserFormat.ts

@@ -0,0 +1 @@
+export const DATE_TIME_PARSER_FORMAT = 'MM/dd/yyyy HH:mm';


same for timeFormat

...ty-front/src/modules/object-record/record-field/form-types/components/FormDateFieldInput.tsx

thomtrp · 2024-12-13T09:43:36Z

...ty-front/src/modules/object-record/record-field/form-types/components/FormDateFieldInput.tsx

+// Inspired by the StyledInlineCellInput component.
+const StyledDateInputContainer = styled.div`
+  position: relative;
+  z-index: 1000;


I would put 998 :D

Didn't is z-index: 1 enough?

I didn't put much thought into it. As my comment says, I copied some part of the code of another component 😄 I will try to minimize the zIndex to 998 or 1.

I tried one, and it worked. That's less cool than 1000, though 😭

...ty-front/src/modules/object-record/record-field/form-types/components/FormDateFieldInput.tsx

thomtrp · 2024-12-13T09:50:43Z

...ty-front/src/modules/object-record/record-field/form-types/components/FormDateFieldInput.tsx

+    persistDate(newDate);
+  };
+
+  const handlePickerClickOutside = (


content should be common with handlePickerEscape

I wrote the same code three times because I had taken inspiration from the other date pickers. In our case, the behavior is sometimes different. In the case of the form field date input, the input's content mustn't be reset when the user clicks outside the picker or presses Escape.

Please note that pressing Escape doesn't work at all. I created an issue as this problem should be solved in the InternalDatePicker and also exists for the date picker used on the RecordTable. See #9056

thomtrp · 2024-12-13T09:51:18Z

...ty-front/src/modules/object-record/record-field/form-types/components/FormDateFieldInput.tsx

+    persistDate(null);
+  };
+
+  const handlePickerSubmit = (newDate: Nullable<Date>) => {


thomtrp · 2024-12-13T10:05:36Z

packages/twenty-front/src/modules/ui/field/input/components/DateInput.tsx

@@ -32,10 +30,13 @@ export type DateInputProps = {
  isDateTimeInput?: boolean;
  onClear?: () => void;
  onSubmit?: (newDate: Nullable<Date>) => void;
+  hideHeaderInput?: boolean;
+  temporaryValue: Nullable<Date>;


I don't really like temporaryValue as naming. It may make sense for your component but not for this one neither the other parents. Let's keep value or internalValue

I agree with you that this could be better. I kept my v1 as Charles said he knew about a better API.

Technically speaking, the internalValue is, in fact, a temporary value every time the DateInput component is used.

On the Record Table, the final value is set when the user clicks outside or press enter.

In my case, I must control this internal value from the outside, and it doesn't make much sense to call it internalValue if its parent provides it 🤔

As a consequence, I need clarification on the best API. @charlesBochet said he had a better solution in mind.

Be sure to check edge cases. Previously, the component was unmounted when the field was saved. Now, we keep it displayed. We need to figure out how to solve the involved edge cases.

…Enter

…and parseStringToDate functions to new files

…n empty string when the default value is a variable

…ker's event, even if that's not useful

…sing Esc

lucasbordeau · 2024-12-16T10:51:52Z

packages/twenty-front/src/modules/ui/field/input/components/DateInput.tsx

@@ -32,10 +30,13 @@ export type DateInputProps = {
  isDateTimeInput?: boolean;
  onClear?: () => void;
  onSubmit?: (newDate: Nullable<Date>) => void;
+  hideHeaderInput?: boolean;
+  temporaryValue: Nullable<Date>;
+  setTemporaryValue: (newValue: Nullable<Date>) => void;


Passing a setter as props is a code smell, it means either there is an inversion of layer of abstraction (child handling parent logic for example) or that we should instead use a Recoil state.

lucasbordeau · 2024-12-16T10:52:24Z

packages/twenty-front/src/modules/ui/field/input/components/DateInput.tsx

 }: DateInputProps) => {
-  const [internalValue, setInternalValue] = useState(value);


This should stay here

…ke the DateInput component fit my needs

github-actions · 2024-12-17T10:12:16Z

Thanks @Devessier for your contribution!
This marks your 35th PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Devessier force-pushed the add-date-form-field branch 2 times, most recently from 1ce4d95 to 83abb94 Compare December 12, 2024 13:11

Devessier marked this pull request as ready for review December 12, 2024 16:41

greptile-apps bot reviewed Dec 12, 2024

View reviewed changes

Devessier added the -PR: wip label Dec 12, 2024

martmull reviewed Dec 13, 2024

View reviewed changes

thomtrp reviewed Dec 13, 2024

View reviewed changes

Devessier force-pushed the add-date-form-field branch from 7bd6af6 to f08fa65 Compare December 13, 2024 13:24

Devessier added 21 commits December 16, 2024 10:31

feat: make a first implementation for the date field

a0b56dc

Be sure to check edge cases. Previously, the component was unmounted when the field was saved. Now, we keep it displayed. We need to figure out how to solve the involved edge cases.

feat: remove mask and use input's value as the new date whe pressing …

4f10a97

…Enter

refactor: remove useDateTimeInput hook and extract parseDateToString …

5ae1b2a

…and parseStringToDate functions to new files

fix: reset input date time when selecting a variable and default to a…

6a3e9c2

…n empty string when the default value is a variable

refactor: move handlers to dedicated functions

40813d3

refactor: move where React states are declared and remove useMemo

2492029

feat: ensure the input's date doesn't go beyond date limits

e0dc88b

fix: get user timezone from UserContext

3bbf078

fix: ensure we set the input date time in reaction for every date pic…

4972aa7

…ker's event, even if that's not useful

refactor: rename variable

92684b7

refactor: prefer a local override

d8b3e79

refactor: extract parser formats in constants

26b591a

refactor: simplify code

fd2a5e4

test: write tests for form date field input

83743c0

test: try to make date related tests pass on CI

e2c575f

refactor: simplify condition

0bed965

refactor: use a smaller zIndex and delete comment

70640f4

refactor: change editingMode to mode

e8a9856

refactor: default parameter + simplify jsx condition

9e9cd55

fix: keep the input state when clicking outside of the picker or pres…

a8e8ef1

…sing Esc

fix: use renamed components

f35ff7e

Devessier force-pushed the add-date-form-field branch from f08fa65 to f35ff7e Compare December 16, 2024 09:32

lucasbordeau reviewed Dec 16, 2024

View reviewed changes

Devessier added 2 commits December 16, 2024 17:36

revert: revert updates on the DateInput component

00d6ccc

refactor: directly use the InternalDatePicker instead of trying to ma…

246a4fb

…ke the DateInput component fit my needs

Devessier force-pushed the add-date-form-field branch from 7c8c9e3 to 246a4fb Compare December 16, 2024 16:49

revert: go back to previous version with untouched DateInput component

2d707ea

Devessier requested a review from lucasbordeau December 16, 2024 17:04

lucasbordeau approved these changes Dec 17, 2024

View reviewed changes

Devessier merged commit 5bd73e0 into main Dec 17, 2024
19 checks passed

Devessier deleted the add-date-form-field branch December 17, 2024 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add date form field #9021

Add date form field #9021

Devessier commented Dec 11, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

Devessier Dec 12, 2024

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

greptile-apps bot Dec 12, 2024

martmull left a comment

martmull Dec 13, 2024

Devessier Dec 13, 2024 •

edited

Loading

Devessier Dec 13, 2024

martmull Dec 13, 2024

martmull Dec 13, 2024

thomtrp Dec 13, 2024

Devessier Dec 13, 2024

Devessier Dec 13, 2024

thomtrp Dec 13, 2024

Devessier Dec 13, 2024

thomtrp Dec 13, 2024

thomtrp Dec 13, 2024

Devessier Dec 13, 2024 •

edited

Loading

Devessier Dec 13, 2024

lucasbordeau Dec 16, 2024 •

edited

Loading

lucasbordeau Dec 16, 2024

github-actions bot commented Dec 17, 2024

		const [temporaryValue, setTemporaryValue] =
		useState<Nullable<Date>>(dateValue);

		? DateTime.fromFormat(dateAsString, parsingFormat, { zone: userTimezone })
		: DateTime.fromFormat(dateAsString, parsingFormat, { zone: 'utc' });

		@@ -0,0 +1 @@
		export const DATE_PARSER_FORMAT = 'MM/dd/yyyy';

		@@ -0,0 +1 @@
		export const DATE_TIME_PARSER_FORMAT = 'MM/dd/yyyy HH:mm';

		}: DateInputProps) => {
		const [internalValue, setInternalValue] = useState(value);

Add date form field #9021

Add date form field #9021

Conversation

Devessier commented Dec 11, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 12, 2024

Choose a reason for hiding this comment

martmull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devessier Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Devessier Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasbordeau Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2024

Devessier commented Dec 11, 2024 •

edited

Loading

Devessier Dec 13, 2024 •

edited

Loading

Devessier Dec 13, 2024 •

edited

Loading

lucasbordeau Dec 16, 2024 •

edited

Loading