-
Notifications
You must be signed in to change notification settings - Fork 212
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
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.
LGTM 👍🏽
return ( | ||
<FtlMsg id="security-password-created-date" vars={{ date: pwdDateText }}> | ||
<FtlMsg id="security-password-created-date" vars={{ date: pwdDateFluent }}> |
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.
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.
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 believe that's what the getLocalizedDate
function is doing, providing a datetime that can be parsed by fluent:
fxa/packages/fxa-react/lib/utils.tsx
Lines 23 to 57 in be6538e
/** | |
* 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); | |
}; |
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.
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 :)
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.
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.
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.
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.
packages/fxa-settings/src/components/Settings/PageRecentActivity/en.ftl
Outdated
Show resolved
Hide resolved
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
41839f2
to
be6538e
Compare
Because
en
locale, regardless of browser language.This pull request
Issue that this pull request solves
Closes: #FXA-7795
Checklist
Put an
x
in the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Subscription Management
English
German
Settings Security
English
German
Security Events
English
German