Skip to content
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

Date formatting per workspace member settings #6408

Merged
merged 16 commits into from
Jul 30, 2024

Conversation

lucasbordeau
Copy link
Contributor

Implement date formatting per workspace member settings

We'll need another round to maybe initialize all workspaces on the default settings.

For now the default behavior is to take system settings if nothing is found in DB.

Implement date formatting per workspace member settings

We'll need another round to maybe initialize all workspaces on the default settings.

For now the default behavior is to take system settings if nothing is found in DB.
@lucasbordeau lucasbordeau self-assigned this Jul 25, 2024
@lucasbordeau lucasbordeau marked this pull request as ready for review July 26, 2024 16:31
@lucasbordeau
Copy link
Contributor Author

@charlesBochet Do we have other places where we should take care of timezone ?

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 introduces date formatting based on workspace member settings, enhancing user customization and localization.

  • GraphQL Schema Updates: Added enums and fields for date and time formats in packages/twenty-front/src/generated/graphql.tsx.
  • New Utility Functions: Introduced multiple utility functions for detecting and formatting date and time in packages/twenty-front/src/modules/localization/utils/.
  • State Management: Added dateTimeFormatState.ts for managing date and time formatting settings.
  • Component Updates: Modified several components to use new date and time formatting settings, e.g., SettingsAccountsCalendarDisplaySettings.tsx.
  • File Reorganization: Moved and renamed files for better organization, such as IanaTimeZones.ts and detectTimeZone.ts.

Thorough testing is recommended to ensure compatibility and correctness across different locales and time zones.

30 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings

Comment on lines 123 to 125
user.workspaceMember.timeZone !== 'system'
? user.workspaceMember.timeZone ?? detectTimeZone()
: detectTimeZone(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Redundant check for user.workspaceMember.timeZone !== 'system'.

Comment on lines 123 to 125
user.workspaceMember.timeZone !== 'system'
? user.workspaceMember.timeZone ?? detectTimeZone()
: detectTimeZone(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Simplify time zone assignment logic.

@@ -0,0 +1,6 @@
export enum DateFormat {
SYSTEM = 'SYSTEM',
MONTH_FIRST = 'MMM d, yyyy', // US
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: The tab character in 'MMM\td, yyyy' should be a space for consistency.

Suggested change
MONTH_FIRST = 'MMM d, yyyy', // US
MONTH_FIRST = 'MMM d, yyyy',

const ianaTimeZoneParts = ianaTimeZone.split('/');
const location =
ianaTimeZoneParts.length > 1
? ianaTimeZoneParts.slice(-1)[0].replaceAll('_', ' ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spelling: Replace replaceAll with replace for broader compatibility with older environments.

Suggested change
? ianaTimeZoneParts.slice(-1)[0].replaceAll('_', ' ')
.replace(/_/g, ' ')

@charlesBochet charlesBochet assigned Weiko and unassigned lucasbordeau Jul 29, 2024
// TODO: factorize with UserProviderEffect
setDateTimeFormat({
timeZone:
user.workspaceMember.timeZone !== 'system'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be simplified imho

          timeZone:
            workspaceMember.timeZone && workspaceMember.timeZone !== 'system'
              ? workspaceMember.timeZone
              : detectTimeZone(),

@@ -0,0 +1,25 @@
import { DateFormat } from '@/localization/constants/DateFormat';

export const detectDateFormat = (): DateFormat => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit hacky, I think we can do that with Intl.DateTimeFormat(navigator.language) and analyse its parts with new Date() instead of parsing a raw date

* @param value the IANA time zone to match
* @returns the matching available IANA time zone select option or undefined
*/
export const findAvailableTimeZoneOption = (value: string) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one already exists in packages/twenty-front/src/modules/settings/accounts/utils/findAvailableTimeZoneOption.ts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we keep it in localization folder but remove the old one

// TODO: factorize
setDateTimeFormat({
timeZone:
workspaceMember.timeZone !== 'system'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

description: 'User time zone',
icon: 'IconTimezone',
})
@WorkspaceIsNullable()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we want it to be nullable, especially if we have a defaultValue

icon: 'IconCalendarEvent',
options: [
{
value: WorkspaceMemberDateFormatEnum.SYSTEM,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure with the term "system". Same for other fields. Could be confusing wether we are talking about server or client system

}

const logFilePath = await this.commandLogger.writeLog(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change intended? We are writing logs regardless of dry-run mode?

@Weiko Weiko merged commit ccf4d1e into main Jul 30, 2024
13 of 14 checks passed
@Weiko Weiko deleted the feat/timezone-workspace-member branch July 30, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants