Update sidebar scrolling #3554
Update sidebar scrolling #3554
Conversation
Refactored sidebar to allow scrolling in only the accounts section. Moved Item buttons to PrimaryButtons. Put Tools inside PrimaryButtons and removed Tools. Moved Add Account button to SecondaryButtons. Moved account sorting from sidebar to Accounts.
actual-github-bot
bot
changed the title
WalkthroughThis pull request introduces several modifications across multiple components in the sidebar of the desktop client. The Changes
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: 2
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/sidebar/SecondaryButtons.tsx (2)
1-6: Consider addressing the ts-strict-ignore comment.The presence of
// @ts-strict-ignoreat the beginning of the file suggests that there might be type-checking issues that need to be addressed. It's generally a good practice to resolve these issues rather than ignoring them to ensure type safety throughout the codebase.The imports look good and are appropriately specific, which is beneficial for bundle size optimization.
Can you investigate and resolve the type-checking issues that necessitate the ts-strict-ignore comment? If you need assistance, I'd be happy to help.
20-38: Clean and efficient component implementation.The
SecondaryButtonscomponent is well-implemented, following React best practices. The use ofmapto renderSecondaryItemcomponents is efficient, and thekeyprop is correctly used with a unique value.One potential improvement could be to consider memoization:
Consider wrapping the component with
React.memoto optimize re-renders, especially if this component is used in a context where its parent might re-render frequently without changing thebuttonsprop:export const SecondaryButtons = React.memo(function SecondaryButtons({ buttons }: SecondaryButtonsProps) { // ... existing implementation ... });This change would prevent unnecessary re-renders when the parent component updates but the
buttonsprop remains the same.packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (1)
10-15: Consider using consistent icon versionsThe component imports icons from both v1 and v2. While this works, it might lead to inconsistent styling across the UI. Consider using icons from the same version to maintain visual consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (96)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-creates-a-new-account-and-views-the-initial-balance-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-creates-a-new-account-and-views-the-initial-balance-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-creates-a-new-account-and-views-the-initial-balance-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-3-chromium-linux.pngis excluded by!**/*.pngpackages/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/reports.test.js-snapshots/Reports-loads-cash-flow-graph-and-checks-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-cash-flow-graph-and-checks-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-cash-flow-graph-and-checks-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-and-cash-flow-reports-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-and-cash-flow-reports-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-and-cash-flow-reports-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-graph-and-checks-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-graph-and-checks-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.js-snapshots/Reports-loads-net-worth-graph-and-checks-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/rules.test.js-snapshots/Rules-creates-a-split-transaction-rule-and-makes-sure-it-is-applied-when-creating-a-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-10-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-11-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-12-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/schedules.test.js-snapshots/Schedules-creates-a-new-schedule-posts-the-transaction-and-later-completes-it-9-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!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-split-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-creates-a-transfer-test-transaction-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-category-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.test.js-snapshots/Transactions-filters-transactions-by-date-9-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (4)
- packages/desktop-client/src/components/sidebar/Accounts.tsx (3 hunks)
- packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (3 hunks)
- packages/desktop-client/src/components/sidebar/SecondaryButtons.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (3 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/Accounts.tsx (1)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🪛 Biome
packages/desktop-client/src/components/sidebar/Accounts.tsx
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 118-118: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 147-147: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (14)
packages/desktop-client/src/components/sidebar/SecondaryButtons.tsx (2)
8-18: Well-defined types for enhanced type safety.The type definitions for
SecondaryButtonItemsandSecondaryButtonsPropsare well-structured and provide good type safety for the component. The use of a union type for theIconprop inSecondaryButtonItemsallows for flexibility in icon implementation, which is a good practice.
1-38: Summary: Well-implemented component aligning with PR objectivesThis new
SecondaryButtonscomponent is a solid implementation that aligns well with the PR objectives of refactoring the sidebar components. It provides a flexible and type-safe way to render secondary buttons, which should integrate nicely with the updated sidebar structure.The component is well-designed, using appropriate TypeScript types and following React best practices. The only minor concerns are the
ts-strict-ignorecomment, which should be addressed if possible, and the potential for optimization through memoization.Overall, this addition contributes positively to the sidebar refactoring effort, enhancing the organization and flexibility of the UI components.
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (5)
21-21: Approve component name changeThe renaming of the component from
ToolstoPrimaryButtonsaccurately reflects its updated purpose and content. This change aligns well with the PR objectives.
Line range hint
28-35: Approve useEffect hook changesThe updated
useEffecthook improves the user experience by automatically expanding the "More" section when navigating to relevant routes. This change enhances the sidebar's responsiveness to user actions.
39-41: Approve new navigation items and confirm orderThe addition of "Budget", "Reports", and "Schedules" as primary navigation items enhances the sidebar's functionality, aligning with the PR objectives. The current order seems logical, but please confirm if this is the intended sequence for these items.
Line range hint
42-69: Approve reorganization of secondary itemsThe reorganization of "Payees", "Rules", and "Settings" under the "More" section, along with the removal of the standalone Tools section, is consistent with the PR objectives. The indentation of these items improves the visual hierarchy of the sidebar.
Line range hint
1-69: Summary: Successful refactoring of PrimaryButtons componentThe changes in this file effectively transform the former
Toolscomponent into a more comprehensivePrimaryButtonscomponent. The additions of "Budget", "Reports", and "Schedules" as primary navigation items, along with the reorganization of secondary items, enhance the sidebar's functionality and improve its structure. These changes align well with the PR objectives and should contribute to a better user experience.Some minor suggestions for consideration:
- Ensure consistent use of icon versions (v1 vs v2) for visual coherence.
- Confirm the intended order of the new primary navigation items.
Overall, this refactoring successfully achieves the goals outlined in the PR objectives.
packages/desktop-client/src/components/sidebar/Accounts.tsx (1)
26-26: Verify all usages of theAccountscomponent after prop removalThe
Accountscomponent no longer accepts any props, simplifying its interface. Please ensure that all instances whereAccountsis used have been updated to reflect this change, removing any passed props to prevent potential runtime errors.To assist with this verification, you can run the following script:
✅ Verification successful
All usages of the
Accountscomponent have been updated to remove propsNo instances of the
Accountscomponent are found with props being passed. The prop removal has been successfully verified.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the Accounts component where props are passed. # Search for Accounts component usage with props rg --type tsx -A 2 -B 2 '<Accounts[^/>]*\s+\w+='Length of output: 78
Script:
#!/bin/bash # Description: Find all usages of the Accounts component in .tsx files where props are passed. # Search for Accounts component usage with props in .tsx files rg '<Accounts[^/>]*\s+\w+=' --glob '*.tsx' -A 2 -B 2Length of output: 54
Script:
#!/bin/bash # Description: Step 1 - Find all usages of the Accounts component in .tsx files. # List all occurrences of <Accounts in .tsx files rg '<Accounts' --glob '*.tsx' -A 2 -B 2 > accounts_usages.txt # Step 2 - Check if any of these usages have props being passed if grep -q '\s\w+=' accounts_usages.txt; then echo "Found usages of Accounts component with props passed." cat accounts_usages.txt | grep '\s\w+=' else echo "No usages of Accounts component with props passed found." fiLength of output: 203
packages/desktop-client/src/components/sidebar/Sidebar.tsx (6)
7-7: Import statement is correct.The import of
closeBudgetandreplaceModalfrom'loot-core/src/client/actions'ensures that these actions are available for use within the component.
16-16: Successfully importingSvgAddicon.The
SvgAddicon is imported from the correct path and will be used appropriately in theSecondaryButtonscomponent.
28-29:PrimaryButtonsandSecondaryButtonscomponents imported correctly.Importing these components aligns with the restructuring of the sidebar and enhances code organization.
135-135:<PrimaryButtons />component integrated properly.Including the
PrimaryButtonscomponent in the sidebar ensures primary actions are displayed as intended.
137-137:<Accounts />component placed appropriately.The
Accountscomponent is correctly positioned to render the accounts section within the sidebar.
139-143:<SecondaryButtons />configured with the add account button.The
SecondaryButtonscomponent receives thebuttonsprop with theAdd accountaction properly defined, including the title, icon, and click handler.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/Accounts.tsx (2)
Line range hint
26-44: Improved component structure and state managementThe refactoring of the
Accountscomponent is well done. Removing props and utilizing custom hooks for state management and data access improves the component's maintainability and reusability.Consider adding a type annotation for the
showClosedAccountsstate:const [showClosedAccounts, setShowClosedAccountsPref] = useLocalPref<boolean>( 'ui.showClosedAccounts' );This would further enhance type safety and code clarity.
79-182: Improved rendering logic with some suggestionsThe updated rendering logic is well-structured and improves the component's flexibility. The addition of the closed accounts section is a good enhancement to the user experience. However, there are a few suggestions for improvement:
Consider extracting the repeated
Accountcomponent rendering logic into a separate function to reduce code duplication and improve maintainability.The conditional checks for
failedAccountsandupdatedAccountscan be simplified using optional chaining, as suggested by the static analysis tool:failed={failedAccounts?.has(account.id)} updated={updatedAccounts?.includes(account.id)}
- Consider using the
useMemohook to memoize the filtered account lists to optimize performance, especially if the lists are large:const renderedBudgetedAccounts = useMemo(() => budgetedAccounts.map((account, i) => ( <Account key={account.id} // ... other props /> )), [budgetedAccounts, syncingAccountIds, failedAccounts, updatedAccounts] );These changes will improve the code's readability, maintainability, and potentially its performance.
🧰 Tools
🪛 Biome
[error] 147-147: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3554.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/src/components/sidebar/Accounts.tsx (3 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: tlesicka PR: actualbudget/actual#3554 File: packages/desktop-client/src/components/sidebar/Accounts.tsx:60-64 Timestamp: 2024-10-04T05:13:58.322Z Learning: The `onReorder` function in `Accounts.tsx` was moved from `Sidebar.tsx`, and the `targetId` parameter remains typed as `unknown` intentionally.packages/desktop-client/src/components/sidebar/Accounts.tsx (2)
Learnt from: tlesicka PR: actualbudget/actual#3554 File: packages/desktop-client/src/components/sidebar/Accounts.tsx:60-64 Timestamp: 2024-10-04T05:13:58.322Z Learning: The `onReorder` function in `Accounts.tsx` was moved from `Sidebar.tsx`, and the `targetId` parameter remains typed as `unknown` intentionally.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🪛 Biome
packages/desktop-client/src/components/sidebar/Accounts.tsx
[error] 147-147: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
packages/desktop-client/src/components/sidebar/Accounts.tsx (2)
4-18: Improved imports and type definitionsThe changes to the imports section are well-structured and improve the overall type safety and state management of the component. The addition of Redux hooks and actions, along with the
AccountEntitytype import, sets a solid foundation for the refactored component.
Line range hint
1-185: Overall excellent refactoring with minor suggestions for improvementThe refactoring of the
Accountscomponent is well-executed and aligns perfectly with the PR objectives. Key improvements include:
- Transition to Redux for state management, enhancing scalability and maintainability.
- Use of custom hooks for better code organization and reusability.
- Implementation of conditional rendering for different account types, improving flexibility.
- Addition of closed accounts section, enhancing user experience.
The changes have significantly improved the component's structure, maintainability, and functionality. The suggestions provided in the previous comments (improving type safety, extracting repeated logic, and optimizing performance) are minor and can be addressed to further enhance the code quality.
Great job on this refactoring! The changes provide a solid foundation for future improvements and maintain a clean, organized codebase.
🧰 Tools
🪛 Biome
[error] 147-147: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 148-148: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/sidebar/SecondaryButtons.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx
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/sidebar/PrimaryButtons.tsx (1)
9-14: Consider using consistent icon versionsThe new imports (
SvgReports,SvgWallet, andSvgCalendar) are aligned with the PR objectives. However, mixing icon versions (v1 and v2) might lead to visual inconsistencies in the UI.Consider using icons from the same version to maintain a consistent look and feel across the application. If there's a specific reason for using different versions, please document it in a comment.
packages/desktop-client/src/components/sidebar/Accounts.tsx (1)
96-180: Improved account rendering with clear separationThe new rendering logic for different account types (budgeted, off-budget, and closed) provides a clear separation and likely improves the user experience. The reuse of the
Accountcomponent for different account types demonstrates good code reuse.However, there's some duplication in the rendering logic for budgeted and off-budget accounts. Consider extracting this logic into a separate function to reduce duplication and improve maintainability. For example:
const renderAccountGroup = (accounts, title, to, query) => ( <> {accounts.length > 0 && ( <Account name={t(title)} to={to} query={query} style={{ fontWeight, marginTop: 13, marginBottom: 5, }} /> )} {accounts.map((account, i) => ( <Account key={account.id} name={account.name} account={account} connected={!!account.bank} pending={syncingAccountIds.includes(account.id)} failed={failedAccounts?.has(account.id)} updated={updatedAccounts?.includes(account.id)} to={getAccountPath(account)} query={queries.accountBalance(account)} onDragChange={onDragChange} onDrop={onReorder} outerStyle={makeDropPadding(i)} /> ))} </> );Then you can use it like this:
{renderAccountGroup(budgetedAccounts, 'For budget', '/accounts/budgeted', queries.budgetedAccountBalance())} {renderAccountGroup(offbudgetAccounts, 'Off budget', '/accounts/offbudget', queries.offbudgetAccountBalance())}This would make the component more maintainable and easier to read.
🧰 Tools
🪛 Biome
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 147-147: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Check: typecheck
[failure] 122-122:
Type '{ paddingTop: number; marginTop: number; } | null' is not assignable to type 'CSSProperties | undefined'.
[failure] 152-152:
Type '{ paddingTop: number; marginTop: number; } | null' is not assignable to type 'CSSProperties | undefined'.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (9)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-closes-an-account-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (4)
- packages/desktop-client/src/components/sidebar/Accounts.tsx (3 hunks)
- packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (2 hunks)
- packages/desktop-client/src/components/sidebar/SecondaryButtons.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/sidebar/SecondaryButtons.tsx
🧰 Additional context used
📓 Learnings (3)
📓 Common learnings
Learnt from: tlesicka PR: actualbudget/actual#3554 File: packages/desktop-client/src/components/sidebar/Accounts.tsx:60-64 Timestamp: 2024-10-04T05:13:58.322Z Learning: The `onReorder` function in `Accounts.tsx` was moved from `Sidebar.tsx`, and the `targetId` parameter remains typed as `unknown` intentionally.packages/desktop-client/src/components/sidebar/Accounts.tsx (2)
Learnt from: tlesicka PR: actualbudget/actual#3554 File: packages/desktop-client/src/components/sidebar/Accounts.tsx:60-64 Timestamp: 2024-10-04T05:13:58.322Z Learning: The `onReorder` function in `Accounts.tsx` was moved from `Sidebar.tsx`, and the `targetId` parameter remains typed as `unknown` intentionally.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
Learnt from: tlesicka PR: actualbudget/actual#3554 File: packages/desktop-client/src/components/sidebar/Accounts.tsx:60-64 Timestamp: 2024-10-04T05:13:58.322Z Learning: The `onReorder` function in `Accounts.tsx` was moved from `Sidebar.tsx`, and the `targetId` parameter remains typed as `unknown` intentionally.
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/sidebar/Accounts.tsx
[failure] 45-45:
Parameter 'drag' implicitly has an 'any' type.
[failure] 122-122:
Type '{ paddingTop: number; marginTop: number; } | null' is not assignable to type 'CSSProperties | undefined'.
[failure] 152-152:
Type '{ paddingTop: number; marginTop: number; } | null' is not assignable to type 'CSSProperties | undefined'.
🪛 Biome
packages/desktop-client/src/components/sidebar/Accounts.tsx
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 147-147: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (14)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (5)
20-20: Approved: Component name changeThe change from
ToolstoPrimaryButtonsis appropriate and aligns with the PR objectives. The new name better reflects the component's purpose in the refactored sidebar structure.
34-34: Approved: Improved useEffect dependenciesAdding
location.pathnameto the useEffect dependency array is a good improvement. This ensures that the effect runs when the route changes, making the component more responsive to navigation events.
38-40: Approved: New sidebar items enhance functionalityThe addition of new Item components for "Budget", "Reports", and "Schedules" aligns well with the PR objectives. These changes enhance the sidebar's functionality by providing quick access to important features. The use of appropriate icons for each item improves usability and visual clarity.
Line range hint
1-68: Approved: Improved sidebar structure and organizationThe overall structural changes to the PrimaryButtons component are well-executed and align with the PR objectives. The integration of the Tools section and the inclusion of both primary and secondary items create a more organized and user-friendly sidebar. The hierarchical navigation structure enhances the user experience by providing clear categorization of different features.
Line range hint
1-68: Summary: Successful refactoring of the sidebar componentOverall, the changes in this file successfully implement the refactoring of the sidebar component as outlined in the PR objectives. The new structure, including the integration of the Tools section and the addition of new navigation items, enhances the sidebar's functionality and improves the overall user experience. The code is well-organized and follows good practices.
A minor suggestion was made regarding the consistency of icon versions, but this doesn't detract from the overall quality of the implementation. Great job on this refactoring!
packages/desktop-client/src/components/sidebar/Accounts.tsx (3)
3-17: Improved state management and code organizationThe addition of Redux hooks and custom hooks enhances the component's state management capabilities and improves code organization. This change aligns with modern React practices and can lead to better maintainability and reusability of the code.
Line range hint
25-43: Enhanced component independence with hooksThe removal of props in favor of hooks makes the
Accountscomponent more self-contained, which can improve reusability and reduce prop drilling. This is a positive change that aligns with modern React practices.However, it's worth considering the trade-offs of this approach:
- The component is now more tightly coupled to the global state, which might make it harder to test in isolation.
- The component's dependencies are less explicit, as they're now hidden in the hooks rather than being visible in the props.
Consider documenting the component's dependencies (e.g., required Redux state shape) in a comment to aid future maintenance.
59-71: Verify the use of 'unknown' type for targetIdThe
onReorderfunction implementation looks good and correctly handles the case of dropping an account at the bottom of the list. However, I noticed that thetargetIdparameter is typed asunknown.While I understand from the retrieved learnings that this was intentional, it might be worth reconsidering if a more specific type could be used without breaking the existing functionality. Could you provide more context on why
unknownis preferred here? If there's a specific reason for this choice, consider adding a comment in the code to explain the rationale for future maintainers.packages/desktop-client/src/components/sidebar/Sidebar.tsx (6)
7-7: Import Statement for ActionsThe
closeBudgetandreplaceModalactions are imported correctly from'loot-core/src/client/actions'. Ensure these actions are utilized appropriately within the component.
16-16: Icon Import VerificationThe
SvgAddicon is imported from'../../icons/v1'. Confirm that this icon renders correctly where it is used.
28-29: Importing New Button ComponentsThe
PrimaryButtonsandSecondaryButtonscomponents are imported successfully. Verify that these components are correctly implemented and integrate well within the sidebar.
137-137: Adding thePrimaryButtonsComponentThe
PrimaryButtonscomponent is added to the sidebar layout. Ensure that it displays properly and that all buttons within it function as expected.
141-145: Implementing the 'Add account' Button inSecondaryButtonsThe
SecondaryButtonscomponent includes an 'Add account' button with theSvgAddicon. Verify that theonAddAccountfunction is correctly linked and that clicking the button opens the 'add-account' modal as expected.
139-139: Ensuring Account Reordering FunctionalityThe
Accountscomponent is utilized here without explicit props for reordering. Since theonReorderfunction has been moved fromSidebar.tsxtoAccounts.tsx, please verify that the account reordering via drag-and-drop continues to work correctly.Run the following script to confirm that the
onReorderfunction is defined inAccounts.tsxwith thetargetIdparameter typed asunknown, as intended:
|
LGTM too @MatissJanis |
Splitting #3457 into several PR's as requested by @MatissJanis.
Refactored sidebar to allow scrolling in only the accounts section.
Moved Item buttons to PrimaryButtons.
Put Tools inside PrimaryButtons and removed Tools.
Moved Add Account button to SecondaryButtons. Moved account sorting from sidebar to Accounts.