Marked files for translation #3548
Conversation
actual-github-bot
bot
changed the title
WalkthroughThis pull request implements internationalization support in the Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
✅ 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 No assets were 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 No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
![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
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/reports/ReportLegend.tsx (2)
38-40: LGTM with a suggestion: Consider extracting the translation.The changes correctly implement internationalization for the 'Interval' string while maintaining the existing logic. However, to improve readability and maintainability, consider extracting the translation to a constant.
Here's a suggested refactor:
+ const intervalText = t('Interval'); - {groupBy === t('Interval') + {groupBy === intervalText ? ReportOptions.intervalMap.get(interval) : groupBy}This change would make the code more readable and ensure consistent use of the translated string if needed elsewhere in the component.
2-2: Summary: Successful implementation of internationalization support.The changes in this file successfully introduce internationalization support using react-i18next. The modifications are minimal, focused, and preserve the existing functionality while allowing for translation of the 'Interval' text. This implementation aligns well with the PR objective of marking files for translation.
As the project moves forward with internationalization, consider the following recommendations:
- Ensure consistent use of the
useTranslationhook across all components that require text translation.- Create a centralized location for managing translation keys to maintain consistency and ease of management.
- Implement a process for extracting and managing translation strings to facilitate the work of translators.
Also applies to: 18-18, 38-40
packages/desktop-client/src/components/reports/getLiveRange.ts (2)
Line range hint
45-49: LGTM: Internationalization implemented correctly.The changes correctly implement internationalization for the strings 'This month' and 'This week' using the
tfunction. This is consistent with the PR objective of marking files for translation.Consider extracting these translated strings into constants at the top of the file for better maintainability:
const THIS_MONTH = t('This month'); const THIS_WEEK = t('This week'); // Then use these constants in the function: [THIS_MONTH, THIS_WEEK].includes(cond)This approach can make it easier to manage and update these strings in the future.
Line range hint
1-58: Summary: Successful implementation of internationalization.The changes in this file successfully implement internationalization for two key strings used in the
getLiveRangefunction. These modifications align well with the PR objective of marking files for translation. The overall structure and logic of the function remain intact, ensuring that existing functionality is preserved while adding support for localization.As the project moves forward with internationalization, consider the following recommendations:
- Implement a consistent strategy for managing translated strings across the entire project.
- Use a translation management system to keep track of all strings that need translation.
- Consider adding context comments for translators to ensure accurate translations in different scenarios.
- Implement a process for testing the application with different languages to catch any layout or formatting issues early.
packages/desktop-client/src/components/reports/DateRange.tsx (1)
Line range hint
58-71: LGTM with a minor suggestion: t function used correctly for translations.The
tfunction is properly used to translate "Compare" and "to". However, there's a minor inconsistency in spacing around the translated "to".Consider adjusting the spacing for consistency:
- {type ? t(' to ') : ' - '} + {type ? ` ${t('to')} ` : ' - '}Apply this change to both lines 60 and 71 for consistency.
packages/desktop-client/src/components/reports/ReportOptions.ts (1)
307-307: Consider adding a translation comment to provide context.While the change itself is fine, consider adding a translation comment above the line to provide context for translators. This can help ensure accurate translations and maintain consistency across languages.
// Translators: This comment describes the purpose of the sorting logic below. + // It sorts categories based on their income status, group sort order, and individual sort order. Number(catGroupA.is_income) - Number(catGroupB.is_income) ||
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/desktop-client/src/components/reports/DateRange.tsx (5 hunks)
- packages/desktop-client/src/components/reports/ReportLegend.tsx (3 hunks)
- packages/desktop-client/src/components/reports/ReportOptions.ts (11 hunks)
- packages/desktop-client/src/components/reports/disabledList.ts (2 hunks)
- packages/desktop-client/src/components/reports/getLiveRange.ts (2 hunks)
- packages/desktop-client/src/components/reports/index.tsx (1 hunks)
🔇 Additional comments (28)
packages/desktop-client/src/components/reports/index.tsx (4)
1-1: LGTM: Import statement for localization.The import of
useTranslationfrom 'react-i18next' is correctly added and aligns with the PR objective of marking files for translation.
7-7: LGTM: Initialization of translation function.The
useTranslationhook is correctly used to initialize thetfunction, enabling localization within the component.
13-13: LGTM: Localization of loading message.The loading message is now correctly wrapped with the
tfunction, enabling its localization. This change is consistent with the PR objective of marking files for translation.
1-13: Overall assessment: Localization support successfully implemented.The changes in this file consistently implement localization support by introducing the
useTranslationhook and applying it to the loading message. These modifications align well with the PR objective of marking files for translation.packages/desktop-client/src/components/reports/ReportLegend.tsx (2)
2-2: LGTM: Correct import for internationalization.The import statement for
useTranslationfrom 'react-i18next' is correctly added, which is necessary for the internationalization changes in the component.
18-18: LGTM: Correct usage of useTranslation hook.The
useTranslationhook is properly utilized to obtain the translation functiont. This setup allows for easy internationalization of the component's text content.packages/desktop-client/src/components/reports/getLiveRange.ts (1)
1-1: LGTM: Import statement for internationalization.The import of the
tfunction from 'i18next' is correctly placed and necessary for the internationalization changes in the file.packages/desktop-client/src/components/reports/DateRange.tsx (5)
2-2: LGTM: Internationalization imports added correctly.The import of
useTranslationandTransfrom 'react-i18next' is correctly placed and necessary for the internationalization changes in this file.
30-31: LGTM: useTranslation hook initialized correctly.The
useTranslationhook is properly initialized and destructured. The added empty line improves code readability.
40-40: LGTM: Error message wrapped with Trans component.The error message "There was a problem loading your date range" is correctly wrapped with the
Transcomponent, making it ready for internationalization.
49-52: LGTM: Comparison text wrapped with Trans component.The comparison text is correctly wrapped with the
Transcomponent, including proper handling of dynamic content (date formatting and conditional text). This change effectively prepares the text for internationalization.
Line range hint
1-83: Summary: Internationalization changes implemented correctly.The changes to add internationalization support to the
DateRangecomponent have been implemented correctly. TheuseTranslationhook andTranscomponent fromreact-i18nextare used appropriately throughout the file. The component's core functionality remains intact while adding proper translation support for all user-facing strings.A minor suggestion was made for spacing consistency around the translated "to" text, but overall, the changes are well-implemented and ready for use.
packages/desktop-client/src/components/reports/disabledList.ts (5)
1-18: LGTM: Internationalization applied correctlyThe import of the
tfunction and its application to theintervalOptionsarray are implemented correctly. This change successfully prepares these strings for translation.
24-40: LGTM: Internationalization applied consistentlyThe
descriptionfields in thecurrentIntervalOptionsarray have been correctly wrapped with thetfunction, maintaining consistency with the internationalization approach.
58-84: LGTM: Internationalization applied correctly to totalGraphOptionsThe
defaultSplit,defaultType, anddisabledSplitfields in thetotalGraphOptionsarray have been correctly wrapped with thetfunction. The array of strings in thedisabledSplitfield for the 'AreaGraph' option has been properly internationalized as well.
92-110: LGTM: Internationalization applied correctly to timeGraphOptionsThe
defaultSplit,defaultType, anddisabledSplitfields in thetimeGraphOptionsarray have been correctly wrapped with thetfunction. The array of strings in thedisabledTypefield for the 'TableGraph' option has been properly internationalized as well.
Line range hint
1-110: Summary: Successful internationalization of string literalsThe changes in this file successfully prepare it for translation by wrapping all relevant string literals with the
tfunction from i18next. The modifications are consistent and don't alter the functionality of the code. This update aligns well with the PR objective of marking files for translation.Key points:
- All string literals in
intervalOptions,currentIntervalOptions,totalGraphOptions, andtimeGraphOptionsarrays have been internationalized.- The changes maintain the existing structure and logic of the code.
- Exported functions and constants remain unchanged, as they didn't require direct internationalization.
These changes lay the groundwork for future localization efforts in the application.
packages/desktop-client/src/components/reports/ReportOptions.ts (11)
21-25: LGTM!The changes to the
defaultReportobject look good. The hardcoded string literals fordateRange,groupBy,interval, andbalanceTypehave been replaced with calls to thet()function, enabling translation for these properties.
37-41: LGTM!The
descriptionfields in thebalanceTypeOptionsarray have been updated to use thet()function for translation. This change is consistent and appropriate.
45-49: LGTM!The
descriptionfields in thegroupByOptionsarray have been updated to use thet()function for translation. This change is consistent and appropriate.
64-147: LGTM!The
descriptionandtypefields in thedateRangeOptionsarray have been updated to use thet()function for translation. This change is consistent and appropriate across all date range options.
168-189: LGTM!The
descriptionandnamefields in theintervalOptionsarray have been updated to use thet()function for translation. This change is consistent and appropriate across all interval options.
256-256: LGTM!The
nameproperty of theuncategorizedCategoryobject has been updated to use thet()function for translation. This change is consistent with the overall internationalization effort.
265-265: LGTM!The
nameproperty of thetransferCategoryobject has been updated to use thet()function for translation. This change is consistent with the overall internationalization effort.
274-274: LGTM!The
nameproperty of theoffBudgetCategoryobject has been updated to use thet()function for translation. This change is consistent with the overall internationalization effort.
290-290: LGTM!The
nameproperty of theuncategorizedGroupobject has been updated to use thet()function for translation. This change is consistent with the overall internationalization effort.
341-368: LGTM!The case statements in the
groupBySelectionsfunction have been updated to use thet()function for translation. This change ensures that the grouping options are properly translated based on the selected language.
1-1: Ensure thei18nextlibrary is installed and properly configured.Verify that the
i18nextlibrary is installed in the project and properly configured to support internationalization. This includes setting up the necessary language files, configuring the fallback language, and initializing the library with the appropriate options.Run the following script to verify the
i18nextsetup:
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/reports/index.tsx (2)
13-13: Consider using a specific translation key.The implementation of localization for the loading message is correct. However, using the English text as the translation key might lead to maintenance issues in the future.
Consider using a more specific translation key:
- message={t('Loading reports...')} + message={t('reports.loading_message')}This approach would make it easier to manage translations if the English text changes and helps ensure uniqueness across the application.
Line range hint
1-20: Summary: Localization successfully implemented.The changes in this file successfully introduce localization support to the
Reportscomponent. The implementation is correct and aligns with the PR objective of marking files for translation.Next steps to consider:
- Implement a consistent strategy for translation keys across the application.
- Ensure that all user-facing strings in this component are localized.
- Create translation files for different languages.
- Set up a process for managing and updating translations.
packages/desktop-client/src/components/reports/getLiveRange.ts (1)
45-47: Approve changes with suggestion for improvement.The use of the
tfunction for translating 'This month' and 'This week' is a good step towards localization. However, there's room for improvement in the translation approach.Consider using translation keys instead of directly translating the English strings. This approach is more maintainable and less error-prone. For example:
[t('report.this_month'), t('report.this_week')].includes(t(cond))This assumes that
condis already a translation key. If it's not, you might need to create a mapping between the English strings and their corresponding translation keys.packages/desktop-client/src/components/reports/DateRange.tsx (1)
58-60: Consider combining phrases for better translation support.The
tfunction is correctly used for translating shorter phrases. However, separating "Compare" and "to" into individual translations might cause issues with languages that have different word orders.Consider combining these phrases into a single translation string with placeholders. For example:
t('Compare {{startDate}} to {{endDate}}', { startDate: d.format(startDate, 'MMM yyyy'), endDate: ['budget', 'average'].includes(type || '') ? type : d.format(endDate, 'MMM yyyy') })This approach would allow translators to rearrange the words as needed for their language.
Also applies to: 69-71
packages/desktop-client/src/components/reports/disabledList.ts (2)
5-6: LGTM: Internationalization applied to intervalOptions.The changes consistently apply the
tfunction to all string literals in theintervalOptionsarray, covering bothdescriptionanddefaultRangefields. This is a good step towards internationalization.Consider extracting these strings into a separate constants file or using a key-based approach for better maintainability. For example:
import { INTERVAL_KEYS } from './constants'; const intervalOptions = [ { description: t(INTERVAL_KEYS.DAILY), defaultRange: t(INTERVAL_KEYS.THIS_MONTH), }, // ... other options ];This approach can help centralize all translatable strings and make it easier to manage them as the application grows.
Also applies to: 9-10, 13-14, 17-18
Line range hint
1-190: LGTM: Consistent internationalization changes.The internationalization changes have been consistently applied to the relevant parts of the file. The overall structure and logic of the file remain intact, which is good.
For consistency, consider applying internationalization to other user-facing strings in the file, if any. This might include error messages or any other text that could potentially be displayed to the user.
Also, to ensure all translatable strings are properly handled, you might want to run a static analysis tool that can detect non-internationalized strings. This can help catch any missed opportunities for translation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/desktop-client/src/components/reports/DateRange.tsx (5 hunks)
- packages/desktop-client/src/components/reports/ReportLegend.tsx (3 hunks)
- packages/desktop-client/src/components/reports/ReportOptions.ts (11 hunks)
- packages/desktop-client/src/components/reports/disabledList.ts (2 hunks)
- packages/desktop-client/src/components/reports/getLiveRange.ts (2 hunks)
- packages/desktop-client/src/components/reports/index.tsx (1 hunks)
🔇 Additional comments (19)
packages/desktop-client/src/components/reports/index.tsx (2)
1-1: LGTM: Import statement for localization added correctly.The import of
useTranslationfrom 'react-i18next' is appropriate for implementing localization in this component.
7-7: LGTM: Translation hook initialized correctly.The
useTranslationhook is properly used to obtain the translation functiont. This follows best practices for implementing localization in React components.packages/desktop-client/src/components/reports/ReportLegend.tsx (4)
2-2: LGTM: Correct import for localization.The import of
useTranslationfrom 'react-i18next' is correctly added and is necessary for the localization changes in this component.
18-19: LGTM: Proper usage of useTranslation hook.The
useTranslationhook is correctly implemented, destructuring thetfunction for use in translations within the component.
Line range hint
1-54: Summary: Localization implemented correctly, with a minor concern.The changes in this file successfully introduce localization using react-i18next, which aligns with the PR objective. The implementation is correct, but there's a potential logic issue in the comparison of 'groupBy' with the translated 'Interval' string. This should be verified and possibly adjusted to ensure correct functionality across different languages.
Overall, good progress on internationalization!
38-40: Approve translation, but verify comparison logic.The use of
t('Interval')for translation is correct. However, there's a potential logic issue:
- The comparison
groupBy === t('Interval')might not work as expected if 'groupBy' is not translated elsewhere.- This could lead to the condition always being false, defaulting to displaying the 'groupBy' value instead of the interval.
To verify this, please run the following script:
If 'groupBy' is not translated elsewhere, consider updating the logic to:
{groupBy.toLowerCase() === 'interval' ? ReportOptions.intervalMap.get(interval) : groupBy}This would make the comparison case-insensitive and independent of translation.
packages/desktop-client/src/components/reports/getLiveRange.ts (2)
1-1: LGTM: Import statement for translation function.The import of the
tfunction from 'i18next' is correctly placed and necessary for the translation functionality added to the file.
Line range hint
1-58: Verify consistent translation approach across the codebase.The changes made to this file are a good start for localization. However, as this PR is marked as WIP, it's important to ensure that a consistent translation approach is applied across all files mentioned in the PR objectives.
To check for consistency and identify other potential translation opportunities, you can run the following script:
This script will help identify potential hardcoded strings that might need translation and show existing uses of the
tfunction in the files mentioned in the PR objectives.✅ Verification successful
Translation Approach Verified
All relevant strings in the specified files are consistently using the
tfunction for translations. No hardcoded strings requiring translation were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential translation opportunities in the files mentioned in the PR. # List of files to check files=( "packages/desktop-client/src/components/reports/DateRange.tsx" "packages/desktop-client/src/components/reports/ReportLegend.tsx" "packages/desktop-client/src/components/reports/ReportOptions.ts" "packages/desktop-client/src/components/reports/disabledList.ts" "packages/desktop-client/src/components/reports/getLiveRange.ts" "packages/desktop-client/src/components/reports/index.tsx" ) # Check for hardcoded strings that might need translation echo "Checking for potential hardcoded strings that might need translation:" rg -n --type typescript --type tsx "'[A-Z][a-z]+'" "${files[@]}" echo # Check for existing uses of the t function echo "Checking for existing uses of the t function:" rg -n --type typescript --type tsx "t\(['\"]" "${files[@]}"Length of output: 1574
Script:
#!/bin/bash # Description: Check for potential translation opportunities in the files mentioned in the PR using file extensions. # List of files to check files=( "packages/desktop-client/src/components/reports/DateRange.tsx" "packages/desktop-client/src/components/reports/ReportLegend.tsx" "packages/desktop-client/src/components/reports/ReportOptions.ts" "packages/desktop-client/src/components/reports/disabledList.ts" "packages/desktop-client/src/components/reports/getLiveRange.ts" "packages/desktop-client/src/components/reports/index.tsx" ) # Check for hardcoded strings that might need translation echo "Checking for potential hardcoded strings that might need translation:" rg -n -g "*.ts" -g "*.tsx" "'[A-Z][a-z]+'" "${files[@]}" echo # Check for existing uses of the t function echo "Checking for existing uses of the t function:" rg -n -g "*.ts" -g "*.tsx" "t\(['\"]" "${files[@]}"Length of output: 16746
packages/desktop-client/src/components/reports/DateRange.tsx (5)
2-2: LGTM: Correct import for internationalization.The import of
useTranslationandTransfrom 'react-i18next' is correctly added, which aligns with the PR objective of preparing the file for translation.
30-31: LGTM: Proper use of useTranslation hook.The
useTranslationhook is correctly implemented to obtain thetfunction for translations. This is a necessary step for internationalizing the component.
40-40: LGTM: Appropriate use of Trans component for error message.The
Transcomponent is correctly used to wrap the error message, making it translatable. This approach allows for more complex translations if needed in the future.
49-52: LGTM: Effective use of Trans component for complex string.The
Transcomponent is appropriately used here to handle a more complex string with interpolation. This approach correctly allows for translation of sentences with dynamic content.
Line range hint
1-83: Overall, excellent preparation for internationalization.The changes made to this file effectively prepare it for translation, aligning well with the PR objective. The implementation of react-i18next is correct and follows best practices. The use of both
Transcomponents for complex strings and thetfunction for simpler phrases is appropriate.There's a minor suggestion for improvement regarding the handling of word order for different languages, but overall, this is a solid foundation for internationalizing the DateRange component.
Great job on taking this important step towards making the application more accessible to a global audience!
packages/desktop-client/src/components/reports/disabledList.ts (5)
1-1: LGTM: Import statement for internationalization.The import of the
tfunction from 'i18next' is correctly placed at the top of the file, setting up the internationalization capability for the rest of the code.
24-24: LGTM: Internationalization applied to currentIntervalOptions.The changes correctly apply the
tfunction to all description fields in thecurrentIntervalOptionsarray. ThedisableIncludeproperty is appropriately left unchanged as it's a boolean value.Also applies to: 28-28, 32-32, 36-36, 40-40
92-92: LGTM: Internationalization applied to timeGraphOptions.The changes correctly apply the
tfunction to most string literals in thetimeGraphOptionsarray. This is consistent with the changes made intotalGraphOptions.As mentioned in the previous comment, please clarify if strings like 'TableGraph', 'StackedBarGraph', etc., should remain untranslated.
Also applies to: 93-93, 94-94, 100-100, 101-101, 103-103, 107-107, 108-108, 110-110
Line range hint
1-190: Overall assessment: Good progress on internationalization.This review has covered the changes made to
disabledList.tsfor internationalization. The modifications align well with the PR objectives of marking files for translation. Here's a summary of the key points:
- The
tfunction from i18next is correctly imported and used throughout the file.- Internationalization has been consistently applied to most user-facing strings in the various option arrays.
- Some strings (like graph types) were left untranslated, which may or may not be intentional.
- The overall structure and logic of the file remain intact.
Suggestions for improvement:
- Clarify whether certain strings (like 'TableGraph') should be translated.
- Consider extracting translatable strings into a separate constants file for better maintainability.
- Ensure all user-facing strings in the file are internationalized, including any that might be in unchanged sections.
- Run a static analysis tool to catch any non-internationalized strings.
These changes represent a solid step towards making the application more accessible to users who speak different languages. Great work on this internationalization effort!
58-58: LGTM with a question: Internationalization applied to totalGraphOptions.The changes correctly apply the
tfunction to most string literals in thetotalGraphOptionsarray. However, some strings like 'TableGraph', 'BarGraph', etc., are not wrapped witht.Could you clarify if strings like 'TableGraph', 'BarGraph', etc., are intentionally left untranslated? If they should be translated, consider wrapping them with the
tfunction as well.Also applies to: 60-60, 67-67, 69-69, 73-73, 74-74, 76-76, 82-82, 84-84
packages/desktop-client/src/components/reports/ReportOptions.ts (1)
1-1: Verify the import source of thetfunction.Currently,
tis imported from'i18next'. Depending on your project's configuration, you might need to importtfrom'react-i18next'to ensure proper functionality within React components.Check if the import should be adjusted:
- import { t } from 'i18next'; + import { useTranslation } from 'react-i18next'; const { t } = useTranslation();Please confirm if this change aligns with your project's localization setup.
packages/desktop-client/src/components/reports/ReportOptions.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/ReportOptions.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/ReportOptions.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/reports/DateRange.tsx (2)
20-20: LGTM: "Invalid Date" string now translatable.The hardcoded "Invalid Date" string has been correctly replaced with
t('Invalid Date'), allowing for translation. This change aligns with the internationalization efforts.Consider extracting 'Invalid Date' into a constant to improve maintainability:
const INVALID_DATE = 'Invalid Date'; // Then use it like this: if (dateParsed.toString() !== t(INVALID_DATE)) { // ... }This approach centralizes the string definition and makes it easier to update if needed.
70-83: LGTM: Content rendering now supports translation for all date comparisons.The content rendering logic for date comparisons has been successfully updated to use
<Trans>components. This change allows for the translation of all date comparison strings, including those with variables.Consider extracting the translation keys into constants to improve maintainability:
const COMPARE_DATES = 'Compare {{ formattedStartDate }} to {{ endDate }}'; const DATE_RANGE = '{{ formattedStartDate }} - {{ formattedEndDate }}'; // Then use them like this: <Trans values={{...}} i18nKey={COMPARE_DATES}> {COMPARE_DATES} </Trans> <Trans i18nKey={DATE_RANGE}> {DATE_RANGE} </Trans>This approach centralizes the translation keys and makes it easier to update them if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/reports/DateRange.tsx (3 hunks)
🧰 Additional context used
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/reports/DateRange.tsx
[failure] 46-46:
Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
🔇 Additional comments (5)
packages/desktop-client/src/components/reports/DateRange.tsx (5)
2-2: LGTM: Necessary imports for internationalization added.The new imports for
Transfrom 'react-i18next' andtfrom 'i18next' are correctly added and align with the PR objectives of preparing the file for translation.Also applies to: 5-5
39-39: LGTM: Error message now translatable.The error message has been correctly wrapped in a
<Trans>component, allowing for translation. This change is in line with the internationalization efforts and maintains the existing functionality.
54-61: LGTM: Content rendering now supports translation for 'budget' and 'average' types.The content rendering logic for 'budget' and 'average' types has been successfully updated to use the
<Trans>component. This change allows for the translation of the entire string, including the date placeholders, which is crucial for proper internationalization.The use of the
valuesprop in the<Trans>component is correct and ensures that the date variables are properly interpolated in the translated string.
87-87: LGTM: Default content rendering updated.The default content rendering has been updated to use
formattedEndDatedirectly, which is consistent with the new variable names introduced earlier in the file. This change maintains the existing functionality while aligning with the overall refactoring for internationalization.
Line range hint
1-91: Overall assessment: Successful implementation of internationalization features.The changes in this file successfully implement internationalization features using react-i18next, aligning well with the PR objectives. The functionality of the
DateRangecomponent remains intact while enabling translation of all user-facing strings.Key improvements:
- All hardcoded strings are now translatable.
- Proper use of
<Trans>components for complex translations.- Consistent handling of date formatting and comparisons.
Minor suggestions have been made for further improvements, including:
- Extracting string constants for better maintainability.
- Addressing a potential type mismatch issue.
These changes significantly enhance the component's ability to support multiple languages, contributing to the overall goal of internationalizing the application.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 46-46:
Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
a-gradina
changed the title
packages/desktop-client/src/components/reports/ReportOptions.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/ReportOptions.ts
Outdated
Show resolved
Hide resolved
|
While we wait for carkom to reply - would you mind taking a look at the VRT test? Something in this PR seems to be breaking it. |
Sure. I thought they were flaky, though. I had them failing in my previous PR as well a couple of times. EDIT: I found the culprit. I mistook |
| description: 'Daily', | ||
| defaultRange: 'This month', | ||
| description: t('Daily'), | ||
| defaultRange: t('This month'), |
There was a problem hiding this comment.
cc @carkom should these be translated?
There was a problem hiding this comment.
It's difficult to know. If you capture all of them then it should be fine. If it's translated then saved then all the options to reference are also translated then it should be fine as long as we capture them all.
However, if you save a report while the app is being translated and then switch back to English (or any other language) then the report will error out.
There was a problem hiding this comment.
Are any of these things stored in the DB? If tgey are then we definitely shouldn't be translating them.
There was a problem hiding this comment.
@a-gradina would you mind removing the translations from all the defaultRange values? I'm happy yo merge once we have that done :)
Sorry for this PR taking so long
There was a problem hiding this comment.
bump @a-gradina . I think there was a misunderstanding. You can keep the translations for description. But remove from defaultRange.
|
Sorry for the slow response all. I've been moving for most of last month. I added a comment to the conversation above. Not sure I'd recommend translating those bits without re-working how they are all saved. |
Marked the following files for translation:
packages/desktop-client/src/components/reports/DateRange.tsxpackages/desktop-client/src/components/reports/ReportOptions.tspackages/desktop-client/src/components/reports/disabledList.tspackages/desktop-client/src/components/reports/index.tsx