-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix: time format issue in datepicker mask #16922
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
base: main
Are you sure you want to change the base?
Fix: time format issue in datepicker mask #16922
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
packages/twenty-front/src/modules/localization/hooks/useDateTimeFormat.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/localization/utils/resolveTimeFormat.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/input/components/internal/date/utils/getDateTimeMask.ts
Outdated
Show resolved
Hide resolved
|
@vasu1303 Thanks for opening this PR—much appreciated. We only accept PRs with passing (green) tests. Since the overall direction looks good, I’ve added a few comments to help guide you. I’m going to close this PR for now because the tests are failing, but feel free to reopen it if you have the bandwidth to continue working on it. We’ll be happy to keep reviewing it. To reopen the PR, you can comment: |
|
/twenty pr open |
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:56153 This environment will automatically shut down when the PR is closed or after 5 hours. |
|
Hi @charlesBochet, sorry for the delay in getting back to this! I have now addressed all the feedback: I unified the date and time resolution using utilities and refactored the components to pass { dateFormat, timeFormat } as an object to improve the DevXP. I've also made sure the tests are passing. Let me know if this looks good! |
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
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.
No issues found across 10 files
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.
No issues found across 1930 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
|
@vasu1303 Nice work. After testing this, there's a small UX problem. The The input has been developed to not destroy any other part when modifying only a part of the date time input text. You can find the right lifecycle with the mask and the input to avoid this annoying effect. |
Enregistrement.de.l.ecran.2026-01-21.a.10.58.06.mov |
- Adds resolveDateFormat for consistency - Passes props as objects
f697442 to
18e42ee
Compare
| const parseJSDateToDateTimeInputString = (date: Date) => { | ||
| const parsingFormat = | ||
| getDateTimeFormatStringFoDatePickerInputMask(dateFormat); | ||
| if (!date || !isValid(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.
I wouldn't bring validation with the Date object, as we're trying to avoid its usage. Prefer using Temporal.
| from: isHour12 ? 1 : 0, | ||
| to: isHour12 ? 12 : 23, | ||
| maxLength: 2, | ||
| }, |
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.
Bug: The time input's AM/PM validation is case-sensitive, expecting AM/PM. However, date-fns formats time with lowercase am/pm, causing a mismatch and validation failure.
Severity: MEDIUM
Suggested Fix
To fix the case-sensitivity issue, either update the IMask.MaskedEnum to include lowercase variants ['AM', 'PM', 'am', 'pm'], or add a prepare hook to the 'aa' block definition in getTimeBlocks.ts to convert the input value to uppercase before validation, like prepare: (str) => str.toUpperCase().
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/modules/ui/input/components/internal/date/utils/getTimeBlocks.ts#L13
Potential issue: In 12-hour time format, the `IMask.MaskedEnum` for the AM/PM selector
is configured with `['AM', 'PM']`, making it case-sensitive. However, the `date-fns`
formatting pattern `'hh:mm a'` produces lowercase "am" and "pm". When a date is
programmatically set, the formatted value (e.g., "03:30 pm") will not match the mask's
enum, causing validation to fail. This also affects users who manually type "am" or "pm"
in lowercase. The component lacks a `prepare` hook or other normalization to handle case
differences.
Did we get this right? 👍 / 👎 to inform future reviews.
Fixes: #16872
Summary
Fixes the DateTimePicker to respect user's 12H/24H time format preference. Previously, the time display at the top of the DateTimePicker always showed 24-hour format (e.g., "14:30") regardless of the user's time format setting.
Screenshots
After: Time respects user preference, showing 12H with AM/PM (e.g., "03:28 PM") or 24H format

Implementation Details
Changes Made:
getTimeMask()to return appropriate mask pattern based on time formattimeFormatparametergetTimePattern()helper (returns 'hh:mm a' for 12H, 'HH:mm' for 24H)timeFormatparameter for consistencyTechnical Approach:
useDateTimeFormat()hook to get user'stimeFormatpreferenceTimeFormat.SYSTEM,TimeFormat.HOUR_12, andTimeFormat.HOUR_24