Add context menu's #3381
Add context menu's #3381
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…_menu # Conflicts: # packages/desktop-client/src/components/transactions/TransactionsTable.jsx
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
|
Had a sneak peak, Chrome on w11: Transaction table: the right click menu isn't dismissed on left click, so it's possible to right click and bring up a menu, left click to try dismiss(doesn't), then right click again and get two multiple menus:
Budget table: |
|
This is really cool! Quick observation: I noticed that the positioning of the menu is not quite under the cursor—for transactions, rules, and payees, it's a few pixels below it, and for schedules, it seems to always be at the bottom of the entry. I think folks are more likely used to context menus appearing directly at the cursor. Would it be possible to make it behave that way instead? |
|
@jfdoming I changed the positioning on the context menu. I didn't do this on the budget page because it didn't really work. |
|
Is it worth having context menus in the budget table? They were already viewable with a regular click. I think it would be worthwile to have the budget table bulk edit options available in the context menu when multiple transactions are selected |
My intuition is to right click to see the option. I constantly right click when wanting to do these actions. |
next steps didn't work
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx (1)
19-19: Reorder imports according to conventions.The
useFeatureFlagimport should be grouped with other hook imports from the application.Apply this diff to fix the import ordering:
import React, { useRef, useState, type CSSProperties, useCallback, } from 'react'; import { envelopeBudget } from 'loot-core/src/client/queries'; + import { useFeatureFlag } from '../../../../hooks/useFeatureFlag'; import { Popover } from '../../../common/Popover'; import { View } from '../../../common/View'; import { CoverMenu } from '../CoverMenu'; import { useEnvelopeSheetValue } from '../EnvelopeBudgetComponents'; import { HoldMenu } from '../HoldMenu'; import { TransferMenu } from '../TransferMenu'; import { ToBudgetAmount } from './ToBudgetAmount'; import { ToBudgetMenu } from './ToBudgetMenu'; - import { useFeatureFlag } from '../../../../hooks/useFeatureFlag';🧰 Tools
🪛 GitHub Check: lint
[warning] 19-19:
../../../../hooks/useFeatureFlagimport should occur before import of../../../common/Popover
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx(6 hunks)packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetAmount.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudgetAmount.tsx
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx
[warning] 19-19:
../../../../hooks/useFeatureFlag import should occur before import of ../../../common/Popover
[warning] 46-46:
React Hook useCallback has an unnecessary dependency: 'triggerRef'. Either exclude it or remove the dependency array
🔇 Additional comments (2)
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx (2)
59-59: LGTM! Good use of feature flag.
The feature flag integration is well-implemented, allowing for controlled rollout of the context menu functionality.
70-74: LGTM! Well-structured context menu handler.
The context menu handler is properly implemented with:
- Feature flag check
- Prevention of default behavior
- Clear action handling
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx
Outdated
Show resolved
Hide resolved
|
Fixxed after approval:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (1)
149-192: LGTM: Well-structured menu implementation with proper error handlingThe menu implementation correctly handles conditional items and follows the codebase standards for null checks and error handling.
Consider adding keyboard accessibility support for the context menu. This could include:
- Handling the 'Menu' or 'Applications' key press
- Adding
role="menu"andaria-label- Supporting Escape key to close the menu
Example implementation:
<Row ref={triggerRef} + onKeyDown={e => { + if (!contextMenusEnabled) return; + if (e.key === 'Menu' || (e.key === 'F10' && e.shiftKey)) { + e.preventDefault(); + setMenuOpen(true); + const rect = triggerRef.current.getBoundingClientRect(); + setCrossOffset(rect.width / 2); + setOffset(0); + } + }} // ... existing props > <Popover // ... existing props + role="menu" + aria-label={t('Payee actions')} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/payees/PayeeTable.tsx(1 hunks)packages/desktop-client/src/components/payees/PayeeTableRow.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (10)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-168
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In this codebase, it's acceptable to use `&&` to conditionally include items in arrays, even if it may result in falsey values. The `Menu` component handles falsey values appropriately.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-168
Timestamp: 2024-09-24T20:35:56.009Z
Learning: In this codebase, it's acceptable to use `&&` to conditionally include items in arrays, even if it may result in falsey values. The `Menu` component handles falsey values appropriately.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:143-143
Timestamp: 2024-09-24T20:35:06.300Z
Learning: In the `PayeeTableRow` component, the vertical offset for menu positioning is correctly calculated using `setOffset(e.clientY - rect.bottom);` due to the menu alignment.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:143-143
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In the `PayeeTableRow` component, the vertical offset for menu positioning is correctly calculated using `setOffset(e.clientY - rect.bottom);` due to the menu alignment.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:184-185
Timestamp: 2024-09-24T20:32:31.244Z
Learning: In this codebase, throwing errors in the default case of switch statements within UI event handlers is the standard practice.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:184-185
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In this codebase, throwing errors in the default case of switch statements within UI event handlers is the standard practice.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-09-24T20:31:48.061Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-10-08T15:46:15.739Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-159
Timestamp: 2024-09-24T20:31:09.185Z
Learning: When checking with `null`, prefer using `==` instead of `===` to match both `null` and `undefined`, since TypeScript checking is not as strict in this project.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-159
Timestamp: 2024-10-08T15:46:15.739Z
Learning: When checking with `null`, prefer using `==` instead of `===` to match both `null` and `undefined`, since TypeScript checking is not as strict in this project.
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/payees/PayeeTable.tsx
[failure] 67-67:
Identifier expected.
[failure] 88-88:
Unexpected token. Did you mean {'>'} or >?
🪛 Biome
packages/desktop-client/src/components/payees/PayeeTable.tsx
[error] 67-67: Expected a JSX attribute but instead found '='.
Expected a JSX attribute here.
(parse)
[error] 67-67: expected ... but instead found tableNavigator
Remove tableNavigator
(parse)
🔇 Additional comments (5)
packages/desktop-client/src/components/payees/PayeeTable.tsx (2)
28-28: LGTM: Clean type definition update
The addition of onDelete to the PayeeTableProps type is well-structured and maintains type safety through the use of Pick utility type.
72-85: Verify context menu implementation in PayeeTableRow
Since this PR implements context menus, we should verify that PayeeTableRow properly handles menu positioning and dismissal, as these were mentioned as issues in the PR comments.
✅ Verification successful
Based on the shell script results, I can see that PayeeTableRow.tsx properly implements context menu functionality using Popover and Menu components. The implementation includes:
- Context menu trigger via right-click with proper positioning calculation (lines 140-147)
- Popover component with proper placement and positioning props (lines 149-158)
- Menu component with appropriate items and handlers (lines 159-191)
- Non-modal behavior to allow proper dismissal (line 157)
Let me verify the imports to ensure the components are properly imported.
Context menu implementation is properly handled
The PayeeTableRow component correctly implements context menu functionality with:
- Proper imports of Menu and Popover components
- Accurate positioning using client coordinates
- Non-modal behavior for proper dismissal
- Feature flag control for context menu enablement
- Comprehensive menu items with appropriate handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for context menu implementation in PayeeTableRow
ast-grep --pattern 'const PayeeTableRow = $_' -A 20
# Look for Popover or Menu component usage
rg -l "import.*(?:Popover|Menu)" packages/desktop-client/src/components/payees/PayeeTableRow.tsx
Length of output: 31703
Script:
#!/bin/bash
# Check imports in PayeeTableRow.tsx
rg -A 5 "^import.*(?:Popover|Menu)" packages/desktop-client/src/components/payees/PayeeTableRow.tsx
Length of output: 269
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (3)
Line range hint 2-82: LGTM: Clean imports and well-typed props
The new imports are properly organized, and the onDelete prop is correctly typed with PayeeEntity['id'].
113-118: LGTM: Clean state management with feature flag integration
The state management for the context menu is well-structured, and the feature flag integration aligns with the PR objectives for controlled rollout of context menus.
140-147: LGTM: Well-implemented context menu handler
The context menu handler is properly guarded by the feature flag and uses correct positioning calculations.
…_menu # Conflicts: # packages/desktop-client/src/components/payees/PayeeTable.tsx
UnderKoen
force-pushed
the
enhancement/context_menu
branch
from
7d1146c to
a0502ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (1)
149-171: Consider adding keyboard navigation support.While the context menu implementation is solid, consider enhancing accessibility by adding keyboard navigation support (e.g., using arrow keys to navigate menu items).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/payees/PayeeTable.tsx(1 hunks)packages/desktop-client/src/components/payees/PayeeTableRow.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (10)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-168
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In this codebase, it's acceptable to use `&&` to conditionally include items in arrays, even if it may result in falsey values. The `Menu` component handles falsey values appropriately.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-168
Timestamp: 2024-09-24T20:35:56.009Z
Learning: In this codebase, it's acceptable to use `&&` to conditionally include items in arrays, even if it may result in falsey values. The `Menu` component handles falsey values appropriately.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:143-143
Timestamp: 2024-09-24T20:35:06.300Z
Learning: In the `PayeeTableRow` component, the vertical offset for menu positioning is correctly calculated using `setOffset(e.clientY - rect.bottom);` due to the menu alignment.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:143-143
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In the `PayeeTableRow` component, the vertical offset for menu positioning is correctly calculated using `setOffset(e.clientY - rect.bottom);` due to the menu alignment.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:184-185
Timestamp: 2024-09-24T20:32:31.244Z
Learning: In this codebase, throwing errors in the default case of switch statements within UI event handlers is the standard practice.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:184-185
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In this codebase, throwing errors in the default case of switch statements within UI event handlers is the standard practice.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-09-24T20:31:48.061Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175
Timestamp: 2024-10-08T15:46:15.739Z
Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-159
Timestamp: 2024-09-24T20:31:09.185Z
Learning: When checking with `null`, prefer using `==` instead of `===` to match both `null` and `undefined`, since TypeScript checking is not as strict in this project.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:158-159
Timestamp: 2024-10-08T15:46:15.739Z
Learning: When checking with `null`, prefer using `==` instead of `===` to match both `null` and `undefined`, since TypeScript checking is not as strict in this project.
🔇 Additional comments (8)
packages/desktop-client/src/components/payees/PayeeTable.tsx (4)
28-28: LGTM: Type definition is well-structured.
The addition of 'onDelete' to PayeeTableProps is properly typed and aligns with the component's enhanced functionality.
34-38: LGTM: Clean props destructuring.
The component implementation properly handles the new onDelete prop while maintaining good code organization.
42-47: Reiterating previous feedback about ref type checking.
67-80: Verify context menu positioning behavior.
Based on the PR comments, there are concerns about context menu positioning. While the props are correctly passed, we should verify the positioning behavior in PayeeTableRow.
packages/desktop-client/src/components/payees/PayeeTableRow.tsx (4)
2-2: LGTM: Type definitions and imports are well-structured.
The new imports and type definitions are properly organized and typed, maintaining type safety with PayeeEntity.
Also applies to: 11-12, 82-82
113-117: LGTM: Context menu state management is well-implemented.
The state management for the context menu follows React best practices using appropriate hooks and refs.
140-147: LGTM: Context menu event handling is properly implemented.
The event handler correctly prevents default behavior and calculates menu positioning.
172-190: Consider handling concurrent menu actions.
The menu action handling could potentially face race conditions if multiple actions are triggered in quick succession. Consider adding a loading state or disabling the menu while an action is in progress.
|
Sorry if I missed it or this isn't the right place, but has anyone had success using this on Firefox? All I get is the default context menu. |
|
@cordlord this is currently behind a feature flag, you can enable this in the settings under experimental |
|
@UnderKoen Bingo. Didn't even think to check here first. My bad. Thanks! |
Currently only on budget page and transactions
Still todo