feat: now button at budget page #3703
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. |
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
|
youngcw
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/budget/MonthPicker.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe changes in the pull request focus on the The overall structure of the component remains unchanged, maintaining the existing hover and selection logic. The Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/common/Link.tsx (1)
28-28: Consider standardizing event handling approach.The mix of
onClick(DOM event) andonPress(likely from an underlying UI framework) handlers suggests inconsistent event handling patterns. Consider standardizing on one approach or properly typing the event handlers to avoid type assertions.Options to consider:
- Standardize on
onPressand removeonClickto maintain consistency with the underlyingButtoncomponent- Create proper event type definitions that handle both DOM and framework-specific events
- Add proper type conversion utilities to safely handle event type differences
packages/desktop-client/src/components/budget/MonthPicker.tsx (1)
74-90: Consider enhancing the "Now" button implementation.While the basic functionality is correct, consider these improvements:
- Add aria-label for better screen reader support
- Consider using relative positioning to prevent potential layout issues
- Add loading state handling during month selection
Apply these enhancements:
<View style={{ flexDirection: 'row', alignItems: 'center', gap: 10, - position: 'absolute', - top: -34, + marginBottom: 10, }} > <Link variant="button" buttonVariant="bare" + aria-label="Jump to current month" onClick={() => onSelect(currentMonth)} > Now </Link> </View>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/budget/MonthPicker.tsx (2 hunks)
- packages/desktop-client/src/components/common/Link.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
packages/desktop-client/src/components/budget/MonthPicker.tsx (4)
8-8: LGTM: Import statement is correctly placed and necessary.The Link component import is properly organized with other component imports and is required for the new "Now" button functionality.
67-70: LGTM: Layout changes properly accommodate the new "Now" button.The flexDirection and marginTop changes create an appropriate layout structure for the new button while maintaining proper component spacing.
94-94: LGTM: Width adjustment ensures proper layout alignment.The width: '100%' property ensures the month picker properly fills the container width in the new column layout.
67-94: Verify layout changes don't affect existing MonthPicker usage.Let's verify the impact of these layout changes on existing implementations.
✅ Verification successful
Let me gather more information about the Link component since the previous search didn't yield its type definition.
Let me search for one more piece of information to find the Link component's type definition, as it might be in a different format.
Based on the gathered information, I can now provide a final response. The Link component's type definition in
packages/desktop-client/src/components/common/Link.tsxshows that it fully supports all the props used in the MonthPicker component (variant,buttonVariant,onClick), and the single usage of MonthPicker inBudgetPageHeader.tsxshows that the style prop is being used in a similar way, making these layout changes safe.Layout changes in MonthPicker are safe to proceed
The changes are verified to be safe because:
- The Link component fully supports all used props (
variant: 'button',buttonVariant: 'bare',onClick)- The only usage of MonthPicker in BudgetPageHeader.tsx already uses the style prop in a similar way
- The layout changes are localized and don't affect the component's API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MonthPicker usage in the codebase echo "Checking MonthPicker usage patterns:" rg -A 5 "<MonthPicker" # Verify Link component props echo "Verifying Link component implementation:" ast-grep --pattern 'type $_ = { $$$ variant?: $_ buttonVariant?: $_ onClick?: $_ $$$ }'Length of output: 925
Script:
#!/bin/bash # Search for Link component definition and its props echo "Searching for Link component definition:" rg -A 10 "export (type|interface) .*Link.*Props" # Alternative search for Link component echo "Alternative search for Link component:" ast-grep --pattern 'const Link = $_' # Search for Link component usage patterns echo "Searching for Link usage patterns with variant and buttonVariant:" rg -A 2 '<Link\s+(?:.*variant|.*buttonVariant)'Length of output: 6515
Script:
#!/bin/bash # Search for Link component definition with different patterns echo "Searching for Link component type definition:" rg -A 10 -i "type.*link.*props|interface.*link.*props|export.*link.*component" # Search for the actual file containing Link component echo "Searching for files containing Link component:" fd -e tsx -e ts -e js Link # Check MonthPicker style prop definition echo "Checking MonthPicker props:" ast-grep --pattern 'type $_ = { $$$ style?: $_ $$$ }'Length of output: 67006
|
test again |
youngcw
changed the title
|
moved back to WIP like it was before |
|
Can you make this not take up extra vertical space? Maybe put the button in line with the picker somewhere instead of over it? |
|
I agree with @youngcw about making the now button in line with the picker. Another suggestion is to put the now button under the picker so that the user knows where to look, the Now is kinda sitting with a lot of whitespace around it to me. And thank you for getting this started! |
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
UnderKoen
force-pushed
the
feat/now-button
branch
3 times, most recently
from
3bbd753 to
023fb54
Compare
UnderKoen
changed the title
UnderKoen
force-pushed
the
feat/now-button
branch
from
023fb54 to
4bef127
Compare
|
/update-vrt |
|
/update-vrt |
2 similar comments
|
/update-vrt |
|
/update-vrt |
|
Would it work to use the calendar icon we have, or would that be confusing? In the dark modes something about my monitor/eyes makes the now button look like its glowing almost like the color changing buttons you see on discord. |
|
It definitely looks cleaner! But I would expect a date picker widget when I press it with this icon. What do you think?
|
|
Isn't it common to have a calendar icon as a "today" button? I feel like that gets used plenty |
|
/update-vrt |
|
Instead of "Now" could the hover text say "Today"? And I think it would look nicer if the button stayed with the month picker instead of being pinned to the far left header. |
|
It looks great! Thanks for all the good work. One bug I found is that the calendar button when multiple months are displayed is too far to the left; it's not flush with the left side of the month. |
|
Could a reviewer (maybe @joel-jeremy) help move this PR along? |
|
/update-vrt |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/budget/BudgetPageHeader.tsx (1)
18-18: Consider making the offset value more maintainableThe magic number
4in the offset calculation could be better maintained as a named constant, especially since it's used for both left and right margins.+const SINGLE_MONTH_OFFSET = 4; -const offsetMultipleMonths = numMonths === 1 ? 4 : 0; +const offsetMultipleMonths = numMonths === 1 ? SINGLE_MONTH_OFFSET : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/budget/BudgetPageHeader.tsx(1 hunks)packages/desktop-client/src/components/budget/MonthPicker.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/budget/MonthPicker.tsx
🔇 Additional comments (1)
packages/desktop-client/src/components/budget/BudgetPageHeader.tsx (1)
20-27: Verify the margin adjustments across different viewport sizes
The margin calculations with offsetMultipleMonths appear to address the alignment issues mentioned in the PR comments. However, we should ensure these adjustments work consistently across different viewport sizes and when the calendar button is present.
Consider using CSS Grid or Flexbox for more robust alignment control instead of fixed margin calculations. This could provide better maintainability and responsiveness.
✅ Verification successful
Based on the code analysis, I can now generate the final response:
The margin adjustments are properly handled for different viewport states
The margin calculations with offsetMultipleMonths are correctly implemented to handle both single-month and multi-month views:
- For single month view (
numMonths === 1), it applies a 4px offset to adjust margins - For multiple months view, no offset is needed (
offsetMultipleMonths = 0) - The
getScrollbarWidth()utility function is properly used to account for the scrollbar presence
The implementation ensures consistent alignment regardless of the number of months displayed, and the scrollbar width is properly compensated for. No viewport-related issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for responsive styling or media queries that might affect these margins
rg -A 5 "marginLeft|marginRight" --type tsx
Length of output: 73
Script:
#!/bin/bash
# Search for responsive styling or media queries that might affect these margins, including both tsx and ts files
rg -A 5 "marginLeft|marginRight" --type ts
# Also check for any viewport-related styling or calculations
rg -A 5 "viewport|window\.(inner|outer)" --type ts
# Look for any related component styling
fd "BudgetPageHeader" . --exec cat {}
Length of output: 110483
Script:
#!/bin/bash
# Look for the getScrollbarWidth function and its usage
rg -A 5 "getScrollbarWidth" --type ts
# Look for any responsive styling or media queries in the BudgetPageHeader component and related files
fd "BudgetPageHeader|budget/.*styles" . --type ts --type tsx --exec cat {}
# Check for any viewport-related calculations in budget components
rg -A 5 "offsetMultipleMonths" --type ts
Length of output: 4387
# Conflicts: # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-1-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-2-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-3-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-1-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-2-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-3-chromium-linux.png
# Conflicts: # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-1-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-2-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-renders-the-summary-information-available-funds-overspent-budgeted-and-for-next-month-3-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-1-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-2-chromium-linux.png # packages/desktop-client/e2e/budget.test.js-snapshots/Budget-transfer-funds-to-another-category-3-chromium-linux.png
|
/update-vrt |
No description provided.