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

fix: Date formatting and language translation for other regions #10385

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

KokeroO
Copy link
Contributor

@KokeroO KokeroO commented Nov 3, 2024

Pull Request Template

Description

I decided to close PR #10360 and split it into 2 separate PRs.

  1. When applying a new language setting from the account settings, date and time formatting is applied to the language of choice. Previously, only the 'en' localization was applied. This caused confusion and doubt for the end user.
  2. Other changes have also been applied to translate texts that were not handled by the i18n library ​​that contain region code.

Resolves #7862, #9285, #10327

Type of change

  • Breaking change (fix or feature that would cause existing functionality not to work as expected)

How Has This Been Tested?

  1. Go to app.chatwoot.com/app/accounts/1/settings/general and change the account language.
  2. All pages showing dates will be in the correct format for your locale.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@KokeroO
Copy link
Contributor Author

KokeroO commented Nov 4, 2024

@scmmishra
#10360 (review)

  1. Fixed
  2. It is fixed
  3. I couldn't bring it to the vue context, the only better way I found was this. Maybe if you implement the script within the vue context.
  4. Datepicker has the problem that it does not use the language chosen by the user. It would be possible to use window to set values ​​from within Vue, but in the helper we would have to use setInterval() to update the value inside the helper, since the value does not update itself.
    It would be possible to use MutationObserver(), but this wastes unnecessary resources to update a single value instead of looking at the changes in the entire DOM.

In short it is now working.

I thank.

@scmmishra
Copy link
Member

Hey @KokeroO this looks great

  • I couldn't bring it to the vue context, the only better way I found was this. Maybe if you implement the script within the vue context.

For this perhaps, you could make file the following file app/javascript/dashboard/composables/useLocaleFormatter.js

import { useI18n } from 'vue-i18n';
import {
  messageStamp,
  messageTimestamp,
  dynamicTime,
  dateFormat,
  shortTimestamp,
} from 'shared/helpers/timeHelper';

export default function useLocaleFormatter() {
  const { locale } = useI18n();

  const localeDynamicTime = (...args) => dynamicTime(...args, locale.value);
  const localeDateFormat = (...args) => dateFormat(...args, locale.value);
  const localeShortTimestamp = (...args) =>
    shortTimestamp(...args, locale.value);
  const localeMessageStamp = (...args) => messageStamp(...args, locale.value);
  const localeMessageTimestamp = (...args) =>
    messageTimestamp(...args, locale.value);

  return {
    localeDynamicTime,
    localeDateFormat,
    localeShortTimestamp,
    localeMessageStamp,
    localeMessageTimestamp,
  };
}

This can be used anywhere in the setup context as

const { localeDateFormat } = useLocaleFormatter();

KokeroO added a commit to KokeroO/ConnectDesk that referenced this pull request Nov 4, 2024
@KokeroO
Copy link
Contributor Author

KokeroO commented Nov 5, 2024

@pranavrajs
I have some doubts regarding dateFormat, messageStamp and messageTimestamp.
They confuse me, originally in certain situations they give the same result.
Could you take a look? I could unify them.

I just need to fix timeHelper.js and timeHelper.spec.js.

app/javascript/shared/helpers/specs/timeHelper.spec.js
app/javascript/shared/helpers/timeHelper.js

@scmmishra
Copy link
Member

Could you take a look? I could unify them.

Yeah, seems like they can be combined. Just need to handle the isSameYear case in messageTimestamp. But we can combine dateFormat and messageStamp for sure

@KokeroO
Copy link
Contributor Author

KokeroO commented Nov 6, 2024

Could you take a look? I could unify them.

Yeah, seems like they can be combined. Just need to handle the isSameYear case in messageTimestamp. But we can combine dateFormat and messageStamp for sure

@scmmishra
Thanks for the tips. I was able to implement it and do the best I could. This PR will greatly help other languages ​​and I believe that the dates are now better represented.

  1. The language setting that contained the region code after the "-" now actually works.

  2. I merged messageStamp with dateFormat, which I found gave the same results on different screens.

  3. I renamed messageTimestamp to messageDateFormat to handle date formatting in message bubbles and validate the isSameYear() and isSameDay() methods.
    image

@scmmishra
Copy link
Member

@KokeroO this looks great! I'll review this ASAP and let you know

@Micorksen
Copy link

Hey! I see that my issue was mentionned here.
In fact, my issue is about the help center and wasn't really about the agent UI...

@KokeroO
Copy link
Contributor Author

KokeroO commented Nov 18, 2024

@Micorksen The help center will also be covered in this PR.

@Micorksen
Copy link

Hey @pranavrajs @sojan-official, can someone please accept this PR?

Copy link

🐢 Turtley slow progress alert! This pull request has been idle for over 30 days. Can we please speed things up and either merge it or release it back into the wild?

@github-actions github-actions bot added the stale label Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CW-2457] fix: Make activity timestamp messages translatable
3 participants