[WIP] Make the width of categories column in the budget view customizable #3445
Conversation
…can be correctly casted to an integer via `parseInt()` but it still contains erroneous letters.
actual-github-bot
bot
changed the title
✅ 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
|
…he received `width` already had the categories width subtracted)
|
At the moment two VRT tests are failing.
|
|
If the changes are expected then update the snapshots as part of the PR |
|
@youngcw Thanks, I'll see what I can do! I am on Linux and so almost every VRT test fails due to a lot of tiny differences in font rendering, so I'll need to have some fun with getting development env up on my TV PC. |
|
They get updated in docker for consistency. The package readme shows the process |
Ugh, I noticed docker and my brain immediately went "nah, I am good". Thanks, updated and checks are passing! Things I am least confident about:
Otherwise I tried to mimic the code as best I could. It's the biggest React codebase I see and it's very intimidating to look at. |
I do think there's #3309 which might attempt mouse resize as well |
It does mouse resize indeed. Then there's a reason to get back to this PR once the other one is merged and rewrite it to use mouse instead. |
|
Rewrite using the mouse to drag the edge or the column sounds like a great idea! It can help fill the whitespace on 1920x1080 monitors too. |
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
WalkthroughThe changes introduce a new hook, Changes
Possibly related PRs
Suggested labels
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: 3
🧹 Outside diff range and nitpick comments (24)
packages/desktop-client/src/components/budget/BudgetPageHeader.tsx (2)
19-20: LGTM: Usage ofuseLocalPrefhook for category width.The
useLocalPrefhook is correctly used to retrieve the customizable category width. The default value of 200 ensures backward compatibility with the previous implementation.Consider adding a comment explaining the purpose of this preference for better code readability:
+ // Retrieve the user-defined category width or use the default of 200 pixels const [categoryWidth = 200] = useLocalPref('category.width');
22-22: LGTM: Application ofcategoryWidthin component styling.The
categoryWidthis correctly applied to set themarginLeftstyle property, implementing the customizable width feature as intended.To improve code readability, consider extracting the margin calculation to a constant:
+ const categoryMargin = 5; + const totalMarginLeft = categoryWidth + categoryMargin; - <View style={{ marginLeft: categoryWidth + 5, flexShrink: 0 }}> + <View style={{ marginLeft: totalMarginLeft, flexShrink: 0 }}>This change would make the purpose of the additional 5 pixels more explicit and easier to maintain.
packages/desktop-client/src/components/settings/Themes.tsx (1)
18-18: Approved: Good move towards modularity.The change to import
Columnfrom './UI' instead of defining it locally is a positive step towards better code organization and reusability. This aligns well with the DRY (Don't Repeat Yourself) principle and makes the codebase more maintainable.Consider grouping related imports together. You could move this import statement next to other UI-related imports for better readability:
import { Select } from '../common/Select'; import { Text } from '../common/Text'; import { View } from '../common/View'; +import { Column, Setting } from './UI'; import { useSidebar } from '../sidebar/SidebarProvider'; -import { Column, Setting } from './UI';packages/loot-core/src/types/prefs.d.ts (2)
70-70: LGTM! Consider adding a comment for clarity.The addition of the 'category.width' property to the LocalPrefs type is well-placed and correctly typed. It aligns with the PR objective of making the category width customizable.
Consider adding a brief comment to explain the purpose and expected range of this property, similar to other properties in this type. For example:
/** Width of the category column in pixels (range: 100-1024) */ 'category.width': number;This would improve code readability and provide context for other developers.
Line range hint
1-94: Overall, the change is well-implemented and aligns with the PR objectives.The addition of the 'category.width' property to the LocalPrefs type is a good foundation for implementing customizable category widths in the budget view. This change:
- Maintains type safety by explicitly defining the new preference.
- Keeps the property optional, ensuring backward compatibility.
- Correctly places the preference in the LocalPrefs type, as it's a device-specific setting.
As the project evolves, consider the following suggestions for future improvements:
- If more UI customization options are added in the future, it might be beneficial to group them under a nested 'ui' object within LocalPrefs, similar to the existing 'ui.showClosedAccounts'.
- Consider adding runtime validation for the 'category.width' value to ensure it stays within the intended range (100-1024 pixels). This could be implemented in the logic that reads/writes this preference.
- As mentioned in the PR comments, a more user-friendly approach in the long term might be to implement draggable column resizing. When that feature is implemented, this preference could serve as the initial or default width.
packages/desktop-client/src/components/settings/UI.tsx (1)
12-31: LGTM: Well-structured Column component with a minor suggestion.The new
Columncomponent is well-implemented, using appropriate common UI elements and following good practices for layout and reusability.Consider extracting the inline styles into separate style objects for better readability and maintainability. For example:
const columnStyle = { alignItems: 'flex-start', flexGrow: 1, gap: '0.5em', width: '100%', }; const childrenContainerStyle = { alignItems: 'flex-start', gap: '1em', }; export function Column({ title, children }: ColumProps) { return ( <View style={columnStyle}> <Text style={{ fontWeight: 500 }}>{title}</Text> <View style={childrenContainerStyle}>{children}</View> </View> ); }This approach can make the component more readable and easier to maintain in the future.
packages/desktop-client/src/components/budget/rollover/budgetsummary/ToBudget.tsx (2)
33-33: LGTM: Dynamic width adjustment implementedThe implementation of
useLocalPrefforcategory.widthand its application to the Popover component's style successfully addresses the PR objective of making the categories column width customizable. This change allows for better flexibility in accommodating different category name lengths and internationalization requirements.Consider extracting the default value (200) to a named constant at the top of the file for better maintainability. For example:
const DEFAULT_CATEGORY_WIDTH = 200; // ... const [categoryWidth = DEFAULT_CATEGORY_WIDTH] = useLocalPref('category.width');This would make it easier to update the default width in the future if needed.
Also applies to: 65-65
Add Unit Tests for
ToBudgetComponentWhile an end-to-end test exists for the
ToBudgetcomponent, there are no unit tests covering its new customizable width feature. To ensure comprehensive test coverage and maintain component reliability, please add unit tests that specifically target theToBudgetcomponent's functionality.🔗 Analysis chain
Line range hint
1-114: Consider updating related testsThe changes to the
ToBudgetcomponent, while minimal, introduce new behavior that may affect existing tests.To ensure the changes are properly covered by tests, please run the following command to check for any tests related to this component:
If any test files are found, please review and update them to account for the new dynamic width behavior. If no tests are found, consider adding new tests to cover this functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files related to ToBudget component rg -t js -t jsx -t ts -t tsx --glob '**/*.test.*' 'ToBudget'Length of output: 88
Script:
#!/bin/bash # Search for test files related to ToBudget component rg --glob '**/*.test.{js,jsx,ts,tsx}' 'ToBudget'Length of output: 149
packages/desktop-client/src/components/budget/BudgetTotals.tsx (2)
29-29: LGTM:useLocalPrefhook is correctly implemented.The usage of
useLocalPrefto retrieve thecategoryWidthis appropriate and maintains backward compatibility with the default value of 200.Consider adding a comment explaining the purpose of this preference for better code readability:
// Retrieve user-defined category width or use default of 200 pixels const [categoryWidth = 200] = useLocalPref('category.width');
Line range hint
1-114: Summary: Implementation successfully addresses PR objectivesThe changes in this file effectively implement the customizable category width feature as outlined in the PR objectives. The use of the
useLocalPrefhook allows for user-defined preferences while maintaining backward compatibility with the default width of 200 pixels.A few points to consider:
- The implementation is clean and consistent with React best practices.
- The changes align well with the goal of addressing the issue of category names not fitting within a fixed width.
- This implementation provides a good foundation for potential future enhancements, such as the draggable column edge mentioned in the PR comments.
As the PR author mentioned concerns about hardcoding the default width throughout the codebase, consider creating a constant for the default width in a central configuration file. This would make it easier to update the default value across the entire application if needed in the future.
Example:
// In a config file (e.g., src/config/uiDefaults.ts) export const DEFAULT_CATEGORY_WIDTH = 200; // In BudgetTotals.tsx import { DEFAULT_CATEGORY_WIDTH } from '../../config/uiDefaults'; // ... const [categoryWidth = DEFAULT_CATEGORY_WIDTH] = useLocalPref('category.width');This approach would address the concern about dependencies and make the codebase more maintainable.
packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx (2)
16-16: LGTM: SimplifiedestimatedTableWidthcalculation.The removal of the fixed subtraction aligns with the goal of making the category width customizable. However, consider updating the
getNumPossibleMonthsfunction to account for the variable category width explicitly.You might want to update the function signature and implementation to include the
categoryWidthas a parameter:function getNumPossibleMonths(width: number, categoryWidth: number) { const estimatedTableWidth = width - categoryWidth; // Rest of the function remains the same }This change would make the function more flexible and accurately reflect the customizable nature of the category width.
50-53: LGTM: Introduction ofuseLocalPrefand updated calculations.The changes successfully implement the customizable category width feature:
- The
useLocalPrefhook retrieves the user-defined category width.- The
numPossiblecalculation now accounts for the variable category width.- The
maxWidthcalculation uses the dynamiccategoryWidth.These modifications align well with the PR objective.
For improved clarity, consider adding a brief comment explaining the significance of the
categoryWidthpreference:// Retrieve user-defined category width (in pixels) from local preferences const [categoryWidth = 200] = useLocalPref('category.width');packages/desktop-client/src/components/settings/Format.tsx (1)
Line range hint
1-138: LGTM! Consider further UI component abstraction.The removal of the local
Columncomponent and its import from a centralized UI module is a positive change that improves code organization and maintainability. This refactoring aligns well with the project's goals and best practices.For future improvements, consider abstracting other UI components used in this file (such as
Select,Checkbox, andText) into the centralized UI module if they are used across multiple components. This would further enhance consistency and reusability throughout the application.packages/desktop-client/src/components/budget/SidebarCategory.tsx (3)
51-51: LGTM:useLocalPrefhook is correctly implemented.The usage of
useLocalPrefto retrieve the category width is correct and follows React hooks conventions. The default value of 200 aligns with the previously hardcoded value.Consider adding a comment explaining the significance of the 'category.width' key and the default value of 200 for better code documentation:
// Retrieve the user-defined category width or use the default of 200 pixels const [categoryWidth = 200] = useLocalPref('category.width');
97-97: LGTM:categoryWidthis correctly applied to component styles.The
categoryWidthvalue is properly used to set the width of both the Popover and the main View component, successfully implementing the customizable width feature.Consider adding a minimum width constraint to ensure the component remains usable even with very small width values:
<View style={{ width: categoryWidth, minWidth: 100, // Ensure a minimum usable width // ... other styles }} >This change would align with the PR objective of allowing widths between 100 and 1024 pixels while preventing potential layout issues with extremely small widths.
Also applies to: 136-136
Hardcoded width values still present in multiple files
The verification revealed that there are remaining hardcoded width values of
200in the following files:
packages/desktop-client/src/components/table.tsxpackages/desktop-client/src/components/filters/SavedFilterMenuButton.tsxpackages/desktop-client/src/components/budget/IncomeHeader.tsxThese instances should be updated to utilize the
categoryWidthpreference to ensure consistency and flexibility across the application.🔗 Analysis chain
Line range hint
1-193: Overall assessment: Changes successfully implement customizable category width.The modifications to
SidebarCategory.tsxeffectively implement the feature for customizable category widths, aligning well with the PR objectives. The changes:
- Introduce the
useLocalPrefhook to retrieve user preferences.- Replace hardcoded width values with the dynamic
categoryWidth.- Apply the custom width to relevant components consistently.
These changes address the issue of category names not fitting within a fixed width and provide users with the ability to customize the categories column width.
While the current implementation is sound, consider the following for future improvements:
- Implement a draggable column edge for resizing, as mentioned in the PR comments. This would provide a more intuitive user experience.
- Explore the possibility of using CSS variables for the width, which could simplify updates across multiple components and stylesheets.
To ensure that the changes don't introduce any regressions, please run the following verification script:
This script will help identify any inconsistencies in the implementation of the customizable width feature across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the categoryWidth is consistently applied and that no hardcoded values remain # Test 1: Check for any remaining hardcoded width values of 200 echo "Checking for hardcoded width values:" rg --type typescript --type javascript "width:?\s*200" packages/desktop-client/src # Test 2: Verify consistent usage of categoryWidth echo "Verifying consistent usage of categoryWidth:" rg --type typescript --type javascript "categoryWidth" packages/desktop-client/src # Test 3: Check for proper import and usage of useLocalPref echo "Checking for proper import and usage of useLocalPref:" rg --type typescript --type javascript "useLocalPref.*category\.width" packages/desktop-client/srcLength of output: 672
Script:
#!/bin/bash # Description: Verify that the categoryWidth is consistently applied and that no hardcoded values remain # Test 1: Check for any remaining hardcoded width values of 200 echo "Checking for hardcoded width values:" rg --type tsx --type ts "width:?\s*200" packages/desktop-client/src # Test 2: Verify consistent usage of categoryWidth echo "Verifying consistent usage of categoryWidth:" rg --type tsx --type ts "categoryWidth" packages/desktop-client/src # Test 3: Check for proper import and usage of useLocalPref echo "Checking for proper import and usage of useLocalPref:" rg --type tsx --type ts "useLocalPref.*category\.width" packages/desktop-client/srcLength of output: 606
Script:
#!/bin/bash # Description: Verify that the categoryWidth is consistently applied and that no hardcoded values remain # Test 1: Check for any remaining hardcoded width values of 200 echo "Checking for hardcoded width values:" rg "width:?\s*200" packages/desktop-client/src --glob "*.ts" --glob "*.tsx" # Test 2: Verify consistent usage of categoryWidth echo "Verifying consistent usage of categoryWidth:" rg "categoryWidth" packages/desktop-client/src --glob "*.ts" --glob "*.tsx" # Test 3: Check for proper import and usage of useLocalPref echo "Checking for proper import and usage of useLocalPref:" rg "useLocalPref.*category\.width" packages/desktop-client/src --glob "*.ts" --glob "*.tsx"Length of output: 4188
packages/desktop-client/src/components/budget/SidebarGroup.tsx (3)
56-56: LGTM:useLocalPrefhook is correctly implemented.The
useLocalPrefhook is properly used to retrieve the category width preference with a default value of 200 pixels. This aligns with the PR objectives.Consider adding a comment explaining the significance of the 200px default value for better code readability:
// Default category width is 200px if no user preference is set const [categoryWidth = 200] = useLocalPref('category.width');
114-114: LGTM:categoryWidthis correctly applied to component styles.The
categoryWidthvariable is properly used to replace hardcoded values in both thePopoverandViewcomponents' style props. This change allows for dynamic sizing based on user preferences, which aligns with the PR objectives.For consistency and to avoid potential bugs in the future, consider using a constant for the default width. This could be defined at the top of the file or in a separate constants file:
const DEFAULT_CATEGORY_WIDTH = 200; // Then use it like this: const [categoryWidth = DEFAULT_CATEGORY_WIDTH] = useLocalPref('category.width');This approach would ensure that the default width is consistent throughout the component and make it easier to update in the future if needed.
Also applies to: 159-159
Found remaining hardcoded width values in multiple files.
The verification scripts have identified several instances where
categoryWidthis still hardcoded to200within the following files:
SidebarGroup.tsx:56SidebarCategory.tsx:51BudgetTotals.tsx:29BudgetPageHeader.tsx:19DynamicBudgetTable.tsx:50RolloverComponents.tsx:196ToBudget.tsx:33IncomeHeader.tsx:22These hardcoded values override the customizable widths intended by the recent changes. To ensure full adherence to the PR objectives:
- Remove or refactor the default assignments of
categoryWidthto allow full customization.- Verify that all components dynamically use the
categoryWidthwithout default fallbacks.Addressing these points will help maintain consistency and flexibility in category width management across the codebase.
🔗 Analysis chain
Line range hint
1-210: Overall assessment: Changes successfully implement customizable category widths.The modifications to
SidebarGroup.tsxeffectively introduce the ability to customize category widths as per the PR objectives. The implementation is clean, follows React best practices, and integrates well with the existing code structure. The use of theuseLocalPrefhook ensures that user preferences are respected while maintaining a sensible default.A few minor suggestions have been made to improve code clarity and consistency, but these are not critical issues. The changes appear to be ready for integration, pending any additional tests or verifications that may be required as part of the broader pull request.
To ensure that these changes don't introduce any unintended side effects, please run the following verification script:
This script will help identify any remaining hardcoded width values, ensure consistent usage of
categoryWidth, and check for potential layout issues that might be affected by the dynamic width.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the categoryWidth is used consistently and that there are no remaining hardcoded width values. # Test 1: Check for any remaining hardcoded width values of 200 echo "Checking for remaining hardcoded width values:" rg --type typescript -n '(width|Width).*200' packages/desktop-client/src/components/budget/ # Test 2: Verify consistent usage of categoryWidth echo "Verifying consistent usage of categoryWidth:" rg --type typescript -n 'categoryWidth' packages/desktop-client/src/components/budget/ # Test 3: Check for any potential layout issues caused by the dynamic width echo "Checking for potential layout issues:" rg --type typescript -n '(flex|flexGrow|flexShrink|flexBasis)' packages/desktop-client/src/components/budget/Length of output: 684
Script:
#!/bin/bash # Description: Verify that the categoryWidth is used consistently and that there are no remaining hardcoded width values. # Test 1: Check for any remaining hardcoded width values of 200 echo "Checking for remaining hardcoded width values:" rg --type ts -n '(width|Width).*200' packages/desktop-client/src/components/budget/ # Test 2: Verify consistent usage of categoryWidth echo "Verifying consistent usage of categoryWidth:" rg --type ts -n 'categoryWidth' packages/desktop-client/src/components/budget/ # Test 3: Check for any potential layout issues caused by the dynamic width echo "Checking for potential layout issues:" rg --type ts -n '(flex|flexGrow|flexShrink|flexBasis)' packages/desktop-client/src/components/budget/Length of output: 13154
packages/desktop-client/src/components/settings/index.tsx (1)
28-28: Summary: Changes align with PR objectives and maintain code integrity.The changes in this file are minimal and focused, successfully introducing the new DisplaySettings feature as per the PR objectives. The import statement and component placement are appropriate and non-disruptive to the existing code structure. These modifications lay the groundwork for implementing customizable category width settings in the budget view.
However, to ensure full functionality:
- Verify that the DisplaySettings component in
Display.tsxis fully implemented with the necessary logic for managing category width preferences.- Confirm that the useLocalPref hook is correctly utilized within the DisplaySettings component to handle user preferences.
- Ensure that the new settings are properly applied in the budget view components where category widths are rendered.
Consider the following architectural points:
- Evaluate whether the DisplaySettings component should include any default values or constraints for the category width setting.
- Assess the need for any migration logic to handle existing users' preferences when introducing this new setting.
- Plan for potential future expansions of display settings to ensure the current implementation is scalable.
Also applies to: 179-179
packages/desktop-client/src/components/budget/BudgetTable.jsx (3)
37-37: LGTM: New state variable for category width.The
categoryWidthstate variable is correctly implemented using theuseLocalPrefhook, allowing for persistent storage of the user's preference. The default value of 200 ensures backward compatibility with the previous implementation.Consider adding a comment to explain the significance of the default value:
const [categoryWidth = 200] = useLocalPref('category.width'); // Default width: 200px
187-187: LGTM: Dynamic width applied to category View component.The
categoryWidthstate variable is correctly used to set the width of the categoryViewcomponent, replacing the hardcoded value. This change successfully implements the customizable width feature.Consider adding an
aria-labelordata-testidattribute to thisViewcomponent to improve accessibility and testability:<View style={{ width: categoryWidth }} aria-label="category-column" />
Multiple hardcoded widths of
200found in the codebase.The verification identified several instances where
width: 200is hardcoded in various components:
LoadBackupModal.jsxImportTransactionsModal.jsxTransactionList.jsxSelectLinkedAccountsModal.jsxPlease review these instances to determine if they should be updated to use the
useLocalPref('category.width')hook or if they serve a different purpose.🔗 Analysis chain
Line range hint
1-248: Overall implementation looks good, but verify impact on other components.The changes to implement the customizable category width feature are minimal, focused, and well-implemented. The use of
useLocalPreffor persistent storage and the maintenance of backward compatibility with the default value are commendable.To ensure the changes don't negatively impact other parts of the application, please run the following verification script:
Please run the following corrected verification script to ensure all hardcoded category widths and uses of the
'category.width'preference are properly checked:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for hardcoded category widths that might need updating # Test: Search for hardcoded category widths echo "Searching for hardcoded category widths..." rg --type js --type jsx -i '(width|flex).*200' packages/desktop-client/src # Test: Search for other uses of the 'category.width' preference echo "Searching for other uses of the 'category.width' preference..." rg --type js --type jsx -i "useLocalPref.*'category.width'" packages/desktop-client/src echo "Please review the results to ensure all relevant components are updated."Length of output: 619
Script:
# #!/bin/bash # Description: Check for hardcoded category widths that might need updating # Define 'jsx' file type for ripgrep rg --type-add 'jsx:*.jsx' --type js --type jsx -i '(width|flex).*200' packages/desktop-client/src # Search for other uses of the 'category.width' preference rg --type-add 'jsx:*.jsx' --type js --type jsx -i "useLocalPref.*'category.width'" packages/desktop-client/src echo "Please review the results to ensure all relevant components are updated."Length of output: 1459
packages/desktop-client/src/components/budget/rollover/RolloverComponents.tsx (1)
196-196: LGTM: Correct usage ofuseLocalPrefhook.The
useLocalPrefhook is used correctly to retrieve the 'category.width' preference with a default value of 200. This aligns with the PR objectives of making the category width customizable.Consider adding a brief comment explaining the purpose of this preference for better code readability:
// Retrieve user-defined category width or use default of 200 pixels const [categoryWidth = 200] = useLocalPref('category.width');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (7)
packages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-checks-that-settings-page-can-be-opened-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/mobile.test.js-snapshots/Mobile-checks-that-settings-page-can-be-opened-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3445.mdis excluded by!**/*.md
📒 Files selected for processing (14)
- packages/desktop-client/src/components/budget/BudgetPageHeader.tsx (2 hunks)
- packages/desktop-client/src/components/budget/BudgetTable.jsx (2 hunks)
- packages/desktop-client/src/components/budget/BudgetTotals.tsx (4 hunks)
- packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx (2 hunks)
- packages/desktop-client/src/components/budget/SidebarCategory.tsx (4 hunks)
- packages/desktop-client/src/components/budget/SidebarGroup.tsx (4 hunks)
- packages/desktop-client/src/components/budget/rollover/RolloverComponents.tsx (4 hunks)
- packages/desktop-client/src/components/budget/rollover/budgetsummary/ToBudget.tsx (3 hunks)
- packages/desktop-client/src/components/settings/Display.tsx (1 hunks)
- packages/desktop-client/src/components/settings/Format.tsx (1 hunks)
- packages/desktop-client/src/components/settings/Themes.tsx (1 hunks)
- packages/desktop-client/src/components/settings/UI.tsx (1 hunks)
- packages/desktop-client/src/components/settings/index.tsx (2 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
🔇 Additional comments not posted (20)
packages/desktop-client/src/components/budget/BudgetPageHeader.tsx (2)
4-4: LGTM: Import statement foruseLocalPrefhook.The import statement for the
useLocalPrefhook is correctly added and follows the project's import conventions.
Line range hint
1-38: Overall implementation of customizable category width looks good.The changes in this file successfully implement the customizable category width feature as described in the PR objectives. The use of the
useLocalPrefhook is an appropriate approach for managing user preferences, and the implementation maintains backward compatibility with the default value.The changes are minimal and well-integrated into the existing component structure. They effectively address the issue of fixed category widths while allowing for user customization.
To ensure that this change doesn't introduce any unintended side effects, please run the following verification script:
This script will help identify any remaining hardcoded category widths and ensure consistent use of the
useLocalPrefhook for the category width setting.✅ Verification successful
<!-- [monologue] [task final_response_1/1] Conclude the verification process based on the shell script results. [observations] - The shell script successfully identified all instances where `useLocalPref('category.width')` is used with a default value of `200`. - No hardcoded category widths outside the use of `useLocalPref` were found. - The use of the `useLocalPref` hook is consistent across the codebase. [analysis] - The absence of hardcoded category widths ensures that the new dynamic width feature is implemented correctly. - Consistent use of the `useLocalPref` hook indicates that user preferences are managed uniformly. - The default value of `200` maintains backward compatibility without introducing fixed widths. [actions] - Since the verification confirms that there are no unintended hardcoded category widths and that `useLocalPref` is used consistently, the verification process can be concluded successfully. -->✅ Verification Successful
All category width settings are managed using the
useLocalPrefhook with a default value of200. No hardcoded category widths were found outside of this implementation, and the hook is used consistently across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any hardcoded category widths that might conflict with the new dynamic width # Test: Search for hardcoded category widths echo "Checking for hardcoded category widths:" rg --type typescript --type javascript "(?i)(category.*width|width.*category).*200" ./packages/desktop-client/src # Test: Verify that the useLocalPref hook is used consistently echo "Checking for consistent use of useLocalPref hook:" rg --type typescript --type javascript "useLocalPref\(['\"]category\.width['\"]\)" ./packages/desktop-client/srcLength of output: 502
Script:
#!/bin/bash # Description: Re-check for any hardcoded category widths and verify consistent use of useLocalPref hook using file extensions # Test: Search for hardcoded category widths echo "Checking for hardcoded category widths:" rg "(?i)(category.*width|width.*category).*200" ./packages/desktop-client/src -g "*.ts" -g "*.tsx" -A 2 # Test: Verify that the useLocalPref hook is used consistently echo "Checking for consistent use of useLocalPref hook:" rg "useLocalPref\(['\"]category\.width['\"]\)" ./packages/desktop-client/src -g "*.ts" -g "*.tsx" -A 2Length of output: 5886
packages/desktop-client/src/components/settings/Display.tsx (2)
1-14: LGTM: Imports and hooks are well-structured.The imports and hooks are appropriately chosen for the component's functionality. The use of
useLocalPreffor managing the category width preference is a good approach for persisting user settings.
1-68: Overall functionality meets requirements, with room for minor improvements.The
DisplaySettingscomponent successfully implements the customizable categories column width as described in the PR objectives. Good job on using local preferences for persistence and implementing input validation.Suggestions for improvement:
- Add visual feedback for invalid inputs (e.g., a warning message or color change).
- Consider adding a reset button to revert to the default width.
- Implement debounce on the input to reduce unnecessary updates.
To ensure the component is properly integrated, run the following script:
This script will help verify that the
DisplaySettingscomponent is properly integrated into the main settings and that theuseLocalPrefhook is used correctly for the category width.✅ Verification successful
DisplaySettings component is properly integrated and functions as expected.
The
DisplaySettingscomponent is correctly imported and utilized within the main settings component. Additionally, theuseLocalPrefhook with'category.width'is appropriately implemented across relevant components, ensuring consistent behavior.No issues were found during the verification process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of DisplaySettings component # Test: Check if DisplaySettings is imported and used in the main settings component rg --type typescript 'import.*DisplaySettings' packages/desktop-client/src/components/settings rg --type typescript '<DisplaySettings' packages/desktop-client/src/components/settings # Test: Verify the usage of useLocalPref hook with 'category.width' rg --type typescript "useLocalPref\\('category\\.width'" packages/desktop-client/srcLength of output: 382
Script:
#!/bin/bash # Description: Verify the integration of DisplaySettings component # Test: Check if DisplaySettings is imported and used in the main settings component rg --type tsx 'import.*DisplaySettings' packages/desktop-client/src/components/settings rg --type tsx '<DisplaySettings' packages/desktop-client/src/components/settings # Test: Verify the usage of useLocalPref hook with 'category.width' rg --type tsx "useLocalPref\\('category\\.width'" packages/desktop-client/srcLength of output: 340
Script:
#!/bin/bash # Description: Verify the integration of DisplaySettings component # Test: Check if DisplaySettings is imported and used in the main settings component rg --type typescriptreact 'import.*DisplaySettings' packages/desktop-client/src/components/settings rg --type typescriptreact '<DisplaySettings' packages/desktop-client/src/components/settings # Test: Verify the usage of useLocalPref hook with 'category.width' rg --type typescriptreact "useLocalPref\\('category\\.width'" packages/desktop-client/srcLength of output: 412
Script:
#!/bin/bash # Description: Verify the integration of DisplaySettings component without using --type flag # Test: Check if DisplaySettings is imported and used in the main settings component rg 'import.*DisplaySettings' packages/desktop-client/src/components/settings --glob '*.tsx' rg '<DisplaySettings' packages/desktop-client/src/components/settings --glob '*.tsx' # Test: Verify the usage of useLocalPref hook with 'category.width' rg "useLocalPref\\('category\\.width'" packages/desktop-client/src --glob '*.tsx'Length of output: 1529
packages/desktop-client/src/components/settings/UI.tsx (2)
9-9: LGTM: New import for Text component.The addition of the
Textimport is appropriate for use in the newColumncomponent.
Line range hint
1-31: Summary: New Column component aligns well with PR objectives.The introduction of the
Columncomponent enhances the settings UI structure, providing a reusable layout element that aligns with the PR's goal of improving the budget view's customizability. This component can be utilized to organize various settings, including the new display settings for category width.The implementation is clean, follows good practices, and doesn't introduce any conflicts with existing code. It sets a good foundation for further UI enhancements in the settings area.
packages/desktop-client/src/components/budget/rollover/budgetsummary/ToBudget.tsx (1)
5-5: LGTM: New import for useLocalPref hookThe import statement for
useLocalPrefis correctly placed and necessary for the new functionality being introduced in this component.packages/desktop-client/src/components/budget/BudgetTotals.tsx (2)
4-4: LGTM: Import statement foruseLocalPrefis correctly added.The new import statement for
useLocalPrefis appropriately placed and necessary for the changes implemented in this component.
49-49: LGTM:categoryWidthis correctly applied to both View and Popover components.The dynamic
categoryWidthvalue is consistently used to replace the hardcoded values, allowing for user customization of the category column width.To ensure that all instances of the hardcoded width have been replaced, please run the following verification script:
Also applies to: 82-82
✅ Verification successful
Verified: All hardcoded width values of 200 have been successfully replaced in
BudgetTotals.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining hardcoded width values of 200 in BudgetTotals.tsx # Test: Search for any remaining instances of width: 200 or width={200} rg --type typescript 'width:\s*200|width=\{200\}' packages/desktop-client/src/components/budget/BudgetTotals.tsx # If no results are found, all instances have been replaced successfullyLength of output: 149
Script:
#!/bin/bash # Description: Check for any remaining hardcoded width values of 200 in BudgetTotals.tsx # Test: Search for any remaining instances of width: 200 or width={200} rg 'width:\s*200|width=\{200\}' packages/desktop-client/src/components/budget/BudgetTotals.tsx # If no results are found, all instances have been replaced successfullyLength of output: 96
packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx (3)
8-8: LGTM: Import statement foruseLocalPrefis correctly added.The import statement for
useLocalPrefis properly placed and correctly references the hook from the parent directory.
Line range hint
1-161: Overall assessment: Changes successfully implement customizable category width.The modifications in this file effectively introduce the customizable category width feature:
- The
useLocalPrefhook is correctly implemented to retrieve user preferences.- Width calculations (
numPossible,maxWidth) are updated to use the dynamiccategoryWidth.- Changes are well-integrated and localized, minimizing potential side effects.
These changes align with the PR objectives and address the issue of fixed category widths. The implementation appears solid, with only minor suggestions for improvement provided in the previous comments.
To ensure the changes don't introduce any unintended consequences, please:
- Thoroughly test the rendering and functionality of the budget table with various category width settings.
- Verify that the changes work well with different screen sizes and resolutions.
- Consider adding unit tests to cover the new logic for calculating table dimensions with customizable widths.
50-53: Verify rendering and functionality with customizable width.The changes to
numPossibleandmaxWidthcalculations should allow theDynamicBudgetTableInnercomponent to render correctly with the customizable category width. However, it's crucial to verify that these modifications don't negatively impact the rendering or functionality of the budget table.Please run the following script to check for any potential issues in the rendering logic:
This script will help identify any remaining hardcoded width values, verify proper propagation of
categoryWidth, and check for any outstanding TODOs or FIXMEs related to width calculations.packages/desktop-client/src/components/settings/Format.tsx (1)
17-17: LGTM! Verify the importedColumncomponent's interface.The refactoring to import the
Columncomponent from a separate UI module is a good practice for code organization and reusability. This change aligns with the DRY (Don't Repeat Yourself) principle.To ensure the imported
Columncomponent maintains the same interface and functionality, please run the following script:This script will help verify that the
Columncomponent in the UI module has the same interface as the previously local one and that its usage inFormatSettingsremains consistent.packages/desktop-client/src/components/budget/SidebarCategory.tsx (1)
10-10: LGTM: Import statement foruseLocalPrefis correctly added.The new import statement for
useLocalPrefis properly placed and follows good practices for code organization.packages/desktop-client/src/components/budget/SidebarGroup.tsx (1)
6-6: LGTM: Import statement foruseLocalPrefis correctly added.The import statement for the
useLocalPrefhook is properly placed and correctly references the hook's location.packages/desktop-client/src/components/settings/index.tsx (2)
28-28: LGTM: Import statement for DisplaySettings.The import statement for the new DisplaySettings component is correctly placed and follows the existing import pattern in the file.
179-179: LGTM: Addition of DisplaySettings component.The DisplaySettings component is appropriately placed within the Settings component, grouped with other visual settings. This addition aligns with the PR objectives of introducing customizable category width settings.
To ensure the DisplaySettings component is correctly implemented, please run the following verification script:
packages/desktop-client/src/components/budget/BudgetTable.jsx (1)
Line range hint
4-4: LGTM: New hook import for local preferences.The addition of the
useLocalPrefhook import is appropriate for implementing the customizable category width feature.packages/desktop-client/src/components/budget/rollover/RolloverComponents.tsx (2)
8-8: LGTM: New hook import added correctly.The
useLocalPrefhook is imported correctly from the appropriate relative path.
Line range hint
1-474: Overall assessment: Changes successfully implement customizable category width.The modifications in this file effectively address the PR objective of making the width of the categories column customizable. Key points:
- The
useLocalPrefhook is correctly implemented to retrieve user preferences.- The default width of 200 pixels is maintained when no custom value is set.
- The
categoryWidthvariable is consistently used in relevant components.These changes provide a solution to the issue of category names not fitting within the fixed width, especially after importing budgets from nYNAB. The implementation aligns well with the described requirements and allows for future flexibility.
Suggestions for consideration:
- Add a brief comment explaining the purpose of the
categoryWidthpreference.- Verify that all relevant occurrences of hardcoded width have been updated.
Great job on your first pull request! The changes are well-implemented and address the stated objectives effectively.
| const onBlur: HTMLProps<HTMLInputElement>['onBlur'] = event => { | ||
| if (document.hasFocus()) { | ||
| const value = parseInt(event.target.value); | ||
|
|
||
| if (Number.isInteger(value)) { | ||
| const clampedValue = Math.max(100, Math.min(1024, value)); | ||
| setPrefWidth(clampedValue); | ||
| setTempWidth(clampedValue.toString()); | ||
| } else { | ||
| setTempWidth(prefWidth.toString()); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the onBlur handler for improved readability and reusability.
While the logic is correct, consider the following improvements:
- Extract the clamping logic into a separate function for reusability.
- Add explicit error handling for non-integer inputs.
- Consider using a custom hook to encapsulate this logic.
Here's a suggested refactoring:
const clampWidth = (value: number) => Math.max(100, Math.min(1024, value));
const useWidthInput = (initialWidth: number) => {
const [width, setWidth] = useState(initialWidth);
const [tempWidth, setTempWidth] = useState(initialWidth.toString());
const handleBlur = (event: React.FocusEvent<HTMLInputElement>) => {
if (document.hasFocus()) {
const value = parseInt(event.target.value, 10);
if (Number.isInteger(value)) {
const clampedValue = clampWidth(value);
setWidth(clampedValue);
setTempWidth(clampedValue.toString());
} else {
setTempWidth(width.toString());
}
}
};
return { width, tempWidth, setTempWidth, handleBlur };
};
// In your component:
const { width, tempWidth, setTempWidth, handleBlur } = useWidthInput(prefWidth);This refactoring improves readability, reusability, and separation of concerns.
| return ( | ||
| <Setting | ||
| primaryAction={ | ||
| <View | ||
| style={{ | ||
| flexDirection: 'column', | ||
| gap: '1em', | ||
| width: '100%', | ||
| }} | ||
| > | ||
| <Column title="Categories Width"> | ||
| <Text> | ||
| <Trans> | ||
| Width of the categories column in pixels. Must be between | ||
| </Trans> | ||
| </Text> | ||
| <Input | ||
| value={tempWidth} | ||
| onChange={event => setTempWidth(event.target.value)} | ||
| onBlur={onBlur} | ||
| style={{ | ||
| ':hover': { | ||
| backgroundColor: themeStyle.buttonNormalBackgroundHover, | ||
| }, | ||
| }} | ||
| /> | ||
| </Column> | ||
| </View> | ||
| } | ||
| > | ||
| <Text> | ||
| <Trans> | ||
| <strong>Display settings</strong> change how certain elements of the | ||
| interface are displayed. | ||
| </Trans> | ||
| </Text> | ||
| </Setting> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting styles and improving component structure.
The component structure is well-organized, but there are a few areas for improvement:
- Extract inline styles into a separate stylesheet or use styled-components for better maintainability.
- Consider breaking down the component into smaller, reusable parts.
- The use of
Transfor internationalization is good, but ensure all user-facing strings are wrapped with it.
Here's a suggestion for extracting styles:
import styled from 'styled-components';
const StyledInput = styled(Input)`
&:hover {
background-color: ${props => props.theme.buttonNormalBackgroundHover};
}
`;
const SettingContainer = styled(View)`
flex-direction: column;
gap: 1em;
width: 100%;
`;
// Then in your JSX:
<SettingContainer>
<Column title={<Trans>Categories Width</Trans>}>
{/* ... */}
<StyledInput
value={tempWidth}
onChange={event => setTempWidth(event.target.value)}
onBlur={handleBlur}
/>
</Column>
</SettingContainer>This approach improves code readability and maintainability by separating concerns.
| @@ -261,7 +263,7 @@ export const ExpenseCategoryMonth = memo(function ExpenseCategoryMonth({ | |||
| placement="bottom start" | |||
| isOpen={budgetMenuOpen} | |||
| onOpenChange={() => setBudgetMenuOpen(false)} | |||
| style={{ width: 200 }} | |||
| style={{ width: categoryWidth }} | |||
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Hardcoded width detected in IncomeHeader.tsx
The search revealed that width: 200 is still hardcoded in the following file:
packages/desktop-client/src/components/budget/IncomeHeader.tsx
Please replace the hardcoded value with the categoryWidth variable to maintain consistency and support dynamic width adjustment.
🔗 Analysis chain
LGTM: Consistent usage of categoryWidth in Popover components.
The categoryWidth variable is correctly used to set the width of the Popover components, replacing the hardcoded value of 200. This change successfully implements the customizable width feature as per the PR objectives.
To ensure consistency across the codebase:
If the script returns any results, please review those occurrences to determine if they should also use the categoryWidth variable.
Also applies to: 392-392
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that all occurrences of hardcoded width 200 for categories have been replaced
rg --type typescript "width: 200" packages/desktop-client/src/components/budget
Length of output: 116
Script:
#!/bin/bash
# Corrected script to verify that all occurrences of hardcoded width 200 for categories have been replaced
rg --type tsx "width: 200" packages/desktop-client/src/components/budget
Length of output: 102
Script:
#!/bin/bash
# Verify that all occurrences of hardcoded width 200 for categories have been replaced using a glob pattern
rg --glob "*.tsx" "width: 200" packages/desktop-client/src/components/budget
Length of output: 163
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
This PR was closed because it has been stalled for 5 days with no activity. |
Background
It's the smallest deal breaker I ever had but after imported my nYNAB budgets I have a lot of categories which names don't fit in the available space on the budget view and in a lot of cases I can't shorten them and keep them meaningful enough for my needs:
What I've done
A new "Display" section in configuration - it supports values from 100 to 1024 (arbitrarily selected), gracefully handles incorrect input (resets to original value).
It should properly handle most UI elements (I've tried to find all the places that hardcoded the old width of 200 px):
Notes:
Finally, this does go a bit against the design strategy:
But at the same time the default 200 is awfully small, especially with upcoming i18n as many languages are much wider than English.