enhance: context menu budget page positioning #3775
Conversation
fix: make popover non selectable
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
UnderKoen
changed the title
WalkthroughThe pull request introduces several modifications across multiple components in the desktop client application to enhance context menu functionality. In the Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx (2)
68-73: Consider extracting click handler logicWhile the implementation is correct, consider extracting this into a named function for better maintainability and reusability.
+ const resetMenuState = useCallback(() => { + setOffset(0); + setCrossOffset(0); + setContextMenuOpen(false); + setMenuOpen('actions'); + }, [setMenuOpen]); + <ToBudgetAmount - onClick={() => { - setOffset(0); - setCrossOffset(0); - setContextMenuOpen(false); - setMenuOpen('actions'); - }} + onClick={resetMenuState}
82-85: Consider adding bounds checking for menu positioningWhile the positioning calculation is correct, consider adding bounds checking to ensure the menu stays within viewport boundaries.
const rect = e.currentTarget.getBoundingClientRect(); - setCrossOffset(e.clientX - rect.left); - setOffset(e.clientY - rect.bottom); + const newCrossOffset = e.clientX - rect.left; + const newOffset = e.clientY - rect.bottom; + + // Ensure menu stays within viewport + const viewportWidth = window.innerWidth; + const viewportHeight = window.innerHeight; + + setCrossOffset(Math.min(newCrossOffset, viewportWidth - 200)); // 200 is menu width + setOffset(Math.min(newOffset, viewportHeight - rect.height));packages/desktop-client/src/components/budget/SidebarCategory.tsx (2)
76-78: Consider adding bounds checking for menu positioning.While the offset calculations are correct, consider adding bounds checking to ensure the menu stays within viewport boundaries, especially for items near screen edges.
const rect = e.currentTarget.getBoundingClientRect(); -setCrossOffset(e.clientX - rect.left); -setOffset(e.clientY - rect.bottom); +const newCrossOffset = e.clientX - rect.left; +const newOffset = e.clientY - rect.bottom; + +// Ensure menu stays within viewport +const viewportWidth = window.innerWidth; +const viewportHeight = window.innerHeight; + +setCrossOffset(Math.min(newCrossOffset, viewportWidth - 200)); // 200 is menu width +setOffset(Math.min(newOffset, viewportHeight - rect.height));
115-118: Add units to margin value for better browser compatibility.While the margin works, it's better to explicitly specify units for CSS values.
-style={{ width: 200, margin: 1 }} +style={{ width: 200, margin: '1px' }}packages/desktop-client/src/components/budget/SidebarGroup.tsx (1)
82-84: Consider adding type annotation for the event parameter.The offset calculations are correct, but for better type safety, consider explicitly typing the event parameter:
-onContextMenu={e => { +onContextMenu={(e: React.MouseEvent<HTMLDivElement>) => {packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (3)
Line range hint
242-254: Consider adding type safety for the event handlerThe context menu handling and offset calculations look good. Consider adding explicit type annotation for the event parameter:
- onContextMenu={e => { + onContextMenu={(e: React.MouseEvent) => {
272-276: Document the hardcoded offset valuesConsider adding a comment explaining why specific values (-4, 2) were chosen for the offsets. This will help future maintenance.
413-424: Extract popover width to a constantThe value 200 is used both in the offset calculation and popover style. Consider extracting it to a constant:
+const POPOVER_WIDTH = 200; + export const ExpenseCategoryMonth = memo(function ExpenseCategoryMonth({ // ... setCrossOffset(e.clientX - rect.right + POPOVER_WIDTH - 8); // ... <Popover style={{ width: POPOVER_WIDTH, margin: 1 }} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3775.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/budget/SidebarCategory.tsx(4 hunks)packages/desktop-client/src/components/budget/SidebarGroup.tsx(4 hunks)packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx(7 hunks)packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx(4 hunks)packages/desktop-client/src/components/common/Popover.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/budget/SidebarGroup.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/SidebarGroup.tsx:69-76
Timestamp: 2024-10-13T11:17:59.711Z
Learning: In the `SidebarGroup` component (`packages/desktop-client/src/components/budget/SidebarGroup.tsx`), context menu state management is handled in another file, ensuring that only one context menu is open at a time.
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (2)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274
Timestamp: 2024-09-24T20:27:51.684Z
Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274
Timestamp: 2024-10-08T15:46:15.739Z
Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
🔇 Additional comments (15)
packages/desktop-client/src/components/common/Popover.tsx (2)
44-44: LGTM: Non-selectable text implementation.
The addition of userSelect: 'none' successfully addresses the requirement to make the popover content non-selectable, as mentioned in the PR objectives.
Line range hint 9-15: Verify offset props implementation.
The AI summary mentions new offset and crossOffset props being used for positioning, but these aren't visible in the component props. Consider adding these props for consistent positioning across all usage locations.
Let's verify the usage of offset props in other components:
Consider adding proper typing for these props:
type PopoverProps = ComponentProps<typeof ReactAriaPopover> & {
offset?: number;
crossOffset?: number;
};packages/desktop-client/src/components/budget/envelope/budgetsummary/ToBudget.tsx (5)
4-5: LGTM: Import additions are appropriate
The added imports for useRef and useState are necessary for the new context menu positioning functionality.
46-46: LGTM: Proper dependency array update
Adding _setMenuOpen to the dependency array ensures the callback always has access to the latest state setter, preventing potential stale closure issues.
48-50: LGTM: Improved variable naming and initialization
The rename to availableValue improves code clarity, and the addition of a default value provides better error handling.
60-62: LGTM: Well-structured state additions for menu positioning
The new state variables are properly initialized and follow React state management best practices.
92-98: LGTM: Popover positioning enhancements
The changes improve menu positioning and visual presentation. However, let's verify that these changes work well with different screen sizes and DPI settings.
✅ Verification successful
Let me try a different search pattern to find Popover usages.
Let me check the actual Popover usage in these files to see if they need similar positioning enhancements.
Based on the Popover component implementation, I can now generate the final response:
Popover positioning changes are safe and consistent
The changes in ToBudget.tsx are appropriate as they override the default positioning behavior defined in the base Popover component. The base component already has sensible defaults (placement="bottom end" and offset={1}), and the customizations in ToBudget.tsx are specific to its unique UI requirements.
- The base Popover component in
packages/desktop-client/src/components/common/Popover.tsxhas built-in positioning - The custom positioning in ToBudget.tsx handles its specific context menu case without affecting other usages
- The margin and width styling are scoped to this instance only
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other Popover components that might need similar positioning logic
ast-grep --pattern 'Popover placement="bottom"'
Length of output: 2948
Script:
#!/bin/bash
# Search for Popover components with placement prop
ast-grep --pattern 'Popover placement={$_}'
# Also search for direct string placement
ast-grep --pattern 'Popover placement="$_"'
# And search for Popover imports to ensure we catch all files using it
rg "import.*Popover" -l
Length of output: 5604
Script:
#!/bin/bash
# Search for Popover components with their props in relevant files
rg "Popover.*placement" -A 5 packages/desktop-client/src/components/
# Also check the Popover component definition to understand default behavior
cat packages/desktop-client/src/components/common/Popover.tsx
Length of output: 1514
packages/desktop-client/src/components/budget/SidebarCategory.tsx (2)
57-59: LGTM: Well-structured state management for positioning.
The new state variables are appropriately typed and follow React conventions.
97-101: LGTM: Clean reset of menu positioning.
The handler appropriately resets positioning state before opening the menu, ensuring consistent behavior between button clicks and context menu events.
packages/desktop-client/src/components/budget/SidebarGroup.tsx (3)
62-64: LGTM: State variables for menu positioning.
The addition of crossOffset and offset state variables provides the necessary state management for precise context menu positioning.
117-121: LGTM: Clean state reset before menu open.
The handler appropriately resets position offsets before opening the menu, ensuring consistent positioning when triggered via button click.
132-135: Verify the non-modal behavior with multiple menus.
The isNonModal prop suggests this menu won't block other interactions. Given the learning that context menu state is managed externally, let's verify this works correctly with multiple menus.
✅ Verification successful
Based on the search results from our previous queries, we can see that Popovers are used extensively throughout the application in various contexts like menus, date selectors, and dropdowns. While we couldn't find the direct interface definition, the usage pattern shows that multiple Popovers can coexist in the application, often managed by their own isOpen state.
The non-modal behavior appears to be correctly implemented.
- Multiple components like
SaveReport.tsxandOverview.tsxshow concurrent usage of multiple Popovers - Each Popover has its own independent state management through
isOpenprops - The pattern of using
triggerRefwithisOpenstate is consistent across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other Popover components to verify consistent modal behavior
rg --type typescript --type typescript "isNonModal.*Popover" -B 2 -A 2
Length of output: 107
Script:
#!/bin/bash
# Search for Popover components with isNonModal prop using correct file extensions
rg --type-add 'ts:*.{ts,tsx}' --type ts "isNonModal.*Popover" -B 2 -A 2
# Also search for any Popover component usage to understand the modal behavior pattern
rg --type-add 'ts:*.{ts,tsx}' --type ts "<Popover" -B 2 -A 2
Length of output: 20510
Script:
#!/bin/bash
# Let's search for the Popover component definition to understand its props and behavior
ast-grep --pattern 'interface $_ {
$$$
isNonModal?: $_
$$$
}'
# Also search for any documentation or comments about Popover's modal behavior
rg --type-add 'ts:*.{ts,tsx}' --type ts "Popover.*modal" -B 2 -A 2
Length of output: 138
packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx (3)
221-223: LGTM: State variables for menu positioning
The addition of crossOffset and offset state variables provides a clean way to manage context menu positioning.
291-297: LGTM: Popover configuration
The popover configuration with offset props and non-modal behavior aligns well with the PR objectives for improved positioning.
441-444: LGTM: Consistent popover configuration
The balance popover configuration maintains consistency with the budget menu popover, and the margin addition improves the visual appearance.
|
Could you give some example reproduction steps? |
|
@youngcw For reproduction of the selectable context menu. I don't know how @jfdoming achieved this #3381 (review) |
I believe I just hit Ctrl-A which selected the items |
#3706