-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Enhance date and time format settings to reflect system preferences #7274
Conversation
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.
PR Summary
This pull request enhances date and time format settings to reflect system preferences, improving user experience in the profile appearance settings.
- Added
detectSystemDateFormat
function inDateTimeSettingsDateFormatSelect.tsx
to display system's date format - Implemented
is12HourFormat
function inDateTimeSettingsTimeFormatSelect.tsx
to show current time in system format - Introduced
getSystemTimeZoneOption
inDateTimeSettingsTimeZoneSelect.tsx
to detect and display system time zone - Updated Select components in all three files to show more informative system setting options
- Improved value handling in
DateTimeSettingsTimeZoneSelect.tsx
for direct passing to Select component
3 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings
...-front/src/pages/settings/profile/appearance/components/DateTimeSettingsDateFormatSelect.tsx
Outdated
Show resolved
Hide resolved
const is12HourFormat = (timeZone: string): boolean => { | ||
const formattedTime = formatInTimeZone(new Date(), timeZone, 'hh:mm a'); | ||
return formattedTime.includes('AM') || formattedTime.includes('PM'); | ||
}; |
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.
style: Consider caching the result of is12HourFormat to avoid unnecessary calculations on each render
label: `System settings - ${systemTimeZoneOption?.label}`, | ||
value: 'system', |
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.
logic: Handle potential undefined systemTimeZoneOption
Hi! I was just working on this issue: #6880 I made the page reflect the wanted format: Sorry, this is my first time contributing to an open source and I wasn't sure how to submit this with the pull request. It worked well locally though! |
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.
Good approach I just replaced the new utils with the existing utils we already have for that.
…wentyhq#7274) Closes twentyhq#6880 --------- Co-authored-by: Lucas Bordeau <[email protected]>
Closes #6880