Update Sidebar - Expanding Button Section #3621
Conversation
Updated PrimaryButtons.tsx to the new style expanding "More" section. Added ui.expandPrimaryButtons to prefs.d.ts Updated Sidebar.tsx to work with the new PrimaryButtons
Settings page is no longer on the expanding "More" section and must be accessed from the budget name drop down menu.
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
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
|
WalkthroughThis pull request introduces significant modifications to the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (4)
packages/loot-core/src/types/prefs.d.ts (1)
58-58: LGTM! Consider adding a brief comment for clarity.The addition of the 'ui.expandPrimaryButtons' property to the LocalPrefs type is appropriate and aligns with the PR objectives. The boolean type and naming convention are consistent with other UI-related properties.
Consider adding a brief comment above this property to explain its purpose, similar to other properties in this type. For example:
/** Whether to expand primary buttons in the UI */ 'ui.expandPrimaryButtons': boolean;This would improve code readability and maintainability.
packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
152-170: LGTM: PrimaryButtons updated with new dynamic configuration.The PrimaryButtons component has been successfully updated to accept a 'buttons' prop, allowing for more flexible and dynamic button configuration. This change aligns well with the PR objectives of modifying the expanding "More" section.
A minor suggestion for improvement:
Consider extracting the button configurations into a separate constant or variable. This would improve readability and make it easier to manage these configurations in the future. For example:
const primaryButtonConfigs = [ { title: t('Budget'), Icon: SvgWallet, to: '/budget' }, { title: t('Reports'), Icon: SvgReports, to: '/reports' }, { title: t('Schedules'), Icon: SvgCalendar, to: '/schedules' }, { title: t('Payees'), Icon: SvgStoreFront, to: '/payees', hidable: true }, { title: t('Rules'), Icon: SvgTuning, to: '/rules', hidable: true }, ]; // Then in the JSX: <PrimaryButtons buttons={primaryButtonConfigs} />This approach would make the component more maintainable and easier to update in the future.
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (2)
78-82: Simplify Conditional Style Logic for Better ReadabilityThe current conditional styling in the
styleprop of the<Item>component is functional but can be simplified for better readability. By extracting the condition into a variable, the code becomes cleaner and easier to understand.Refactor the style logic:
+ const shouldHide = item.hidable && !expanded && !isActive; return ( <Item key={item.title} title={item.title} Icon={item.Icon} to={item.to} style={{ - ...(item.hidable && - !expanded && - !isActive && { display: 'none' }), + ...(shouldHide && { display: 'none' }), }} /> );
33-33: Consider Making 'collapseSpeed' a Constant or Context ValueThe default
collapseSpeedof0.15seconds is hardcoded. If this value is used in multiple components or might need adjustments in the future, consider defining it as a constant or retrieving it from a context or theme to maintain consistency across the application.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (101)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/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-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/e2e/page-models/navigation.js (1 hunks)
- packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (2 hunks)
- packages/loot-core/src/types/prefs.d.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/e2e/page-models/navigation.js (2)
Learnt from: tlesicka PR: actualbudget/actual#3457 File: packages/desktop-client/e2e/page-models/navigation.js:50-50 Timestamp: 2024-09-25T22:11:14.624Z Learning: In the e2e tests, the budget name is 'Test Budget', so when navigating to the 'Settings' page, references to the 'More' menu are now replaced with 'Test Budget' to reflect the new placement of the 'Settings' option under the budget name dropdown.Learnt from: tlesicka PR: actualbudget/actual#3457 File: packages/desktop-client/e2e/page-models/navigation.js:50-50 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the e2e tests, the budget name is 'Test Budget', so when navigating to the 'Settings' page, references to the 'More' menu are now replaced with 'Test Budget' to reflect the new placement of the 'Settings' option under the budget name dropdown.
🔇 Additional comments (4)
packages/desktop-client/e2e/page-models/navigation.js (1)
46-50: Changes align well with PR objectives and improve test accuracy.The modifications to the
goToSettingsPagemethod are well-implemented and consistent with the PR objectives:
- Changing
settingsLinkto a 'button' role (line 46) reflects the removal of the "Settings" button from the expanding buttons section.- Updating the comment (line 48) to be more generic improves code clarity.
- Changing the button name from 'More' to 'Test Budget' (line 50) accurately represents the new UI structure in e2e tests.
These changes maintain the existing logic while adapting to the new UI layout, ensuring backwards compatibility and improving test accuracy.
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
16-23: LGTM: New icon imports added correctly.The new icon imports have been added consistently with the existing import style. These icons are likely used in the updated PrimaryButtons component, which aligns with the PR objectives of modifying the sidebar's expanding "More" section.
Line range hint
1-290: Overall changes align well with PR objectives.The modifications to the Sidebar component successfully implement the intended changes outlined in the PR objectives:
- The expanding "More" section has been updated to ensure all buttons remain in a consistent flow.
- The "Settings" button has been removed from the PrimaryButtons component, as it's accessible through the budget name drop-down menu.
- The new dynamic button configuration in PrimaryButtons allows for more flexible management of sidebar items.
These changes improve the sidebar's functionality and user experience while maintaining the overall structure of the component. The implementation is clean and consistent with the existing codebase.
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (1)
71-71: 🛠️ Refactor suggestionEnsure Accurate Path Matching for Active State
Using
location.pathname.startsWith(item.to)may lead to incorrect active states if one route's path is a prefix of another. For example,/settingswould match/settings/profile. To ensure the active state is accurate, consider usinguseMatchfromreact-router-domto perform an exact match.Here's how you can adjust the code:
-import { useLocation } from 'react-router-dom'; +import { useLocation, useMatch } from 'react-router-dom'; ... {buttons.map(item => { - const isActive = location.pathname.startsWith(item.to); + const match = useMatch({ + path: item.to, + end: false, + }); + const isActive = match !== null;
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx
Outdated
Show resolved
Hide resolved
…hub.com/tlesicka/actual into update-sidebar-expanding-button-section
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (3)
30-44: LGTM: Improved component flexibility and state managementThe new function signature with props and default values enhances the component's flexibility. The use of refs and the custom hook
useLocalPrefimproves state management and DOM manipulation.A minor suggestion for improved readability:
Consider destructuring
tfromuseTranslation()in the function parameters for consistency with the other hooks:-export function PrimaryButtons({ +export function PrimaryButtons({ collapseSpeed = 0.15, buttons, -}: PrimaryButtonsProps) { - const { t } = useTranslation(); +}: PrimaryButtonsProps & { t: ReturnType<typeof useTranslation>['t'] }) { + const { t } = useTranslation();This change would make the
tfunction a required prop, ensuring type safety for translations throughout the component.
46-50: LGTM: Proper height adjustment for animationsThe useEffect hook correctly adjusts the height based on the itemsRef's scroll height, allowing for smooth transition animations. This implementation aligns with the past learning about not setting the height to '0px' when collapsed.
For a minor performance optimization, consider memoizing the
buttonsprop if it's coming from a parent component and doesn't change frequently. This could prevent unnecessary re-renders:const memoizedButtons = useMemo(() => buttons, [buttons]);Then use
memoizedButtonsinstead ofbuttonsin your component.
52-85: LGTM: Flexible and reusable button renderingThe dynamic rendering of buttons based on the 'buttons' prop significantly improves the component's flexibility and reusability. The use of refs for managing transitions and the conditional styling for hidable items are implemented correctly.
Consider adding an
aria-expandedattribute to improve accessibility:<View ref={divRef} style={{ overflow: 'hidden', transition: 'height ' + collapseSpeed + 's ease-in-out', }} + aria-expanded={expanded} >This change will help screen readers understand the expanded state of the component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3621.mdis excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (2)
Learnt from: tlesicka PR: actualbudget/actual#3621 File: packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx:49-52 Timestamp: 2024-10-10T02:22:54.491Z Learning: In `PrimaryButtons.tsx`, setting `divRef`'s height to `itemsRef`'s scroll height allows the transition animations to work correctly because `itemsRef` changes size based on the number of items inside. Setting the height to '0px' when collapsed would prevent the buttons from showing.Learnt from: tlesicka PR: actualbudget/actual#3621 File: packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx:11-11 Timestamp: 2024-10-10T02:17:31.654Z Learning: In the codebase, the icons in `icons/v1` are named `SvgCheveronDown` and `SvgCheveronUp` with the spelling "Cheveron", and other components use the icons with this spelling. Therefore, when reviewing code in this codebase, maintain consistency with the existing icon naming, even if the spelling might seem incorrect.
🔇 Additional comments (3)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (3)
1-28: LGTM: Improved type definitions and importsThe new type definitions
PrimaryButtonItemsandPrimaryButtonsPropsenhance type safety and code readability. The additional imports support the refactored component structure.
86-91: LGTM: Proper toggle implementation for expanded stateThe SecondaryItem is correctly implemented to toggle the expanded state. The use of conditional icons and translated text for "Less" and "More" is appropriate and supports internationalization.
1-93: Overall: Excellent refactoring with improved flexibility and type safetyThe changes to the PrimaryButtons component significantly enhance its flexibility, reusability, and type safety. The introduction of prop types, dynamic button rendering, and improved state management are all positive improvements. The minor suggestions provided for readability, performance, and accessibility can further refine this already well-implemented component.
Great job on this refactoring!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (3)
30-39: LGTM! Component setup looks good with a minor suggestion.The new props structure and use of hooks enhance the component's flexibility and state management. The
useLocalPrefhook is a nice touch for persisting the expanded state locally.Consider destructuring the
tfunction fromuseTranslationin the function signature for consistency:-export function PrimaryButtons({ +export function PrimaryButtons({ collapseSpeed = 0.15, buttons, -}: PrimaryButtonsProps) { - const { t } = useTranslation(); +}: PrimaryButtonsProps) { + const { t } = useTranslation();
42-50: LGTM! Toggle function and useEffect look good with a minor optimization suggestion.The toggle function and useEffect hook are implemented correctly. The height adjustment based on
itemsRef.scrollHeightallows for proper transition animations, as explained in the past comment.Consider optimizing the useEffect hook by memoizing the
location.pathnameor using a more specific dependency:- useEffect(() => { + const currentPath = location.pathname; + useEffect(() => { if (divRef?.current && itemsRef?.current) { divRef.current.style.height = itemsRef.current.scrollHeight + 'px'; } - }, [expanded, location.pathname]); + }, [expanded, currentPath]);This change can help prevent unnecessary re-renders if other properties of the
locationobject change.
52-85: LGTM! Rendering logic looks great with a minor accessibility suggestion.The updated component structure with refs for animation and dynamic button rendering enhances flexibility and performance. The conditional styling for hiding buttons when collapsed is implemented correctly.
Consider adding an
aria-labelto the outerViewcomponent for better accessibility:- <View style={{ padding: '5px 0', flexShrink: 0 }}> + <View style={{ padding: '5px 0', flexShrink: 0 }} aria-label={t('Primary navigation buttons')}>This change will provide more context for screen readers about the purpose of this section.
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
82-88: LGTM: Well-structured primaryButtonConfigs array.The
primaryButtonConfigsarray is well-organized and aligns with the PR objectives. The use of the translation function for titles supports internationalization, and the 'hidable' property allows for flexible UI management.Consider extracting this configuration to a separate file if it grows larger in the future, to maintain the cleanliness of the Sidebar component.
160-162: LGTM: Updated PrimaryButtons component with dynamic configuration.The
PrimaryButtonscomponent now accepts abuttonsprop, which is populated with theprimaryButtonConfigsarray. This change implements a more flexible and dynamic sidebar button management system, consistent with the PR objectives.To address the lint warning, consider updating the formatting as follows:
- <PrimaryButtons - buttons={primaryButtonConfigs} - /> + <PrimaryButtons buttons={primaryButtonConfigs} />This change will improve code consistency and satisfy the linter.
🧰 Tools
🪛 GitHub Check: lint
[warning] 160-160:
Replace⏎············buttons={primaryButtonConfigs}⏎·········with·buttons={primaryButtonConfigs}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
packages/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 (2)
- packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (3 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (2)
Learnt from: tlesicka PR: actualbudget/actual#3621 File: packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx:49-52 Timestamp: 2024-10-10T02:22:54.491Z Learning: In `PrimaryButtons.tsx`, setting `divRef`'s height to `itemsRef`'s scroll height allows the transition animations to work correctly because `itemsRef` changes size based on the number of items inside. Setting the height to '0px' when collapsed would prevent the buttons from showing.Learnt from: tlesicka PR: actualbudget/actual#3621 File: packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx:11-11 Timestamp: 2024-10-10T02:17:31.654Z Learning: In the codebase, the icons in `icons/v1` are named `SvgCheveronDown` and `SvgCheveronUp` with the spelling "Cheveron", and other components use the icons with this spelling. Therefore, when reviewing code in this codebase, maintain consistency with the existing icon naming, even if the spelling might seem incorrect.
🪛 GitHub Check: lint
packages/desktop-client/src/components/sidebar/Sidebar.tsx
[warning] 160-160:
Replace⏎············buttons={primaryButtonConfigs}⏎·········with·buttons={primaryButtonConfigs}
🔇 Additional comments (4)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (2)
1-28: LGTM! Type definitions and imports look good.The new type definitions
PrimaryButtonItemsandPrimaryButtonsPropsenhance type safety and component flexibility. TheIcontype has been correctly simplified as suggested in a past comment. The "Cheveron" spelling in the icon imports is intentional and consistent with the existing codebase.
86-91: LGTM! SecondaryItem implementation looks good.The SecondaryItem for toggling the expanded state is implemented correctly. The use of the
t()function for internationalization is a good practice. The "Cheveron" spelling in the icon components is intentional and consistent with the existing codebase.packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
16-23: LGTM: New icon imports for updated sidebar.The new icon imports are consistent with the PR objectives and correspond to the new buttons being added to the sidebar. This change enhances the visual representation of the navigation options.
Line range hint
1-280: Summary: Sidebar updates successfully implement new navigation structure.The changes to the Sidebar component effectively implement the new navigation structure as outlined in the PR objectives. The code is well-organized, with clear separation of concerns between configuration and rendering. The use of internationalization and the 'hidable' property for buttons adds flexibility to the UI.
A few minor suggestions were made to further improve code quality:
- Consider extracting the
primaryButtonConfigsto a separate file if it grows larger.- Adjust the formatting of the
PrimaryButtonscomponent usage to satisfy the linter.Overall, this implementation successfully enhances the sidebar functionality while maintaining code quality and consistency.
tlesicka
changed the title
|
Having the Payees and Rules be in the More accordion looks good to me! See my idea below for having a place for Settings to live because settings living as a dropdown underneath the budget name does seem odd to me. Perhaps the below idea should be in another PR, as it could be considered an extension of scope. Although it's currently set to the top right, I proposed two ideas.
So in order it would be
Thoughts? |
…panding-button-section
|
👋 I don't quite understand why we would need to change this part of the UI? Personally the new proposal here seems a lot less polished than what we currently have in |
|
Personally, I don't think there should be a "More" section on the sidebar. In both this PR and IMO the current More section disrupts the look and flow of the sidebar. While I agree that Payees, Rules, and Settings are not as important as Budget and Reports, I don't believe that they should be a smaller button size. That is why this PR uses the sliding/expanding More section to reveal more items. I am not suggesting this next part, but mentioning it as an example. If Reports was to be an expanding section, then yes, whatever is underneath Reports should be a smaller button style. But, Payees, Rules, and Settings are not underneath any other section, so IMO they should not be smaller than the rest. I don't understand what you mean by this PR is less polished than On From a code level, currently, the Primary Buttons/expanding section are hardcoded into the PrimaryButtons component. For cleaner code and better maintainability, it would be better to have the PrimaryButtons component read a list of buttons that is given to it from the main Sidebar component. I know that the previous PR that I presented and that was merged separated the PrimaryButtons into it's own component and that was intentional. Yes, PrimaryButtons should be it's own component, but the actual content should be managed from the main Sidebar. This is the way that SecondaryButtons was merged. The main menu content should also be separated from the BudgetName component even though it is displayed on the BudgetName component (but, that's for a different PR). This allows easy maintainability by having menu items, primary buttons, and secondary buttons created as lists in the Sidebar component, while the actual code to display those items is generated from the individual components (ie modular design). I understand that there are many people who have worked on this project in the past and all have strong opinions, which is not necessarily a bad thing. I highly appreciate and respect all of you for bringing AB to the place that it is. However, it still has a bit of a rugged and unfinished look to it. I think there are quite a few people moving from other software that are used to a more finished look and feel. I know that I am one of those people and while I don't want to clone those other apps, I think that seeing what works for other software is a good way to make this app better. Neither YNAB4 nor nYNAB use an expanding more section, and both have a better menu system. |
|
I agree with @tlesicka 's notes on wanting to get AB a more polished UI, and I think the "ruggedness" is more like the current misalignment of the More toggle and everything below it, it doesn't line up with the rest of the items in the sidebar. A big question here is where to put Payees, Rules, and a pending Exchange Rates button. At this point I may even suggest adding them at the bottom of the sidebar in a slightly smaller font because they're only three buttons. And the top part of the sidebar can be the more prominent Budget, Reports, and Schedules, with a menu to rename the budget or even prepare for multi-user. Another idea is to put the Payees, Rules, and the pending Exchange Rates button inside the menu dropdown when you click the Budget Name, and then everything is in one place. The proposed PR to create the react command bar would make accessibility quite easy, but even for click-only users, knowing that there's a smooth workflow to navigating the software is key to make Actual Budget appeal to everyone, not just developers. |
|
While this is well beyond the scope of this PR, here's an idea for a way to clean up the side bar and prepare for possible future additions.
Not all elements would be active when the topbar is new, but the idea is to have a plan in place to where future things will go. The center input box where Cmd-K will eventually live would not be visible until that feature is built. The budget name there was just to add initial text. I don't care if it's the budget name or "search" or whatever wording, that will be for whoever builds that component. This would remove the menu dropdown from the budget name. (Some people have mentioned that it is odd to have the budget name as a dropdown menu.) Again, this is just a quick thrown together idea, highly based off of VS Code and will need a lot of tweaks to get the visuals to look great. But, I think it would be good to have a plan of where the design is going instead of just putting the pieces together as someone develops a piece. |
|
👋 I really appreciate the work you're doing on improving the nav, but sadly here I will have many conflicting opinions. Before we begin - here are two good reads for how we think about design in Actual
Let me go through things one by one.
Generally we try to follow the YAGNI principle here. Currently we don't have the bloat in side-nav items, so the problem does not exist. If we ever get to a point where there are too many items - we can return to solving the problem. But until then - lets not over-complicate things.
I am open to the menu settings item being removed.
Exchange rates is not a first-party feature of Actual. Similar to bank-sync: users can discover the feature when they need to use it. As such: it could be a modal or separate page, but it should not use primary real-estate in the side nav.
The margins/paddings and font size are inconsistent and not aligned.
This is a good functional change. I have no objections here.
Honestly: I've not reviewed the code. My focus here so far has been on the product and design implications. Code review comes next. On a high-level I think I won't have any objections about the implementation (but again: I've not actually reviewed it).
I would argue that inconsistent margins/paddings and mis-aligned elements makes the app look even more unfinished and rugged. But taking a step back: I would still like to understand the "why" behind this change. What does the product gain from this change?
Interesting proposal, but lets discuss it first separately before building it. |
|
@MatissJanis Although your question was directed to @tlesicka, I would also like to contribute to the discussion on the reason for this PR change. IMO, the current top-section of the sidebar as being discussed in this PR is not "sleek and clutter-free" as stated in https://actualbudget.org/docs/vision/. First, there are two competing menus [the More toggle and the dropdown budget menu] that have secondary priority items (Payees, Rules, Settings, and Help), and I recommend choosing one place if the app is supposed to be clutter-free.
Second, the Payees and Rules feel like an afterthought inside the More cabinet; it's the items inside that look misaligned compared to everything else in the sidebar. I agree with your statement that "inconsistent margins/paddings and mis-aligned elements makes the app look even more unfinished" because that's what the this top section of the sidebar in its current state (not this PR version) looks like to me. The PR version actually looks better because it attempts to address the connection between the More toggle and the items underneath. That being said, if it's this accordion setup that is disagreeable, I propose keeping the way @tlesicka has the Payees and Rules be the same button font, margins and padding as the Budget and Reports, but remaining in the More toggle. It's hide-able behind a toggle anyway, and the indented subitems is enough for me to indicate its relationship as a subitem to the mother toggle. Original I welcome any feedback on this take and comments. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (4)
18-23: Add JSDoc comments to improve type documentation.The
PrimaryButtonItemstype would benefit from JSDoc comments explaining the purpose of each field, especially thehidableproperty which affects the component's behavior.+/** + * Represents a primary button item in the sidebar + */ type PrimaryButtonItems = { + /** The text to display for the button */ title: string; + /** The icon component to render */ Icon: ComponentType<SVGProps<SVGSVGElement>>; + /** The route path for navigation */ to: string; + /** Whether the button can be hidden in collapsed state */ hidable?: boolean; };
25-33: Add prop validation for the buttons array.Consider adding runtime validation to ensure the buttons array is not empty and contains valid routes.
export function PrimaryButtons({ collapseSpeed = 0.15, buttons, }: PrimaryButtonsProps) { + if (!buttons?.length) { + console.warn('PrimaryButtons: No buttons provided'); + }
46-50: Add cleanup to useEffect.Consider adding a cleanup function to reset the height when the component unmounts.
useEffect(() => { if (divRef?.current && itemsRef?.current) { divRef.current.style.height = itemsRef.current.scrollHeight + 'px'; } + return () => { + if (divRef?.current) { + divRef.current.style.height = 'auto'; + } + }; }, [expanded, location.pathname]);
78-81: Consider extracting styles to constants.The inline styles and hardcoded margins could be moved to style constants for better maintainability.
+const buttonStyles = { + hidden: { display: 'none' }, + secondary: { marginLeft: 6, ...styles.verySmallText }, +}; style={{ - ...(shouldHide && { display: 'none' }), + ...(shouldHide && buttonStyles.hidden), }} -style={{ marginLeft: 6, ...styles.verySmallText }} +style={buttonStyles.secondary}Also applies to: 90-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (105)
packages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-import-csv-file-twice-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/accounts.test.js-snapshots/Accounts-Import-Transactions-imports-transactions-from-a-CSV-file-3-chromium-linux.pngis excluded by!**/*.pngpackages/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-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!**/*.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 (2)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx(1 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loot-core/src/types/prefs.d.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx (2)
Learnt from: tlesicka
PR: actualbudget/actual#3621
File: packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx:49-52
Timestamp: 2024-10-10T02:22:54.491Z
Learning: In `PrimaryButtons.tsx`, setting `divRef`'s height to `itemsRef`'s scroll height allows the transition animations to work correctly because `itemsRef` changes size based on the number of items inside. Setting the height to '0px' when collapsed would prevent the buttons from showing.
Learnt from: tlesicka
PR: actualbudget/actual#3621
File: packages/desktop-client/src/components/sidebar/PrimaryButtons.tsx:11-11
Timestamp: 2024-10-10T02:17:31.654Z
Learning: In the codebase, the icons in `icons/v1` are named `SvgCheveronDown` and `SvgCheveronUp` with the spelling "Cheveron", and other components use the icons with this spelling. Therefore, when reviewing code in this codebase, maintain consistency with the existing icon naming, even if the spelling might seem incorrect.
| <View style={{ padding: '5px 0', flexShrink: 0 }}> | ||
| <View | ||
| ref={divRef} | ||
| style={{ | ||
| overflow: 'hidden', | ||
| transition: 'height ' + collapseSpeed + 's ease-in-out', | ||
| }} | ||
| aria-expanded={expanded} | ||
| > | ||
| <View | ||
| ref={itemsRef} | ||
| style={{ | ||
| flexShrink: 0, | ||
| height: 'auto', | ||
| }} | ||
| > | ||
| {buttons.map(item => { | ||
| const isActive = location.pathname.startsWith(item.to); | ||
| const shouldHide = item.hidable && !expanded && !isActive; | ||
| return ( | ||
| <Item | ||
| key={item.title} | ||
| title={item.title} | ||
| Icon={item.Icon} | ||
| to={item.to} | ||
| style={{ | ||
| ...(shouldHide && { display: 'none' }), | ||
| }} | ||
| /> | ||
| ); | ||
| })} | ||
| </View> | ||
| </View> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility attributes.
The expandable section should have additional ARIA attributes for better screen reader support.
<View
ref={divRef}
style={{
overflow: 'hidden',
transition: 'height ' + collapseSpeed + 's ease-in-out',
}}
aria-expanded={expanded}
+ role="region"
+ aria-label={t('Additional navigation options')}
>Committable suggestion skipped: line range outside the PR's diff.
|
@MatissJanis I looked back at the changes that have happened to the sidebar and specifically the expanding More section. Now I understand your perspective a little better. Or at least, I think I understand it a little better. There has already been some cleanup of the sidebar and there was a move away from the menu in favor of putting more items on the sidebar. While that is a good thought in finding things that would otherwise be hidden in a menu, it also tends to clutter things up. You, having seen things before and now the way they are, are looking at this from an improvement standpoint. And I get that. But, it still doesn't have the final polish that in my opinion it could and should have. In my opinion it still looks a little hacked together. So, to answer your primary question:
This change was initially proposed to remove the More section completely. I agree with you when you said:
This is the "why" behind this PR. The current more section in both the expanded and collapsed state is not sleek nor clutter-free and looks mis-aligned, even though it is more clutter-free than what it was.
That is why in #3414 I initially proposed removing it and assigning the Payees and Rules as menu items instead of action buttons. I think I understand now that removing the expanding More section is not going to happen, even though I (and I believe others too) would think that this would clean up the sidebar. If I am mistaken in my understanding of your position, let me know. I truly want to make this product look amazing.
Thank you for letting me know this. I looked at it and you are correct. I adjusted it a bit at the button level. The space between the icon and the text needs to be adjusted but I opted to not work on that until we have a final consensus on the status of this PR. Also, the font size can be brought up to a full size button, that is not a big deal. |
…panding-button-section
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
17-24: Consider consolidating icon versions for consistency.The icons are split between v1 and v2 imports. This could lead to visual inconsistencies if the icon styles differ between versions.
Consider moving all icons to the same version to ensure consistent styling across the sidebar.
Line range hint
208-212: Reconsider Settings button placement for better UX.Based on the PR discussion, placing the Settings option in the budget name dropdown might not be the most intuitive location for users. Consider the following alternatives suggested in the PR comments:
- Place Settings as a smaller button below the accounts section
- Keep Settings in the main navigation area for better visibility
The current implementation, while functional, might make the Settings option less discoverable for users.
Would you like to explore alternative implementations for the Settings button placement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/sidebar/Sidebar.tsx(3 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
83-89: LGTM! Well-structured button configuration.
The primaryButtonConfigs array is well-organized with:
- Consistent structure for each button
- Proper translation integration
- Clear routing paths
- Appropriate hidable flags for Payees and Rules
161-161: LGTM! Clean integration of PrimaryButtons.
The PrimaryButtons component is properly integrated with the new configuration array.
…panding-button-section
|
Apologies for my bluntness: I still do not see a good argument for moving the "more" link around. From a design perspective - I still think our current implementation has an edge over the proposal here (which still has paddings/margins all over the place, mismatching fonts, etc.). On a functional level - the "settings" link must remain in the side-nav. Because of these three things: I will not be approving this PR. |
Continuation of splitting #3457 into several PR's as requested by @MatissJanis.
This PR updates the expanding "More" section to keep all buttons in the same flow instead of being divided by the "More".
"Settings" is removed from the expanding buttons section as it can be accessed from the budget name drop down menu and does not need to be in two places.
Previous discussions can be found:
Issue #3414
PR #3457