Skip to content

LG-10302: Change Address Search i18n lib#9109

Merged
allthesignals merged 2 commits intomainfrom
wmg/10302-change-i18n
Aug 29, 2023
Merged

LG-10302: Change Address Search i18n lib#9109
allthesignals merged 2 commits intomainfrom
wmg/10302-change-i18n

Conversation

@allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Aug 28, 2023

🎫 Ticket

🛠 Summary of changes

Swap in less React-ish i18n lib so consumer app (Help Center) can more simply supply its own translations.

📜 Testing Plan

Refactor should result in no changes.

  • Local visual check that translations in address search and location-collection-item work correctly.

@allthesignals allthesignals requested review from a team and gina-yamada August 28, 2023 22:44
Comment on lines -248 to +252
<I18nContext.Provider
value={
new I18n({
strings: {
'in_person_proofing.body.location.po_search.results_description': {
one: 'There is one participating Post Office within 50 miles of %{address}.',
other:
'There are %{count} participating Post Offices within 50 miles of %{address}.',
},
},
})
}
>
context('pluralized and singularized translations are set', () => {
usePropertyValue(i18n, 'strings', {
'in_person_proofing.body.location.po_search.results_description': {
one: 'There is one participating Post Office within 50 miles of %{address}.',
other: 'There are %{count} participating Post Offices within 50 miles of %{address}.',
},
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we aren't using the Provider to set the translation values, we instead override the locale data in the i18n global using usePropertyValue test helper, which resets the original values later.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM

{
"name": "@18f/identity-address-search",
"version": "1.0.5",
"version": "2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change expected here? I suppose it shouldn't matter much if it's already in main via #9103.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth Weird, I don't know why it appears in the diff. It's not expected because we already merged it. Maybe I branched off #9103 or something.

@allthesignals allthesignals merged commit 535bfaa into main Aug 29, 2023
@allthesignals allthesignals deleted the wmg/10302-change-i18n branch August 29, 2023 14:01
@mdiarra3 mdiarra3 mentioned this pull request Aug 31, 2023
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