Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: now button at budget page #3703

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

UnderKoen
Copy link
Member

No description provided.

@actual-github-bot actual-github-bot bot changed the title feat: now button at budget page [WIP] feat: now button at budget page Oct 21, 2024
Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit a271d54
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/67574acc983fac0008b7e322
😎 Deploy Preview https://deploy-preview-3703.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Oct 21, 2024

Bundle Stats — desktop-client

Hey 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

Files count Total bundle size % Changed
10 5.45 MB → 5.46 MB (+938 B) +0.02%
Changeset
File Δ Size
src/components/budget/MonthPicker.tsx 📈 +836 B (+18.37%) 4.44 kB → 5.26 kB
src/components/budget/BudgetPageHeader.tsx 📈 +102 B (+14.74%) 692 B → 794 B
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/wide.js 241.09 kB → 242.01 kB (+938 B) +0.38%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/AppliedFilters.js 21.32 kB 0%
static/js/workbox-window.prod.es5.js 5.69 kB 0%
static/js/useAccountPreviewTransactions.js 1.63 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/narrow.js 82.96 kB 0%
static/js/ReportRouter.js 1.52 MB 0%
static/js/index.js 3.44 MB 0%

Copy link
Contributor

github-actions bot commented Oct 21, 2024

Bundle Stats — loot-core

Hey 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

Files count Total bundle size % Changed
1 1.32 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.32 MB 0%

@youngcw youngcw changed the title [WIP] feat: now button at budget page feat: now button at budget page Oct 22, 2024
Copy link
Contributor

coderabbitai bot commented Oct 22, 2024

Walkthrough

The changes in the pull request focus on the MonthPicker component located in MonthPicker.tsx. The useTranslation hook has been imported from react-i18next, along with the SvgCalendar component from ../../icons/v2. A new "Now" button has been introduced, which, when clicked, invokes the onSelect function with the current month as its argument. This button is styled with specific attributes and is positioned absolutely within the component's layout.

The overall structure of the component remains unchanged, maintaining the existing hover and selection logic. The t function from useTranslation is used to set the button's title to "Today". The method signature for onSelect in MonthPickerProps has been updated to reflect the context of usage with the addition of the "Now" button, although the signature itself has not changed. The component continues to display months responsively, allowing users to select a month with the current month highlighted.

Possibly related PRs

  • Marked files for translation #3832: The main PR introduces the useTranslation hook from react-i18next in the MonthPicker component, which is directly related to the changes in multiple retrieved PRs that also implement the useTranslation hook for internationalization in various components.

Suggested labels

sparkles: Merged

Suggested reviewers

  • youngcw

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) and onPress (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:

  1. Standardize on onPress and remove onClick to maintain consistency with the underlying Button component
  2. Create proper event type definitions that handle both DOM and framework-specific events
  3. 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:

  1. Add aria-label for better screen reader support
  2. Consider using relative positioning to prevent potential layout issues
  3. 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

📥 Commits

Files that changed from the base of the PR and between 1895bc8 and ef485ef.

📒 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.tsx shows that it fully supports all the props used in the MonthPicker component (variant, buttonVariant, onClick), and the single usage of MonthPicker in BudgetPageHeader.tsx shows 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

packages/desktop-client/src/components/common/Link.tsx Outdated Show resolved Hide resolved
@youngcw
Copy link
Member

youngcw commented Oct 22, 2024

test again

@youngcw youngcw changed the title feat: now button at budget page [WIP] feat: now button at budget page Oct 22, 2024
@youngcw
Copy link
Member

youngcw commented Oct 22, 2024

moved back to WIP like it was before

@youngcw
Copy link
Member

youngcw commented Oct 22, 2024

Can you make this not take up extra vertical space? Maybe put the button in line with the picker somewhere instead of over it?

@Jonathan-Fang
Copy link
Contributor

Jonathan-Fang commented Oct 24, 2024

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!

Copy link
Contributor

github-actions bot commented Nov 1, 2024

👋 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.

@github-actions github-actions bot added the Stale label Nov 1, 2024
@UnderKoen UnderKoen force-pushed the feat/now-button branch 3 times, most recently from 3bbd753 to 023fb54 Compare November 3, 2024 13:01
@UnderKoen UnderKoen changed the title [WIP] feat: now button at budget page feat: now button at budget page Nov 3, 2024
@UnderKoen
Copy link
Member Author

/update-vrt

@github-actions github-actions bot removed the Stale label Nov 4, 2024
@UnderKoen
Copy link
Member Author

/update-vrt

2 similar comments
@UnderKoen
Copy link
Member Author

/update-vrt

@UnderKoen
Copy link
Member Author

/update-vrt

@youngcw
Copy link
Member

youngcw commented Nov 4, 2024

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.

@UnderKoen
Copy link
Member Author

It definitely looks cleaner! But I would expect a date picker widget when I press it with this icon. What do you think?

image

@youngcw
Copy link
Member

youngcw commented Nov 4, 2024

Isn't it common to have a calendar icon as a "today" button? I feel like that gets used plenty

@UnderKoen
Copy link
Member Author

/update-vrt

@youngcw
Copy link
Member

youngcw commented Nov 4, 2024

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.

@Jonathan-Fang
Copy link
Contributor

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.
image

@Jonathan-Fang
Copy link
Contributor

Could a reviewer (maybe @joel-jeremy) help move this PR along?

@UnderKoen
Copy link
Member Author

/update-vrt

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 maintainable

The magic number 4 in 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

📥 Commits

Reviewing files that changed from the base of the PR and between 87835c7 and a19b2de.

📒 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants