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(l10n): fix date localization in sub management #15459

Merged
merged 1 commit into from
Jun 20, 2023

Conversation

StaberindeZA
Copy link
Contributor

Because

  • Subscription management date is localized to en locale, regardless of browser language.
  • SecurityEvent created date is not being localized.

This pull request

  • Pass FluentDate to Fluent localization libraries to correctly localize dates.
  • Add localization string for SecurityEvent created date.

Issue that this pull request solves

Closes: #FXA-7795

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Subscription Management

English
image

German
image

Settings Security

English
image

German
image

Security Events

English
image

German
image

@StaberindeZA StaberindeZA requested review from a team as code owners June 16, 2023 17:51
Copy link
Contributor

@vbudhram vbudhram left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

return (
<FtlMsg id="security-password-created-date" vars={{ date: pwdDateText }}>
<FtlMsg id="security-password-created-date" vars={{ date: pwdDateFluent }}>
Copy link
Contributor

@vpomerleau vpomerleau Jun 16, 2023

Choose a reason for hiding this comment

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

This might be a question for @bcolsson, but wouldn't Fluent handle localization of the date if we passed in a date variable instead of a string?

According to Fluent's documentation :

If the variable passed from the developer is a date and is used in a placeable, FTL will implicitly call a DATETIME function on it.

In most cases, Fluent will automatically select the right formatter for the argument and format it into a given locale.

https://projectfluent.org/fluent/guide/builtins.html

Copy link
Contributor

@bcolsson bcolsson Jun 16, 2023

Choose a reason for hiding this comment

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

I believe that's what the getLocalizedDate function is doing, providing a datetime that can be parsed by fluent:

/**
* This method is used to provide Fluent with a localizable value that can be formatted per .ftl file based on localization requirements
*
* @param milliseconds
* @param numericDate
*/
export const getLocalizedDate = (
milliseconds: number,
dateOptions: LocalizedDateOptions
): FluentDateTime => {
let options: Intl.DateTimeFormatOptions | undefined;
switch (dateOptions) {
case LocalizedDateOptions.NumericDate:
options = {
day: 'numeric',
month: 'numeric',
year: 'numeric',
};
break;
case LocalizedDateOptions.NumericDateAndTime:
options = {
year: 'numeric',
month: 'numeric',
day: 'numeric',
hour: 'numeric',
minute: 'numeric',
};
break;
default:
options = undefined;
}
return new FluentDateTime(milliseconds, options);
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I see that we're using this to pass localization options with the date.

Equivalent to passing the options in more explicitly, as documented? @bcolsson would it be preferable to expose these options to localizers so they can have visibility on the date formatting? If not like below, then at least in a comment?
today-is = Today is { DATETIME($date, month: "long", year: "numeric", day: "numeric") }

@StaberindeZA this is non-blocking BTW - I'm somewhat hijacking your PR to get more details on how to handle date localization in the future :)

Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of things like correct date and number formatting, typically it is expected that the internationalization layer should handle the correct formatting for each locale without intervention by the linguists.

Copy link
Contributor

Choose a reason for hiding this comment

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

That being said, I see your point regarding how a date is displayed. We could definitely add a comment if you wanted to include how it appears.

Because:

* Subscription management date is localized to `en` locale, regardless
  of browser language.
* SecurityEvent created date is not being localized.

This commit:

* Pass FluentDate to Fluent localization libraries to correctly
  localize dates.
* Add localization string for SecurityEvent created date.

Closes FXA-7795
@StaberindeZA StaberindeZA force-pushed the fxa-7795-fix-date-localization branch from 41839f2 to be6538e Compare June 16, 2023 19:33
@sardesam sardesam self-assigned this Jun 20, 2023
@StaberindeZA StaberindeZA merged commit 0b24259 into main Jun 20, 2023
@StaberindeZA StaberindeZA deleted the fxa-7795-fix-date-localization branch June 20, 2023 18:39
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.

5 participants