Move help-related items under one single menu #3648
Conversation
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. |
jfdoming
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
|
WalkthroughThe pull request introduces several changes across multiple components in a desktop application. Key modifications include the removal of the keyboard shortcut functionality for the '?' key when the input is not focused, while retaining existing shortcuts for opening files and undo/redo actions. A new 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 (
|
Migrations out of sync modal![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 (6)
packages/desktop-client/src/icons/v2/Help.tsx (1)
4-18: LGTM: SVG structure is well-defined. Consider adding accessibility attributes.The SVG element and path are correctly structured, using "currentColor" for fill, which allows for easy color customization. The icon shape appears to be appropriate for a help icon.
Consider adding the following accessibility attributes to enhance the icon's usability:
<svg {...props} xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" + role="img" + aria-label="Help icon" style={{ color: 'inherit', ...props.style, }} >These attributes will improve the icon's accessibility for screen readers and other assistive technologies.
packages/desktop-client/src/components/ThemeSelector.tsx (1)
56-56: LGTM! Consider using a constant for consistency.The addition of the
offsetproperty to thePopovercomponent is a good improvement for the visual appearance of the theme selector menu. It provides a small gap between the trigger button and the popover, which enhances the user experience.For consistency across the application, consider defining a constant for the offset value (e.g.,
POPOVER_OFFSET = 8) and using it here and in other similar components (likeLoggedInUser). This would make it easier to maintain a consistent look throughout the app.+const POPOVER_OFFSET = 8; // ... <Popover - offset={8} + offset={POPOVER_OFFSET} triggerRef={triggerRef} isOpen={menuOpen} onOpenChange={() => setMenuOpen(false)} >packages/desktop-client/src/components/Titlebar.tsx (2)
337-346: LGTM: Improved layout organization with SpaceBetween.The use of the
SpaceBetweencomponent with a consistent gap improves the layout organization and spacing between elements. This change aligns well with the PR objectives.Consider extracting the gap value to a constant or theme variable for better maintainability:
const TITLEBAR_GAP = 10; // ... <SpaceBetween gap={TITLEBAR_GAP}> {/* ... */} </SpaceBetween>This would make it easier to adjust the spacing consistently across the application if needed in the future.
345-345: LGTM: HelpMenu added as per PR objectives.The addition of the
HelpMenucomponent at the end of theSpaceBetweencomponent aligns perfectly with the PR objectives. This placement should result in the Help menu appearing on the far right of the Titlebar, as intended.Consider adding an
aria-labelto theHelpMenucomponent to improve accessibility:<HelpMenu aria-label="Help and documentation" />This would provide more context for screen reader users about the purpose of this menu.
packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx (1)
171-171: LGTM! Consider a minor clarification.The updated description accurately reflects the new functionality of opening the help menu. This change aligns well with the PR objectives of adding a new 'Help' button with a sub-menu.
For even more clarity, consider specifying the location of the help menu:
- <Shortcut shortcut="?" description="Open the help menu" /> + <Shortcut shortcut="?" description="Open the help menu in the top-right corner" />This addition would help users quickly locate the new help menu.
packages/desktop-client/src/components/HelpMenu.tsx (1)
51-69: RefactorgetPageDocsto use a mapping objectUsing a mapping object instead of a switch statement can improve the maintainability and scalability of the
getPageDocsfunction.Refactored code:
-const getPageDocs = (page: string) => { - switch (page) { - case '/budget': - return 'https://actualbudget.org/docs/getting-started/envelope-budgeting'; - case '/reports': - return 'https://actualbudget.org/docs/reports/'; - case '/schedules': - return 'https://actualbudget.org/docs/schedules'; - case '/payees': - return 'https://actualbudget.org/docs/transactions/payees'; - case '/rules': - return 'https://actualbudget.org/docs/budgeting/rules'; - case '/settings': - return 'https://actualbudget.org/docs/settings'; - default: - return 'https://actualbudget.org/docs'; - } -}; +const pageDocsMap: Record<string, string> = { + '/budget': 'https://actualbudget.org/docs/getting-started/envelope-budgeting', + '/reports': 'https://actualbudget.org/docs/reports/', + '/schedules': 'https://actualbudget.org/docs/schedules', + '/payees': 'https://actualbudget.org/docs/transactions/payees', + '/rules': 'https://actualbudget.org/docs/budgeting/rules', + '/settings': 'https://actualbudget.org/docs/settings', +}; + +const getPageDocs = (page: string) => { + return pageDocsMap[page] || 'https://actualbudget.org/docs'; +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/desktop-client/src/icons/v2/help.svgis excluded by!**/*.svgupcoming-release-notes/3648.mdis excluded by!**/*.md
📒 Files selected for processing (10)
- packages/desktop-client/src/browser-preload.browser.js (0 hunks)
- packages/desktop-client/src/components/HelpMenu.tsx (1 hunks)
- packages/desktop-client/src/components/LoggedInUser.tsx (1 hunks)
- packages/desktop-client/src/components/ThemeSelector.tsx (1 hunks)
- packages/desktop-client/src/components/Titlebar.tsx (3 hunks)
- packages/desktop-client/src/components/modals/KeyboardShortcutModal.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (0 hunks)
- packages/desktop-client/src/icons/v2/Help.tsx (1 hunks)
- packages/desktop-client/src/icons/v2/index.ts (1 hunks)
- packages/loot-core/src/client/state-types/modals.d.ts (1 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/src/browser-preload.browser.js
- packages/desktop-client/src/components/sidebar/Sidebar.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/LoggedInUser.tsx
🧰 Additional context used
🔇 Additional comments (9)
packages/desktop-client/src/icons/v2/Help.tsx (2)
1-2: LGTM: Imports are correct and appropriate.The import statements are well-structured, importing the necessary React namespace and SVGProps type for the component.
3-13: LGTM: Component declaration and props handling are well-implemented.The
SvgHelpcomponent is correctly declared as a functional component with appropriate prop types. The use of prop spreading and style merging allows for flexible and customizable usage of the icon.packages/desktop-client/src/icons/v2/index.ts (1)
24-24: LGTM: New icon export added correctly.The new export statement for
SvgHelphas been added consistently with the existing pattern in the file. This addition aligns with the PR objectives of implementing a new Help menu, as the icon will likely be used for the new Help button.To ensure the
Helpmodule exists and theSvgHelpcomponent is properly defined, let's run the following verification:✅ Verification successful
Verification Successful:
SvgHelpExport ConfirmedThe
Help.tsxfile exists and theSvgHelpcomponent is properly exported.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the Help.ts file and the SvgHelp component within it. # Test 1: Check if the Help.ts file exists if fd -t f "Help.ts" packages/desktop-client/src/icons/v2; then echo "Help.ts file found." else echo "Error: Help.ts file not found in the expected directory." exit 1 fi # Test 2: Check if SvgHelp is exported from Help.ts if rg -q "export const SvgHelp" packages/desktop-client/src/icons/v2/Help.ts; then echo "SvgHelp component found in Help.ts" else echo "Error: SvgHelp component not found in Help.ts" exit 1 fi echo "Verification completed successfully."Length of output: 420
Script:
#!/bin/bash # Description: Verify the existence of the Help.tsx file and the SvgHelp component within it. # Test 1: Check if the Help.tsx file exists if fd -t f "Help.tsx" packages/desktop-client/src/icons/v2; then echo "Help.tsx file found." else echo "Error: Help.tsx file not found in the expected directory." exit 1 fi # Test 2: Check if SvgHelp is exported from Help.tsx if rg -q "export const SvgHelp" packages/desktop-client/src/icons/v2/Help.tsx; then echo "SvgHelp component found in Help.tsx" else echo "Error: SvgHelp component not found in Help.tsx" exit 1 fi echo "Verification completed successfully."Length of output: 393
packages/loot-core/src/client/state-types/modals.d.ts (1)
288-288: LGTM: New 'keyboard-shortcuts' modal type added.The addition of the 'keyboard-shortcuts' modal type with
EmptyObjectis appropriate and aligns well with the PR objectives. This new type will support the implementation of the 'Keyboard shortcuts' option in the Help menu.packages/desktop-client/src/components/Titlebar.tsx (3)
32-32: LGTM: New imports align with PR objectives.The addition of
SpaceBetweenandHelpMenuimports is consistent with the PR's goal of implementing a new Help menu and improving layout organization.Also applies to: 35-35
Line range hint
1-346: Overall: Excellent implementation of PR objectives.The changes to the Titlebar component successfully implement the new Help menu and improve layout organization as intended. The use of the
SpaceBetweencomponent enhances consistency in spacing, and the addition of theHelpMenucomponent achieves the main goal of the PR.A few minor suggestions have been made to further improve maintainability and accessibility, but these are not critical. The implementation is solid and ready for merging after addressing the small nitpicks if desired.
Great work on this PR!
278-278: LGTM: Padding adjustment, but verify layout.The padding change (right padding reduced by 5px) is likely to accommodate the new Help menu. However, please ensure that this asymmetrical padding (10px right, 15px left) doesn't cause any layout issues across different screen sizes.
✅ Verification successful
Padding adjustment verified.
The change to
padding: '0 10px 0 15px'inTitlebar.tsxis confirmed and does not introduce any layout issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any other padding adjustments in the codebase that might be related to this change. rg --type typescript --type javascript 'padding:.*10px.*15px' ./packages/desktop-client/srcLength of output: 128
Script:
#!/bin/bash # Description: Check for any other padding adjustments in the codebase that might be related to this change. rg --type js --type ts 'padding:\s*["'\''0\s*10px\s*0\s*15px"'\'']' ./packages/desktop-client/srcLength of output: 26251
packages/desktop-client/src/components/HelpMenu.tsx (2)
90-90: Verify the keyboard shortcut does not conflict with existing shortcutsThe
shift+?keyboard shortcut may conflict with browser or system-level shortcuts, which could hinder the accessibility of the help menu.Please check for any conflicts with existing shortcuts on supported platforms. If conflicts exist, consider choosing an alternative shortcut.
79-88:⚠️ Potential issueEnsure type safety for menu item selection
The
handleItemSelectfunction receivesitemfrom theMenu, but the type ofitemmay not be strictly enforced, potentially leading to runtime errors if unexpected values are passed.Explicitly type the
itemparameter to enhance type safety:-const handleItemSelect = (item: HelpMenuItem) => { +const handleItemSelect = (item: string) => { + const menuItem = item as HelpMenuItem; + switch (menuItem) {Ensure that the
Menucomponent'sitemsprop aligns with theHelpMenuItemtype to maintain consistency.Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/util/router-tools.ts (1)
13-15: LGTM! Consider adding basic error handling.The
openUrlfunction is well-implemented, using appropriate security measures ('noopener,noreferrer') when opening external links. This aligns with the PR objectives of improving access to documentation.Consider adding basic error handling to improve robustness:
export function openUrl(url: string) { - window.open(url, '_blank', 'noopener,noreferrer'); + const newWindow = window.open(url, '_blank', 'noopener,noreferrer'); + if (!newWindow) { + console.error('Failed to open URL:', url); + // Optionally, you could show a user-friendly error message here + } }This change will log an error if the window fails to open, which can happen if pop-ups are blocked by the browser.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/HelpMenu.tsx (1 hunks)
- packages/desktop-client/src/util/router-tools.ts (1 hunks)
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: jfdoming PR: actualbudget/actual#3648 File: packages/desktop-client/src/components/HelpMenu.tsx:88-88 Timestamp: 2024-10-12T23:59:26.003Z Learning: In the `HelpMenu` component in `packages/desktop-client/src/components/HelpMenu.tsx`, when the help menu is opened using the `shift+?` keyboard shortcut, the visual opening of the menu in the top-right corner provides sufficient feedback to the user, and additional focus management is not necessary.packages/desktop-client/src/components/HelpMenu.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3648 File: packages/desktop-client/src/components/HelpMenu.tsx:88-88 Timestamp: 2024-10-12T23:59:26.003Z Learning: In the `HelpMenu` component in `packages/desktop-client/src/components/HelpMenu.tsx`, when the help menu is opened using the `shift+?` keyboard shortcut, the visual opening of the menu in the top-right corner provides sufficient feedback to the user, and additional focus management is not necessary.Learnt from: jfdoming PR: actualbudget/actual#3648 File: packages/desktop-client/src/components/HelpMenu.tsx:25-47 Timestamp: 2024-10-12T23:57:22.683Z Learning: In `packages/desktop-client/src/components/HelpMenu.tsx`, when a `<Button>` component includes text content as a child, an explicit `aria-label` may not be required for accessibility, as the text content provides the accessible name.
🪛 GitHub Check: lint
packages/desktop-client/src/components/HelpMenu.tsx
[warning] 27-27:
't' is assigned a value but never used. Allowed unused vars must match /^(_|React)/u
🔇 Additional comments (2)
packages/desktop-client/src/util/router-tools.ts (1)
Line range hint
1-15: Good job on maintaining file structure and purpose.The addition of the
openUrlfunction maintains the file's focus on routing and navigation utilities. It's well-integrated and doesn't interfere with the existingExposeNavigatefunction. The export makes it readily available for use throughout the application, which aligns with the PR objectives of improving access to help-related features.packages/desktop-client/src/components/HelpMenu.tsx (1)
69-117:HelpMenucomponent implementation looks goodThe
HelpMenucomponent is well-structured and effectively manages state and user interactions.
jfdoming
force-pushed
the
jfdoming/unified-help-experience
branch
from
5c4ebbb to
f66b367
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/HelpMenu.tsx (1)
25-47: HelpButton component looks great!The component is well-structured, uses forwardRef appropriately, and is accessible with both an icon and text. Great job!
A minor suggestion for consistency:
Consider defining the
sizevariable outside the component to avoid recreating it on each render:+const ICON_SIZE = 15; const HelpButton = forwardRef<HTMLButtonElement, HelpButtonProps>( ({ onPress }, ref) => { - const size = 15; return ( // ... - <SvgHelp width={size} height={size} /> + <SvgHelp width={ICON_SIZE} height={ICON_SIZE} /> // ... ); }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (106)
packages/desktop-client/e2e/accounts.mobile.test.js-snapshots/Mobile-Accounts-opens-the-accounts-page-and-asserts-on-balances-1-chromium-linux.pngis excluded by!**/*.pngpackages/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/HelpMenu.tsx (1 hunks)
- packages/desktop-client/src/util/router-tools.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/util/router-tools.ts
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: jfdoming PR: actualbudget/actual#3648 File: packages/desktop-client/src/components/HelpMenu.tsx:88-88 Timestamp: 2024-10-12T23:59:26.003Z Learning: In the `HelpMenu` component in `packages/desktop-client/src/components/HelpMenu.tsx`, when the help menu is opened using the `shift+?` keyboard shortcut, the visual opening of the menu in the top-right corner provides sufficient feedback to the user, and additional focus management is not necessary.packages/desktop-client/src/components/HelpMenu.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3648 File: packages/desktop-client/src/components/HelpMenu.tsx:88-88 Timestamp: 2024-10-12T23:59:26.003Z Learning: In the `HelpMenu` component in `packages/desktop-client/src/components/HelpMenu.tsx`, when the help menu is opened using the `shift+?` keyboard shortcut, the visual opening of the menu in the top-right corner provides sufficient feedback to the user, and additional focus management is not necessary.Learnt from: jfdoming PR: actualbudget/actual#3648 File: packages/desktop-client/src/components/HelpMenu.tsx:25-47 Timestamp: 2024-10-12T23:57:22.683Z Learning: In `packages/desktop-client/src/components/HelpMenu.tsx`, when a `<Button>` component includes text content as a child, an explicit `aria-label` may not be required for accessibility, as the text content provides the accessible name.
🔇 Additional comments (2)
packages/desktop-client/src/components/HelpMenu.tsx (2)
1-23: Imports and type definitions look good!The imports cover all necessary dependencies for the component's functionality, and the type definitions for
HelpMenuItemandHelpButtonPropsare clear and concise.
1-116: Overall, excellent implementation of the HelpMenu component!The HelpMenu component successfully achieves the objectives outlined in the PR summary:
- It introduces a 'Help' button in the interface.
- The button opens a sub-menu with 'Documentation' and 'Keyboard shortcuts' options.
- It implements a keyboard shortcut (shift+?) to open the help menu.
The code is well-structured, follows React best practices, and effectively uses hooks for state management and side effects. The implementation of context-aware documentation links is a nice touch that enhances user experience.
Great job on this feature! The suggestions provided in the previous comments are minor and aimed at further improving code maintainability and organization.
jfdoming
force-pushed
the
jfdoming/unified-help-experience
branch
2 times, most recently
from
aa6ec0c to
c327a60
Compare
jfdoming
force-pushed
the
jfdoming/unified-help-experience
branch
from
c327a60 to
b6d9f2e
Compare
|
For new users, I think that making the help button so visible makes a lot of sense, and I agree that a unified help experience is very much helpful, but I think regular users don't need it so visible all the time. Instead I propose placing the help button under the More dropdown, along with Settings. Another option is making it above the add account button, having tiny buttons for settings and help in the sidebar below the accounts This option has overlap with #3621
This would make it more visible and likely more people who download the app to join the Discord and Github. |
|
I like the idea of moving Help out of the drop down menu to make it more visible. The upper right corner is where I would normally look to find a help icon, so I'm ok with it going there. I would also look for a settings cog in that location too, but that's not part of this PR. I mostly like @Jonathan-Fang idea, but maybe a few tweaks. I think that the Add account button would need to be more closely associated with the account groups. So, what about: Budget Name The medium buttons could be sized for 2 across at sidebar min-width. If more buttons are needed later on then the buttons could flow as the sidebar is widened. I think both ways have good points to them. |
There was a problem hiding this comment.
Brilliant! I've been thinking about how to do this recently and the top right feels so natural and doesn't intrude at all.
I do like the suggestions above with regards to expanding the menu to include things like the discord, and OC but I don't want to force the scope of this to expand - it can always be added afterwards. Definitely happy to merge as is
|
Thanks for the feedback folks! Agree that the sidebar could also be a logical place to put this. Happy to consider a future refactor to that end, once we've already consolidated the help menu. |
|
I think changing the keyboard shortcut doesn't make much sense. If you don't know what you are looking for you may have no idea that the menu opened over in the corner after pressing "?". I think it makes a lot more sense to have the keyboard shortcut show the shortcut modal than open a menu. Opening a menu by keyboard seem a bit strange since you have to then click a menu option anyway. |
Hmm okay, I see your point; though my original idea was to try to make this menu more discoverable, it's a little odd for a single character shortcut to open a menu. As far as I'm aware, it's a common pattern in desktop apps for key combinations with Alt to open menus. How would you feel about '?' for shortcuts and 'Alt-h' for help? In electron we could move the help menu to the window titlebar. |
I think that sounds fine. Im not sure what is already done in the electron app. Those things may already be in a help menu |
This change stemmed from a few things:
To start to fix these, my proposal is to put a 'Help' button in the top-right, with a sub-menu for different help-related options (just 'Documentation' and 'Keyboard shortcuts' for now). We can add a 'Tutorial' section later once we start supporting some in-app tutorials. The impact of this change is to remove the duplicate 'Help' submenu item mentioned above, and to remove the global shortcut '?' for keyboard shortcuts as this is now bound to the new menu.