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

[MPDX-8442] Localize more labels #1193

Merged
merged 10 commits into from
Nov 14, 2024
Merged

[MPDX-8442] Localize more labels #1193

merged 10 commits into from
Nov 14, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Nov 12, 2024

Description

Localize nearly all of the labels reported by Scott in the screenshots here: https://secure.helpscout.net/conversation/2761067138/1258312

The impersonation message doesn't appear to be translated yet in OneSkey, so that one isn't fixed in this PR. The action filters need to be localized by the server, so that one isn't fixed in this PR either.

@dr-bizz Any chance you remember why the "All Tasks" label was explicitly not localized before? I removed that logic, but let me know if there's a reason to keep it.

MPDX-8442

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@canac canac added the Preview Environment Add this label to create an Amplify Preview label Nov 12, 2024
@canac canac requested a review from dr-bizz November 12, 2024 21:17
@canac canac self-assigned this Nov 12, 2024
Copy link
Contributor

Preview branch generated at https://8442-localize-labels.d3dytjb8adxkk5.amplifyapp.com

Copy link
Contributor

github-actions bot commented Nov 12, 2024

Bundle sizes [mpdx-react]

Compared against d7a008e

Route Size (gzipped) Diff
/accountLists/[accountListId]/reports/donations/[[...contactId]] 349.04 KB +1.33 KB
/accountLists/[accountListId]/reports/financialAccounts/[financialAccountId]/entries 296.01 KB +1.35 KB
/accountLists/[accountListId]/reports/partnerGivingAnalysis/[[...contactId]] 136.51 KB +1.35 KB
/accountLists/[accountListId]/tools/appeals 191.7 KB +4.01 KB
Dynamic import Size (gzipped) Diff
../src/components/Contacts/ContactDetails/ContactDonationsTab/DynamicContactDonationsTab.tsx -> ./ContactDonationsTab 191.58 KB +1.33 KB
../src/components/Tool/Appeal/Modals/UpdateDonationsModal/DynamicUpdateDonationsModal.tsx -> ./UpdateDonationsModal 105.51 KB +1.33 KB

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Great work! Thank you for improving MPDX

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should consoladate the useLocations hooks, so we have one? It means there will be less to maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The location options for email addresses and phone numbers are similar but different. That's why I have two files.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of passing in a value of email or phone so that you get the correct locations back, so you only have to maintain one file. It's not a biggy, so I won't die on the hill.

src/components/DonationTable/DonationTable.tsx Outdated Show resolved Hide resolved
@canac canac requested a review from dr-bizz November 13, 2024 17:32
@canac
Copy link
Contributor Author

canac commented Nov 13, 2024

The list of default Data Grid labels are defined here: https://github.com/mui/mui-x/blob/v5.17.4/packages/grid/x-data-grid/src/constants/localeTextConstants.ts. I copy/pasted them and converted each one to a t call.

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Thank you for adding the translations for the table and DataGrid and putting the locations hooks in the global hooks folder.

Could you also run extract and upload the new phrases to OneSky so Scott can work on translating them?

@canac canac force-pushed the 8442-localize-labels branch from 9dcc2e2 to ea4b566 Compare November 14, 2024 17:15
@canac
Copy link
Contributor Author

canac commented Nov 14, 2024

Could you also run extract and upload the new phrases to OneSky so Scott can work on translating them?

@dr-bizz Thanks for the reminder and for the code review feedback!

@canac canac merged commit c0745fa into main Nov 14, 2024
17 of 18 checks passed
@canac canac deleted the 8442-localize-labels branch November 14, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants