🌍 improve translation strings - part 2 #4154
Conversation
actual-github-bot
bot
changed the title
|
/update-vrt |
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
matt-fidd
force-pushed
the
translations-2
branch
2 times, most recently
from
bb884f7 to
f80d091
Compare
|
/update-vrt |
...ts/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-1-chromium-linux.png
Outdated
Show resolved
Hide resolved
matt-fidd
force-pushed
the
translations-2
branch
from
1373116 to
397d0be
Compare
matt-fidd
changed the title
WalkthroughThis pull request encompasses a comprehensive set of changes across multiple components in the desktop client, focusing primarily on internationalization (i18n) and text formatting improvements. The modifications span various files within the The key areas of modification include:
These changes aim to improve the overall user interface consistency, localization support, and text presentation without altering the fundamental functionality of the components. Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (1)
104-106: LGTM! Consider one minor consistency improvement.The error message changes appropriately specify "budget file" instead of just "budget", which aligns with the PR's goal of improving translation string clarity and consistency.
For complete consistency, consider updating the error message in the notification dispatch as well:
dispatch( addNotification({ type: 'error', - message: t('Failed to duplicate budget file.'), + message: t('Failed to duplicate budget file'), }), );This removes the trailing period to match the format of the other error messages.
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (1)
974-976: Consider standardizing error message handling across the component.While this specific error message is well-formatted for translation, other error messages in the component (e.g., schedule saving error with URL) could benefit from the same named interpolation pattern for consistency.
Example of consistent error message formatting:
- error: t('An error occurred while saving. Please visit https://actualbudget.org/contact/ for support.'), + error: t('An error occurred while saving. Please visit {{supportUrl}} for support.', { supportUrl: 'https://actualbudget.org/contact/' }),packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx (1)
81-89: Good use of Trans component, with minor optimization opportunitiesThe transition to using the
Transcomponent with proper interpolation is a good improvement for translation handling. However, there are a couple of potential optimizations:
- Consider extracting the '28+' translation outside the interpolation:
+ const dayPlus = t('28+'); <Trans> Day:{' '} {{ dayOfMonth: Number(payload[0].payload.day) >= 28 - ? t('28+') + ? dayPlus : payload[0].payload.day, }} </Trans>
- Consider extracting the magic number 28 into a named constant for better maintainability:
+ const LAST_DAY_THRESHOLD = 28; <Trans> Day:{' '} {{ dayOfMonth: - Number(payload[0].payload.day) >= 28 + Number(payload[0].payload.day) >= LAST_DAY_THRESHOLD ? dayPlus : payload[0].payload.day, }} </Trans>packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx (2)
143-143: Consider usingTranscomponent for modal title consistency.While using
t('Settings')works correctly, consider using theTranscomponent for consistency with other translated strings in this file:-title={t('Settings')} +title={<Trans>Settings</Trans>}This would maintain a consistent approach to translations throughout the component.
Also applies to: 157-157
Line range hint
143-190: Consider enhancing keyboard accessibility for scrollable content.The file paths are displayed in scrollable containers. Consider adding keyboard navigation support to allow users to scroll these containers using arrow keys. This would improve accessibility for keyboard users.
Example implementation:
function handleKeyDown(e: React.KeyboardEvent) { const container = e.currentTarget; if (e.key === 'ArrowLeft') { container.scrollLeft -= 50; } else if (e.key === 'ArrowRight') { container.scrollLeft += 50; } }Then add to the scrollable Text component:
<Text title={documentDir} + tabIndex={0} + onKeyDown={handleKeyDown} style={{ /* existing styles */ }} >packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx (1)
78-80: Consider standardizing variable naming across notifications.The implementation is good and consistent with the transfer message pattern. Consider standardizing the variable name to
sourceCategoryNameto be more explicit about its role in the context of covering overbudgeted amounts.- message: t('Covered overbudgeted from {{categoryName}}', { - categoryName: categoriesById[categoryId].name, + message: t('Covered overbudgeted from {{sourceCategoryName}}', { + sourceCategoryName: categoriesById[categoryId].name,packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx (1)
55-57: Consider adding translation comments for better context.While the translation string is now properly structured with placeholders, adding a translation comment would help translators understand what
usedAmountandallocatedAmountrepresent in this context.<Trans> + {/* Translators: Shows budget usage, e.g., "$500 of $1000" */} {{ usedAmount: '' }} of {{ allocatedAmount: '' }} </Trans>packages/desktop-client/src/components/settings/Format.tsx (1)
24-41: Well-structured hook for days of week translations.Good improvements:
- Proper typing with SyncedPrefs
- Translations moved to a reusable hook
- Consistent structure for all days
Consider adding a JSDoc comment to document the hook's purpose and return type.
+/** + * Hook to provide translated days of the week + * @returns {{ daysOfWeek: Array<{ value: SyncedPrefs['firstDayOfWeekIdx']; label: string }> }} + */ function useDaysOfWeek() {packages/desktop-client/src/components/accounts/Reconcile.tsx (1)
86-93: Improved translation string structure with proper typing.Good improvements:
- Proper use of TransObjectLiteral type
- Clear placeholders for dynamic values
- Maintained HTML structure within translation
Consider adding a translation comment to explain the context and placeholders.
<Trans> + {/* Translators: Shows reconciliation status with bank balance comparison */} Your cleared balance{' '} <strong>{{ clearedBalance } as TransObjectLiteral}</strong>{' '} needs <strong>{{ difference } as TransObjectLiteral}</strong> to match <br /> your bank's balance of{' '} <Text style={{ fontWeight: 700 }}> {{ bankBalance } as TransObjectLiteral} </Text> </Trans>packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx (1)
214-234: Consider extracting label translations to constants.While the translation implementation is correct, consider extracting these frequently used labels to constants at the top of the file to improve maintainability and reusability.
+const CASH_FLOW_LABELS = { + income: t('Income:'), + expenses: t('Expenses:'), + change: t('Change:'), + transfers: t('Transfers:'), + balance: t('Balance:'), +}; // Then in the JSX: - left={t('Income:')} + left={CASH_FLOW_LABELS.income}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
packages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (39)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/accounts/Account.tsx(2 hunks)packages/desktop-client/src/components/accounts/Reconcile.tsx(2 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(1 hunks)packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx(2 hunks)packages/desktop-client/src/components/budget/SidebarCategory.tsx(1 hunks)packages/desktop-client/src/components/budget/SidebarGroup.tsx(1 hunks)packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetMenu.tsx(1 hunks)packages/desktop-client/src/components/budget/index.tsx(1 hunks)packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx(2 hunks)packages/desktop-client/src/components/filters/NameFilter.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx(2 hunks)packages/desktop-client/src/components/mobile/accounts/Accounts.tsx(4 hunks)packages/desktop-client/src/components/modals/BudgetPageMenuModal.tsx(1 hunks)packages/desktop-client/src/components/modals/ConfirmTransactionEditModal.tsx(4 hunks)packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx(3 hunks)packages/desktop-client/src/components/modals/EditRuleModal.jsx(2 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx(5 hunks)packages/desktop-client/src/components/modals/HoldBufferModal.tsx(1 hunks)packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx(4 hunks)packages/desktop-client/src/components/modals/manager/ConfirmChangeDocumentDir.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx(3 hunks)packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/CashFlow.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(3 hunks)packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx(3 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.tsx(1 hunks)packages/desktop-client/src/components/schedules/ScheduleLink.tsx(1 hunks)packages/desktop-client/src/components/settings/Encryption.tsx(1 hunks)packages/desktop-client/src/components/settings/Format.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(1 hunks)packages/desktop-client/src/components/transactions/SimpleTransactionsTable.tsx(4 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/shared/errors.ts(1 hunks)
✅ Files skipped from review due to trivial changes (23)
- packages/desktop-client/src/components/modals/manager/ConfirmChangeDocumentDir.tsx
- packages/desktop-client/src/components/manager/subscribe/Login.tsx
- packages/desktop-client/src/components/budget/SidebarGroup.tsx
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx
- packages/desktop-client/src/components/filters/NameFilter.tsx
- packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetMenu.tsx
- packages/desktop-client/src/components/modals/HoldBufferModal.tsx
- packages/desktop-client/src/components/modals/BudgetPageMenuModal.tsx
- packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx
- packages/desktop-client/src/components/settings/Encryption.tsx
- packages/desktop-client/src/components/modals/EditRuleModal.jsx
- packages/desktop-client/src/components/accounts/Account.tsx
- packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
- packages/loot-core/src/server/main.ts
- packages/desktop-client/src/components/budget/SidebarCategory.tsx
- packages/desktop-client/src/components/reports/reports/Calendar.tsx
- packages/desktop-client/src/components/reports/reports/CashFlow.tsx
- packages/desktop-client/src/components/budget/index.tsx
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
- packages/desktop-client/src/components/settings/index.tsx
- packages/loot-core/src/client/actions/budgets.ts
- packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx
🧰 Additional context used
📓 Learnings (3)
packages/loot-core/src/shared/errors.ts (1)
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/loot-core/src/server/main.ts:1873-1877
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `duplicate-budget` function in `packages/loot-core/src/server/main.ts`, when `loadBudget` returns an object with an `error` property where `error` is a string, returning `error` directly is consistent with the function's declared return type `Promise<string>`.
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (4)
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:181-196
Timestamp: 2024-11-10T16:45:25.627Z
Learning: After refactoring `nameError` in `DuplicateFileModal.tsx`, it's not necessary to use `setNameError` in the `onPress` handlers when validation fails.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:144-156
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In `packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx`, styles for buttons may differ for each button in the future, so avoid suggesting extraction of common styles in this file.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:181-196
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `DuplicateFileModal.tsx`, `trim()` is performed during the blur event when validating `newName`, so additional trimming is not necessary in the `onPress` handlers.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:0-0
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `DuplicateFileModal.tsx`, the `validateAndSetName` function trims `newName` before validation and duplication.
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🔇 Additional comments (30)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (1)
974-974: Great improvement to the translation string!The change from direct interpolation
{props.error}to named interpolation{{ errorReason: props.error }}is a good practice for internationalization. Named interpolations provide better context for translators and prevent potential issues with word order in different languages.packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx (1)
3-3: LGTM: Import change aligns with i18n best practicesAdding the
Transcomponent import is appropriate for handling complex translations with interpolation.packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx (1)
112-117: Well-structured translation implementation!The use of
Transcomponent here is appropriate as it keeps the entire string, including HTML formatting, as a single translatable unit. This prevents sentence fragmentation and maintains context for translators.packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx (4)
28-29: LGTM! Good hook placement.Moving the translation hook to the start of the component follows React hooks best practices and improves code organization.
45-45: Great improvement to the modal title!The new title "Transfer to category" is clearer and more translation-friendly by:
- Using simpler language
- Removing unnecessary punctuation
- Providing better context for translators
56-59: Excellent i18n implementation!The notification message now:
- Uses proper i18next interpolation syntax
- Has descriptive variable names
- Allows for flexible word order in different languages
69-69: Consistent improvement in modal title!The simplified title maintains the pattern of improvements seen earlier:
- Removes unnecessary punctuation
- Uses more natural language
- Maintains consistency with other modal titles
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx (1)
7-7: LGTM! Import statement added correctly.The
Transcomponent import aligns with the PR's objective of improving translation handling.packages/desktop-client/src/components/modals/ConfirmTransactionEditModal.tsx (3)
3-3: LGTM! Import statement updated correctly.Good practice to keep both
useTranslationandTransimports together.
38-77: Well-structured translation blocks for different scenarios.The translation strings are now properly structured and provide better context for translators. Each condition has its own clear, complete sentence.
100-118: Consistent pattern for accessibility and visible text.Good practice using:
t()for aria-labelsTranscomponent for visible button textpackages/desktop-client/src/components/settings/Format.tsx (1)
79-80: LGTM! Clean hook usage.The hook is properly destructured and used in the component.
packages/desktop-client/src/components/accounts/Reconcile.tsx (2)
8-8: LGTM! Type import added correctly.Good addition of TransObjectLiteral type for improved type safety.
100-106: Consistent capitalization in UI text.Good improvements in text consistency:
- "Done reconciling" instead of "Done Reconciling"
- "Create reconciliation transaction" instead of "Create Reconciliation Transaction"
packages/loot-core/src/shared/errors.ts (1)
104-106: LGTM! Improved error message clarity and translation support.The changes enhance the error message by:
- Using proper
{{id}}syntax for i18n variable interpolation- Correcting capitalization of "ID"
- Adding helpful context about where to find the budget ID in settings
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (1)
4-4: LGTM! Enhanced translation support for complex text.The changes properly implement the
Transcomponent for text containing links and HTML elements, which is the recommended approach for handling complex translations in react-i18next.Also applies to: 130-130, 142-152
packages/desktop-client/src/components/transactions/SimpleTransactionsTable.tsx (2)
129-134: LGTM! Proper handling of dynamic text translation.The account field correctly uses the
t()function for the dynamic "No account" fallback text while preserving the account name display.
220-220: LGTM! Consistent use of Trans component for static labels.Table headers now properly use the
Transcomponent for static text, improving translation management.Also applies to: 226-226, 232-232, 238-238, 244-244, 250-250, 256-256
packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx (1)
153-154: LGTM! Good variable renaming to avoid conflicts.The parameter renaming from
ttotransin map functions prevents confusion with the translation function.Also applies to: 156-157
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (5)
8-8: LGTM!The necessary imports for the
Transcomponent andTransObjectLiteraltype are correctly added.Also applies to: 12-12
153-153: LGTM!The static text is correctly wrapped with the
Transcomponent for translation.
157-166: LGTM!The translation is correctly implemented with dynamic value interpolation for the amount.
170-179: LGTM!The translation is correctly implemented with dynamic value interpolation for the amount.
184-205: LGTM!The translations are correctly implemented with:
- Dynamic value interpolation for goal type and amount
- Proper type casting using
TransObjectLiteralpackages/desktop-client/src/components/mobile/accounts/Accounts.tsx (3)
2-2: LGTM!The necessary import for the
Transcomponent is correctly added.
198-202: LGTM!The translation is correctly implemented with HTML-like elements for rich text formatting.
259-259: LGTM!The aria-label is correctly implemented with translation support, improving accessibility.
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (3)
2-2: LGTM!The necessary imports for the
Transcomponent andTransObjectLiteraltype are correctly added.Also applies to: 20-20
701-703: LGTM!The translation is correctly implemented with dynamic value interpolation for the report name.
709-718: LGTM!The translation is correctly implemented with:
- HTML-like elements for text formatting
- Dynamic value interpolation for the report name
- Proper type casting using
TransObjectLiteral
|
/update-vrt |
matt-fidd
force-pushed
the
translations-2
branch
from
397d0be to
e30f24c
Compare
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/desktop-client/src/components/settings/Format.tsx (2)
24-41: Memoize the days array to prevent unnecessary re-renders.The
daysOfWeekarray is recreated on every render. Consider usinguseMemoto optimize performance.function useDaysOfWeek() { const { t } = useTranslation(); - const daysOfWeek: { - value: SyncedPrefs['firstDayOfWeekIdx']; - label: string; - }[] = [ + const daysOfWeek = React.useMemo(() => ([ { value: '0', label: t('Sunday') }, { value: '1', label: t('Monday') }, { value: '2', label: t('Tuesday') }, { value: '3', label: t('Wednesday') }, { value: '4', label: t('Thursday') }, { value: '5', label: t('Friday') }, { value: '6', label: t('Saturday') }, - ] as const; + ] as const), [t]); return { daysOfWeek }; }
Line range hint
1-1: Consider enabling TypeScript strict mode.The file currently disables strict type checking with
@ts-strict-ignore. Consider addressing the TypeScript errors and enabling strict mode for better type safety.packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (2)
153-179: Consider extracting funding status messages for better maintainability.The translation implementation looks good, but the nested conditional rendering could be simplified by extracting the funding status messages into a separate component or helper function.
Example refactor:
const FundingStatus = ({ difference, format }) => { if (difference === 0) { return ( <span style={{ color: theme.noticeText }}> <Trans>Fully funded</Trans> </span> ); } const isOverfunded = difference > 0; return ( <span style={{ color: isOverfunded ? theme.noticeText : theme.errorText }}> <Trans> {isOverfunded ? 'Overfunded (' : 'Underfunded ('} {{ amount: format(difference, 'financial') }} ) </Trans> </span> ); };
184-205: Consider creating a type-safe translation key mapping.While the current implementation works, we could improve type safety for translation keys by creating a mapping between the goal types and their translations.
Example:
const GOAL_TYPE_TRANSLATIONS = { LONG: 'Long', TEMPLATE: 'Template', } as const; // Then use it like: type: longGoalValue === 1 ? t(GOAL_TYPE_TRANSLATIONS.LONG) : t(GOAL_TYPE_TRANSLATIONS.TEMPLATE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (27)
packages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Bar-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Data-Table-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Donut-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-labels-button-shows-the-labels-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-legend-button-shows-the-legend-side-bar-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-custom-reports-Validates-that-show-summary-button-shows-the-summary-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (39)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/accounts/Account.tsx(2 hunks)packages/desktop-client/src/components/accounts/Reconcile.tsx(2 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(1 hunks)packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx(2 hunks)packages/desktop-client/src/components/budget/SidebarCategory.tsx(1 hunks)packages/desktop-client/src/components/budget/SidebarGroup.tsx(1 hunks)packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetMenu.tsx(1 hunks)packages/desktop-client/src/components/budget/index.tsx(1 hunks)packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx(2 hunks)packages/desktop-client/src/components/filters/NameFilter.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx(2 hunks)packages/desktop-client/src/components/mobile/accounts/Accounts.tsx(4 hunks)packages/desktop-client/src/components/modals/BudgetPageMenuModal.tsx(1 hunks)packages/desktop-client/src/components/modals/ConfirmTransactionEditModal.tsx(4 hunks)packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx(3 hunks)packages/desktop-client/src/components/modals/EditRuleModal.jsx(2 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx(5 hunks)packages/desktop-client/src/components/modals/HoldBufferModal.tsx(1 hunks)packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx(4 hunks)packages/desktop-client/src/components/modals/manager/ConfirmChangeDocumentDir.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx(3 hunks)packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/CashFlow.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(3 hunks)packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx(3 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.tsx(1 hunks)packages/desktop-client/src/components/schedules/ScheduleLink.tsx(1 hunks)packages/desktop-client/src/components/settings/Encryption.tsx(1 hunks)packages/desktop-client/src/components/settings/Format.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(1 hunks)packages/desktop-client/src/components/transactions/SimpleTransactionsTable.tsx(4 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/shared/errors.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (36)
- packages/desktop-client/src/components/filters/NameFilter.tsx
- packages/desktop-client/src/components/budget/SidebarCategory.tsx
- packages/desktop-client/src/components/manager/subscribe/Login.tsx
- packages/desktop-client/src/components/modals/manager/ConfirmChangeDocumentDir.tsx
- packages/desktop-client/src/components/modals/HoldBufferModal.tsx
- packages/desktop-client/src/components/reports/reports/CashFlow.tsx
- packages/desktop-client/src/components/budget/SidebarGroup.tsx
- packages/desktop-client/src/components/settings/Encryption.tsx
- packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetMenu.tsx
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx
- packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx
- packages/loot-core/src/shared/errors.ts
- packages/desktop-client/src/components/budget/index.tsx
- packages/desktop-client/src/components/modals/BudgetPageMenuModal.tsx
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/settings/index.tsx
- packages/loot-core/src/client/actions/budgets.ts
- packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx
- packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
- packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
- packages/desktop-client/src/components/modals/EditRuleModal.jsx
- packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx
- packages/desktop-client/src/components/accounts/Reconcile.tsx
- packages/desktop-client/src/components/accounts/Account.tsx
- packages/desktop-client/src/components/reports/reports/CustomReport.tsx
- packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx
- packages/desktop-client/src/components/schedules/ScheduleDetails.tsx
- packages/desktop-client/src/components/reports/reports/Calendar.tsx
- packages/desktop-client/src/components/modals/ConfirmTransactionEditModal.tsx
- packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx
- packages/desktop-client/src/components/transactions/SimpleTransactionsTable.tsx
- packages/loot-core/src/server/main.ts
- packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.tsx
- packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
🔇 Additional comments (4)
packages/desktop-client/src/components/settings/Format.tsx (1)
79-79: LGTM! Good i18n improvement.The usage of the new
useDaysOfWeekhook effectively centralizes the translation logic for days of the week, making it more maintainable and consistent with the PR's goal of improving translation strings.packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx (1)
7-7: LGTM! Import of Trans component aligns with i18n improvements.The addition of the
Transcomponent import is appropriate for the improved translation handling approach.packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (2)
8-8: LGTM! Necessary imports for i18n improvements.The addition of
Transcomponent andTransObjectLiteraltype imports supports the enhanced internationalization capabilities.Also applies to: 12-13
209-229: LGTM! Verify translation string consistency across the application.The implementation of budgeted and balance displays is correct. However, we should ensure that similar financial labels are translated consistently across the application.
✅ Verification successful
Translation implementation is consistent across the codebase
The implementation of "Budgeted" and "Balance" translations follows a consistent pattern using
<Trans>components throughout the application, with proper formatting and structure maintained across all components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of "Budgeted" and "Balance" translations # to verify consistency echo "Checking 'Budgeted' translations:" rg -l "Budgeted" --type ts --type tsx echo "Checking 'Balance' translations:" rg -l "Balance" --type ts --type tsxLength of output: 294
Script:
#!/bin/bash # Search with correct file type handling and comprehensive patterns echo "Searching for 'Budgeted' translations:" rg -i "budgeted" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts -C 2 echo -e "\nSearching for 'Balance' translations:" rg -i "balance" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts -C 2 | grep -i "balance.*[\"'>]" echo -e "\nSearching for Trans components with these terms:" rg "<Trans>" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type ts -A 3 -B 1 | grep -i -E "(budgeted|balance)"Length of output: 76458
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx
Outdated
Show resolved
Hide resolved
...ts/Reports-custom-reports-Switches-to-Area-Graph-and-checks-the-visuals-1-chromium-linux.png
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks good, thanks! Working on addressing the cimode issue. Do you think it'd be worth encoding some of the knowledge we've started to develop around how to write high-quality source strings in https://actualbudget.org/docs/contributing/i18n/?
| @@ -53,8 +52,9 @@ export function BudgetTotal< | |||
| <Text> | |||
| <CellValue binding={current} type="financial" /> | |||
There was a problem hiding this comment.
Wondering, would it work to move the Trans outside of the Text? Otherwise I'm thinking it could be hard to represent languages where the direction is normally inverted, e.g. Korean
There was a problem hiding this comment.
Sorry, I'm not sure what you mean
There was a problem hiding this comment.
Sorry, should have commented a line lower so this was clearer. I'm worried about languages where the following would need to be inverted:
{{ usedAmount: '' }} of {{ allocatedAmount: '' }}With allocatedAmount on the left instead
There was a problem hiding this comment.
I was hoping that by keeping the variables in the translation that Weblate would let them put it the other way round
it seems to let you put the variables wherever you'd like so they'd be able to flip them.
There was a problem hiding this comment.
Right, that's true, but since these are placeholders and not the actual numbers, won't that not actually affect how Actual displays them?
There was a problem hiding this comment.
🤦♂️ doh. yes, it will.
I'll see if I can do it properly
There was a problem hiding this comment.
what do you think of c613d29?
packages/desktop-client/src/components/manager/subscribe/Login.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/ConfirmTransactionEditModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/EditRuleModal.jsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetMenu.tsx
Outdated
Show resolved
Hide resolved
Absolutely, I'll give it a go unless you'd like to? |
Go ahead! Think you probably have the most experience with it at this point |
matt-fidd
force-pushed
the
translations-2
branch
from
1c15c64 to
457a644
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🛑 Comments failed to post (1)
packages/desktop-client/src/components/reports/reports/CustomReport.tsx (1)
709-720:
⚠️ Potential issueFix formatting issues in the translation component.
The implementation correctly uses the
Transcomponent for complex text with embedded styling. However, there are formatting issues that need to be addressed.Apply this diff to fix the formatting:
<Trans> <Text>Custom Report:</Text>{' '} <Text style={{ marginLeft: 5, color: theme.pageTextPositive }}> - { - { - name: report.name?.length > 0 - ? report.name - : t('Unsaved report'), - } as TransObjectLiteral - } + {{ + name: report.name?.length > 0 + ? report.name + : t('Unsaved report'), + } as TransObjectLiteral} </Text> </Trans>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<Trans> <Text>Custom Report:</Text>{' '} <Text style={{ marginLeft: 5, color: theme.pageTextPositive }}> {{ name: report.name?.length > 0 ? report.name : t('Unsaved report'), } as TransObjectLiteral} </Text> </Trans>🧰 Tools
🪛 eslint
[error] 714-714: Insert
⏎·······················(prettier/prettier)
[error] 715-715: Insert
··(prettier/prettier)
[error] 716-716: Insert
··(prettier/prettier)
🪛 GitHub Check: lint
[failure] 714-714:
Insert⏎·······················
[failure] 715-715:
Insert··
[failure] 716-716:
Insert··🪛 GitHub Actions: Test
[error] 714-714: Prettier formatting error: Missing line break at column 28
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/desktop-client/src/components/settings/Format.tsx (1)
24-41: LGTM! Consider adding JSDoc for better documentation.Good separation of concerns by extracting the days of week translations into a custom hook. The implementation correctly follows Pikaday's firstDay numbering and provides proper typing.
Consider adding JSDoc to document the hook's purpose and return type:
+/** + * Custom hook that provides translated day names following Pikaday's firstDay numbering. + * @returns {{ daysOfWeek: Array<{ value: SyncedPrefs['firstDayOfWeekIdx']; label: string }> }} + */ function useDaysOfWeek() {packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (1)
199-232: Consider extracting repeated translation pattern.The implementation is correct, but there's a repeated pattern in the tooltip rows that could be extracted into a reusable component for better maintainability.
Consider creating a reusable component:
type TooltipRowProps = { label: string; value: string | number; type?: string; }; function TooltipRow({ label, value, type = 'financial' }: TooltipRowProps) { const format = useFormat(); return ( <GoalTooltipRow> <Trans> <div>{label}:</div> <div> {{ amount: typeof value === 'number' ? format(value, type) : value, } as TransObjectLiteral} </div> </Trans> </GoalTooltipRow> ); }Then use it like:
<TooltipRow label="Goal" value={goalValue} /> <TooltipRow label="Balance" value={balanceValue} type={type} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
packages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/4154.mdis excluded by!**/*.md
📒 Files selected for processing (40)
packages/desktop-client/src/components/App.tsx(1 hunks)packages/desktop-client/src/components/accounts/Account.tsx(2 hunks)packages/desktop-client/src/components/accounts/Reconcile.tsx(2 hunks)packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx(1 hunks)packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx(2 hunks)packages/desktop-client/src/components/budget/SidebarCategory.tsx(1 hunks)packages/desktop-client/src/components/budget/SidebarGroup.tsx(1 hunks)packages/desktop-client/src/components/budget/envelope/CoverMenu.tsx(1 hunks)packages/desktop-client/src/components/budget/index.tsx(1 hunks)packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx(2 hunks)packages/desktop-client/src/components/filters/NameFilter.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/Login.tsx(1 hunks)packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx(2 hunks)packages/desktop-client/src/components/mobile/accounts/Accounts.tsx(4 hunks)packages/desktop-client/src/components/modals/BudgetPageMenuModal.tsx(1 hunks)packages/desktop-client/src/components/modals/ConfirmTransactionEditModal.tsx(4 hunks)packages/desktop-client/src/components/modals/CoverModal.tsx(1 hunks)packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx(3 hunks)packages/desktop-client/src/components/modals/EditRuleModal.jsx(2 hunks)packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx(5 hunks)packages/desktop-client/src/components/modals/HoldBufferModal.tsx(1 hunks)packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx(4 hunks)packages/desktop-client/src/components/modals/manager/ConfirmChangeDocumentDir.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx(3 hunks)packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(2 hunks)packages/desktop-client/src/components/reports/reports/CashFlow.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/CustomReport.tsx(3 hunks)packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx(3 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.tsx(1 hunks)packages/desktop-client/src/components/schedules/ScheduleLink.tsx(1 hunks)packages/desktop-client/src/components/settings/Encryption.tsx(1 hunks)packages/desktop-client/src/components/settings/Format.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(1 hunks)packages/desktop-client/src/components/transactions/SimpleTransactionsTable.tsx(4 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/server/main.ts(1 hunks)packages/loot-core/src/shared/errors.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (38)
- packages/desktop-client/src/components/budget/envelope/CoverMenu.tsx
- packages/desktop-client/src/components/budget/SidebarGroup.tsx
- packages/desktop-client/src/components/budget/SidebarCategory.tsx
- packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx
- packages/desktop-client/src/components/filters/NameFilter.tsx
- packages/desktop-client/src/components/modals/HoldBufferModal.tsx
- packages/desktop-client/src/components/modals/manager/ConfirmChangeDocumentDir.tsx
- packages/desktop-client/src/components/settings/Encryption.tsx
- packages/desktop-client/src/components/budget/index.tsx
- packages/desktop-client/src/components/manager/subscribe/Login.tsx
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/schedules/ScheduleDetails.tsx
- packages/desktop-client/src/components/modals/CoverModal.tsx
- packages/desktop-client/src/components/settings/index.tsx
- packages/desktop-client/src/components/reports/spreadsheets/cash-flow-spreadsheet.tsx
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx
- packages/desktop-client/src/components/reports/reports/CashFlow.tsx
- packages/loot-core/src/shared/errors.ts
- packages/desktop-client/src/components/reports/graphs/CalendarGraph.tsx
- packages/desktop-client/src/components/accounts/Account.tsx
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx
- packages/desktop-client/src/components/manager/subscribe/OpenIdForm.tsx
- packages/desktop-client/src/components/reports/reports/CustomReport.tsx
- packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
- packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx
- packages/loot-core/src/server/main.ts
- packages/desktop-client/src/components/modals/BudgetPageMenuModal.tsx
- packages/loot-core/src/client/actions/budgets.ts
- packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx
- packages/desktop-client/src/components/modals/EditRuleModal.jsx
- packages/desktop-client/src/components/modals/ConfirmTransactionEditModal.tsx
- packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx
- packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx
- packages/desktop-client/src/components/accounts/Reconcile.tsx
- packages/desktop-client/src/components/reports/reports/Calendar.tsx
- packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
- packages/desktop-client/src/components/transactions/SimpleTransactionsTable.tsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.tsx
🔇 Additional comments (4)
packages/desktop-client/src/components/settings/Format.tsx (1)
79-80: LGTM!The hook is properly integrated into the FormatSettings component and correctly used with the Select component.
packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx (3)
8-13: LGTM!Proper imports added for enhanced i18n support with type safety.
153-179: LGTM! Improved translation handling for status messages.Good use of the Trans component for complex strings with dynamic amount interpolation. The status messages are properly color-coded and formatted.
184-196: LGTM! Successfully implemented translation context.The implementation properly addresses the previous feedback about adding context for the 'Long' translation. Good use of TransObjectLiteral for type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx (1)
53-70: Improve i18n key description for translators.The component implementation looks good and properly addresses RTL concerns by using component placeholders. However, the i18n key could be more descriptive to provide better context for translators.
Consider updating the key to better describe the budget context:
- i18nKey="<allocatedAmount /> <italic>of <totalAmount /></italic>" + i18nKey="Budget amount: <allocatedAmount /> <italic>of <totalAmount /></italic>"This maintains the natural language key approach while providing more context about the budget-related nature of the text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx(2 hunks)packages/desktop-client/src/components/spreadsheet/CellValue.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4154
File: packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx:55-57
Timestamp: 2025-01-15T23:51:51.099Z
Learning: In the Actual Budget codebase, natural language keys are used for translations instead of structured i18n keys (e.g., "budget.total.amount"). Empty strings are intentionally used in translation placeholders to provide context for translators about where dynamic content will be inserted.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
- GitHub Check: compare
🔇 Additional comments (1)
packages/desktop-client/src/components/budget/tracking/budgetsummary/BudgetTotal.tsx (1)
7-7: LGTM!The
Transcomponent import is correctly placed and necessary for the internationalization changes.
packages/desktop-client/src/components/spreadsheet/CellValue.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/spreadsheet/CellValue.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Julian Dominguez-Schatz <julian.dominguezschatz@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/desktop-client/src/components/spreadsheet/CellValue.tsx (1)
Line range hint
2-5: Consider using named imports from React.Replace the default React import with named imports to align with the linting rules.
-import React, { +import { type ComponentPropsWithoutRef, type ReactNode, type CSSProperties, + Children, } from 'react';Then update the children check to use the named import:
- return children && React.Children.toArray(children).length ? ( + return children && Children.toArray(children).length ? (🧰 Tools
🪛 GitHub Check: typecheck
[failure] 47-47:
Argument of type '({ type, name, value, }: { type?: FormatType; name: string; value: Spreadsheets[SheetName][FieldName]; }) => ReactNode' is not assignable to parameter of type 'ReactNode | ReactNode[]'.🪛 GitHub Check: lint
[warning] 47-47:
Using default React import is discouraged, please use named exports directly instead🪛 GitHub Actions: Test
[error] 47-47: Type mismatch error: Argument type '({ type, name, value, }: { type?: FormatType; name: string; value: Spreadsheets[SheetName][FieldName]; }) => ReactNode' is not assignable to parameter type 'ReactNode | ReactNode[]'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/spreadsheet/CellValue.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/spreadsheet/CellValue.tsx (1)
Learnt from: matt-fidd
PR: actualbudget/actual#4154
File: packages/desktop-client/src/components/spreadsheet/CellValue.tsx:47-47
Timestamp: 2025-01-21T11:39:38.461Z
Learning: In the CellValue component of packages/desktop-client/src/components/spreadsheet/CellValue.tsx, the children prop needs an additional length check (children && children.length > 0) because the Trans component can pass an empty array at runtime despite the type definition expecting a function.
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/spreadsheet/CellValue.tsx
[failure] 47-47:
Argument of type '({ type, name, value, }: { type?: FormatType; name: string; value: Spreadsheets[SheetName][FieldName]; }) => ReactNode' is not assignable to parameter of type 'ReactNode | ReactNode[]'.
🪛 GitHub Check: lint
packages/desktop-client/src/components/spreadsheet/CellValue.tsx
[warning] 47-47:
Using default React import is discouraged, please use named exports directly instead
🪛 GitHub Actions: Test
packages/desktop-client/src/components/spreadsheet/CellValue.tsx
[error] 47-47: Type mismatch error: Argument type '({ type, name, value, }: { type?: FormatType; name: string; value: Spreadsheets[SheetName][FieldName]; }) => ReactNode' is not assignable to parameter type 'ReactNode | ReactNode[]'
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Wait for Netlify build to finish
- GitHub Check: Analyze
🔇 Additional comments (1)
packages/desktop-client/src/components/spreadsheet/CellValue.tsx (1)
47-47: LGTM! The children prop check is now more robust.The implementation correctly uses
React.Children.toArray(children).lengthto handle the edge case where the Trans component passes an empty array at runtime.Note: The type error in the pipeline is a known issue due to the mismatch between the runtime behavior of the Trans component and the type definition. The current implementation is the pragmatic solution to handle this edge case.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 47-47:
Argument of type '({ type, name, value, }: { type?: FormatType; name: string; value: Spreadsheets[SheetName][FieldName]; }) => ReactNode' is not assignable to parameter of type 'ReactNode | ReactNode[]'.🪛 GitHub Check: lint
[warning] 47-47:
Using default React import is discouraged, please use named exports directly instead🪛 GitHub Actions: Test
[error] 47-47: Type mismatch error: Argument type '({ type, name, value, }: { type?: FormatType; name: string; value: Spreadsheets[SheetName][FieldName]; }) => ReactNode' is not assignable to parameter type 'ReactNode | ReactNode[]'
|
Looks like lint and typecheck don't like that change, was I meant to do anything else? Sorry, React children behaviour is not something I'm massively familiar with. edit: okay, should have read the lint first, that was easy, not sure on the typecheck though |
@matt-fidd Try something on this lines (didn't tested): const memoizedChildren = useMemo(() => {
if (typeof children === "function") {
return children({
type,
name: fullSheetName,
value: sheetValue,
});
}
return null;
}, [children, type, fullSheetName, sheetValue]);
return memoizedChildren ? (
<>{memoizedChildren}</>
) : (
<CellValueText
type={type}
name={fullSheetName}
value={sheetValue}
{...props}
/>
); |
|
Ah, thank you. I was misinterpreting the error |
Carrying on from #4041
Down to "O" now. I don't think I need to bother with release notes for the rest of these unless you do?