Add date time form field #9133

Devessier · 2024-12-18T17:17:16Z

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

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}

greptile-apps · 2024-12-18T17:33:36Z

...ront/src/modules/object-record/record-field/form-types/components/FormDateTimeFieldInput.tsx

+  return (
+    <FormDateishFieldInputBase
+      mode="datetime"
+      placeholder="mm/dd/yyyy hh:mm"


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.

greptile-apps · 2024-12-18T17:33:36Z

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

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


style: consider handling timezone conversion here since datetime values often need timezone awareness

greptile-apps · 2024-12-18T17:33:38Z

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

+    <FormDateishFieldInputBase
+      mode="date"
+      placeholder="mm/dd/yyyy"
+      {...{ label, defaultValue, onPersist, VariablePicker }}
+    />


style: Props spreading could hide future prop changes in FormDateishFieldInputBase. Consider explicitly passing each prop.

greptile-apps · 2024-12-18T17:33:41Z

...t/src/modules/object-record/record-field/form-types/components/FormDateishFieldInputBase.tsx

+                    <OverlayContainer>
+                      <InternalDatePicker
+                        date={pickerDate ?? new Date()}
+                        isDateTimeInput={false}


logic: isDateTimeInput is hardcoded to false but mode prop indicates this component handles both date and datetime

greptile-apps · 2024-12-18T17:33:42Z

...t/src/modules/object-record/record-field/form-types/components/FormDateishFieldInputBase.tsx

+    persistDate(newDate);
+  };
+
+  const handlePickerEnter = () => {};


logic: empty handlePickerEnter function could lead to unexpected behavior when user presses Enter key in picker

greptile-apps · 2024-12-18T17:33:43Z

...t/src/modules/object-record/record-field/form-types/components/FormDateishFieldInputBase.tsx

+    if (!isDefined(parsedInputDateTime)) {
+      return;
+    }


style: no feedback to user when invalid date is entered

greptile-apps · 2024-12-18T17:33:43Z

...t/src/modules/object-record/record-field/form-types/components/FormDateishFieldInputBase.tsx

+    setDraftValue({
+      type: 'static',
+      mode: 'edit',
+      value: newDate?.toDateString() ?? null,


logic: using toDateString() loses time information for datetime mode

greptile-apps · 2024-12-18T17:34:12Z

...ect-record/record-field/form-types/components/__stories__/FormDateTimeFieldInput.stories.tsx

+    await waitFor(() => {
+      expect(args.onPersist).toHaveBeenCalledWith(
+        expect.stringMatching(/2024-12-08T\d{2}:10:00.000Z/),
+      );


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.

martmull

Hey nice code. I think the components logic can be simplified. Delete FormDateFieldInput.tsx and FormDateTimeFieldInput.tsx then rename FormDateishFieldInputBase.tsx into FormDateTimeFieldInput.tsx
In FormFieldInput.tsx use <FormDateTimeFieldInput mode='datetime' ...> and <FormDateTimeFieldInput mode='date' ...>

martmull · 2024-12-19T07:45:51Z

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

+    <FormDateishFieldInputBase
+      mode="date"
+      placeholder="mm/dd/yyyy"
+      {...{ label, defaultValue, onPersist, VariablePicker }}


no props spreading in project

martmull · 2024-12-19T07:58:42Z

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

-        ) : null}
-      </FormFieldInputRowContainer>
-    </FormFieldInputContainer>
+    <FormDateishFieldInputBase


Looks like we don't need that component

martmull · 2024-12-19T07:58:54Z

...ront/src/modules/object-record/record-field/form-types/components/FormDateTimeFieldInput.tsx

+  VariablePicker?: VariablePickerComponent;
+};
+
+export const FormDateTimeFieldInput = ({


Looks like we don't need that component

martmull · 2024-12-19T08:01:57Z

...t/src/modules/object-record/record-field/form-types/components/FormDateishFieldInputBase.tsx

+  VariablePicker?: VariablePickerComponent;
+};
+
+export const FormDateishFieldInputBase = ({


This naming is not very clear. See my global comment

Suggested change

export const FormDateishFieldInputBase = ({

export const FormDateTimeFieldInput = ({

thomtrp · 2024-12-19T09:40:56Z

Hey nice code. I think the components logic can be simplified. Delete FormDateFieldInput.tsx and FormDateTimeFieldInput.tsx then rename FormDateishFieldInputBase.tsx into FormDateTimeFieldInput.tsx In FormFieldInput.tsx use <FormDateTimeFieldInput mode='datetime' ...> and <FormDateTimeFieldInput mode='date' ...>

+1 on this

martmull

LGTM thanks

github-actions · 2024-12-19T10:47:13Z

	Fails
🚫	`node` failed.

Log

�[31mError: �[39m RequestError [HttpError]: You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 7A40:320300:117C3E0:22CC7F6:6763F9AF.
    at /home/runner/work/twenty/twenty/node_modules/�[4m@octokit�[24m/request/dist-node/index.js:86:21
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m {
  status: �[33m403�[39m,
  response: {
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3ADevessier%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    status: �[33m403�[39m,
    headers: {
      �[32m'access-control-allow-origin'�[39m: �[32m'*'�[39m,
      �[32m'access-control-expose-headers'�[39m: �[32m'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Resource, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, X-GitHub-SSO, X-GitHub-Request-Id, Deprecation, Sunset'�[39m,
      connection: �[32m'close'�[39m,
      �[32m'content-encoding'�[39m: �[32m'gzip'�[39m,
      �[32m'content-security-policy'�[39m: �[32m"default-src 'none'"�[39m,
      �[32m'content-type'�[39m: �[32m'application/json; charset=utf-8'�[39m,
      date: �[32m'Thu, 19 Dec 2024 10:47:11 GMT'�[39m,
      �[32m'referrer-policy'�[39m: �[32m'origin-when-cross-origin, strict-origin-when-cross-origin'�[39m,
      server: �[32m'github.com'�[39m,
      �[32m'strict-transport-security'�[39m: �[32m'max-age=31536000; includeSubdomains; preload'�[39m,
      �[32m'transfer-encoding'�[39m: �[32m'chunked'�[39m,
      vary: �[32m'Accept-Encoding, Accept, X-Requested-With'�[39m,
      �[32m'x-content-type-options'�[39m: �[32m'nosniff'�[39m,
      �[32m'x-frame-options'�[39m: �[32m'deny'�[39m,
      �[32m'x-github-api-version-selected'�[39m: �[32m'2022-11-28'�[39m,
      �[32m'x-github-media-type'�[39m: �[32m'github.v3; format=json'�[39m,
      �[32m'x-github-request-id'�[39m: �[32m'7A40:320300:117C3E0:22CC7F6:6763F9AF'�[39m,
      �[32m'x-ratelimit-limit'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-remaining'�[39m: �[32m'30'�[39m,
      �[32m'x-ratelimit-reset'�[39m: �[32m'1734605291'�[39m,
      �[32m'x-ratelimit-resource'�[39m: �[32m'search'�[39m,
      �[32m'x-ratelimit-used'�[39m: �[32m'1'�[39m,
      �[32m'x-xss-protection'�[39m: �[32m'0'�[39m
    },
    data: {
      documentation_url: �[32m'https://docs.github.com/free-pro-team@latest/rest/overview/rate-limits-for-the-rest-api#about-secondary-rate-limits'�[39m,
      message: �[32m'You have exceeded a secondary rate limit. Please wait a few minutes before you try again. If you reach out to GitHub Support for help, please include the request ID 7A40:320300:117C3E0:22CC7F6:6763F9AF.'�[39m
    }
  },
  request: {
    method: �[32m'GET'�[39m,
    url: �[32m'https://api.github.com/search/issues?q=is%3Apr%20author%3ADevessier%20is%3Aclosed%20repo%3Atwentyhq%2Ftwenty&per_page=2&page=1'�[39m,
    headers: {
      accept: �[32m'application/vnd.github.v3+json'�[39m,
      �[32m'user-agent'�[39m: �[32m'octokit-rest.js/18.12.0 octokit-core.js/3.6.0 Node.js/18.20.5 (linux; x64)'�[39m,
      authorization: �[32m'token [REDACTED]'�[39m
    },
    request: { hook: �[36m[Function: bound bound register]�[39m }
  }
}
danger-results://tmp/danger-results-4680d19a.json

Generated by 🚫 dangerJS against 0ee0331

Devessier added 2 commits December 18, 2024 17:44

feat: implement the date time form field

e464924

test: write tests for form date time field input

13f00e0

Devessier marked this pull request as ready for review December 18, 2024 17:32

greptile-apps bot reviewed Dec 18, 2024

View reviewed changes

charlesBochet added the -PR: awaiting review label Dec 18, 2024

charlesBochet assigned Devessier Dec 18, 2024

martmull requested changes Dec 19, 2024

View reviewed changes

Devessier added 2 commits December 19, 2024 11:04

refactor: explicit provide props

cc59dac

refactor: combine date and datetime fields

0ee0331

martmull reviewed Dec 19, 2024

View reviewed changes

martmull approved these changes Dec 19, 2024

View reviewed changes

Devessier enabled auto-merge (squash) December 19, 2024 10:42

Devessier merged commit 65586a0 into main Dec 19, 2024
22 checks passed

Devessier deleted the add-date-time-form-field branch December 19, 2024 10:46

martmull restored the add-date-time-form-field branch December 20, 2024 09:20

martmull deleted the add-date-time-form-field branch December 20, 2024 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add date time form field #9133

Add date time form field #9133

Devessier commented Dec 18, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Dec 18, 2024

greptile-apps bot Dec 18, 2024

greptile-apps bot Dec 18, 2024

greptile-apps bot Dec 18, 2024

greptile-apps bot Dec 18, 2024

greptile-apps bot Dec 18, 2024

greptile-apps bot Dec 18, 2024

greptile-apps bot Dec 18, 2024

martmull left a comment

martmull Dec 19, 2024

martmull Dec 19, 2024

martmull Dec 19, 2024

martmull Dec 19, 2024

thomtrp commented Dec 19, 2024

martmull left a comment

github-actions bot commented Dec 19, 2024

	export const FormDateishFieldInputBase = ({
	export const FormDateTimeFieldInput = ({

Add date time form field #9133

Add date time form field #9133

Conversation

Devessier commented Dec 18, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 18, 2024

Choose a reason for hiding this comment

martmull left a comment

Choose a reason for hiding this comment

martmull Dec 19, 2024

Choose a reason for hiding this comment

martmull Dec 19, 2024

Choose a reason for hiding this comment

martmull Dec 19, 2024

Choose a reason for hiding this comment

martmull Dec 19, 2024

Choose a reason for hiding this comment

thomtrp commented Dec 19, 2024

martmull left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 19, 2024

Log

Devessier commented Dec 18, 2024 •

edited

Loading