-
-
Notifications
You must be signed in to change notification settings - Fork 960
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
Added Year comparison feature. #2806
Conversation
✅ 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
|
…ctual into spendingGraphYear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. Let me know when you feel it's ready for review.
@carkom Is the bug fix mentioned in the title the one causing the monthly spending report to crash the reports tab when enabled (today)? (if not, I can log it properly - just noticed it and then remembered this PR title) |
Yes it is :) |
Ready to be reviewed :) |
Hey mate, you need to "resolve conflicts" with the master branch. This will mean removing your code for the bug fix we discussed (since it's already been merged into the master branch). Also, we usually remove the [WIP] from the title to signify that it's ready for review. The [WIP] is added automatically to all PRs so that the owner has to actively signify that they've finished. |
Thanks for the heads up, completed what you asked :) |
Can we hide the last year button if there's no data? Same as average. |
Sure I'll add that when I get home. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, really good overall. Will be quite nice to have this option on the report. Thanks for your work! Have a look at my suggestions. Cheers!
packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Spending.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/spreadsheets/spending-spreadsheet.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the warning text, it's a good addition. It has, however, added new code that needs to be reviewed. Cheers!
packages/desktop-client/src/components/reports/reports/Spending.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Spending.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/SpendingCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/SpendingCard.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Spending.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/SpendingCard.tsx
Outdated
Show resolved
Hide resolved
I think it would be good to keep the YAxis fix separated into it's own PR. Thoughts? |
…ctual into spendingGraphYear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buttons aren't working.
packages/desktop-client/src/components/reports/graphs/SpendingGraph.tsx
Outdated
Show resolved
Hide resolved
default should be default for everyone other part of the graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good!
Year feature compares your spending to the previous year if you have data. If you do not have data, it won't crash, but the graph won't show any information.
This is my first ever real coding using React ;)