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

Custom reports updates #2386

Merged
merged 20 commits into from
Mar 1, 2024
Merged

Custom reports updates #2386

merged 20 commits into from
Mar 1, 2024

Conversation

carkom
Copy link
Contributor

@carkom carkom commented Feb 22, 2024

Some slight UI and visual changes to custom reports

Copy link

netlify bot commented Feb 22, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 927c240
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/65e1b11e29f5ca0008caec53
😎 Deploy Preview https://deploy-preview-2386.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 Feb 22, 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 4.47 MB → 4.47 MB (+1.13 kB) +0.02%
Changeset
File Δ Size
src/components/reports/reports/CustomReportListCards.tsx 📈 +1.63 kB (+28.35%) 5.75 kB → 7.38 kB
src/components/reports/ReportOptions.ts 📈 +632 B (+16.06%) 3.84 kB → 4.46 kB
src/components/reports/SaveReportName.tsx 📈 +116 B (+5.34%) 2.12 kB → 2.24 kB
src/components/reports/Header.jsx 📈 +239 B (+4.51%) 5.18 kB → 5.41 kB
src/components/reports/reports/CustomReport.jsx 📈 +462 B (+3.23%) 13.97 kB → 14.42 kB
src/components/reports/SaveReport.tsx 📈 +86 B (+1.84%) 4.58 kB → 4.66 kB
src/components/reports/ReportSidebar.jsx 📈 +36 B (+0.22%) 16.23 kB → 16.26 kB
src/components/reports/spreadsheets/net-worth-spreadsheet.ts 📈 +4 B (+0.11%) 3.43 kB → 3.43 kB
src/components/reports/spreadsheets/custom-spreadsheet.ts 📉 -2 B (-0.05%) 3.62 kB → 3.62 kB
src/components/reports/spreadsheets/grouped-spreadsheet.ts 📉 -2 B (-0.06%) 3.11 kB → 3.11 kB
src/components/reports/spreadsheets/recalculate.ts 📉 -2 B (-0.13%) 1.5 kB → 1.5 kB
src/components/reports/Overview.jsx 📉 -166 B (-8.35%) 1.94 kB → 1.78 kB
src/components/reports/reports/CustomReportCard.jsx 🔥 -1.87 kB (-100%) 1.87 kB → 0 B
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/ReportRouter.js 1.21 MB → 1.21 MB (+1.13 kB) +0.09%

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/ButtonLink.js 379 B 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/BalanceTooltip.js 6.06 kB 0%
static/js/AppliedFilters.js 20.35 kB 0%
static/js/narrow.js 79.85 kB 0%
static/js/wide.js 267.47 kB 0%
static/js/index.js 2.74 MB 0%

Copy link
Contributor

github-actions bot commented Feb 22, 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.2 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.2 MB 0%

@carkom carkom mentioned this pull request Feb 23, 2024
39 tasks
@Teprifer
Copy link

Confirming categories sort per budget order when importing my budget.

@Teprifer
Copy link

Teprifer commented Feb 24, 2024

Ok this is interesting, thought I'd dig a little deeper as I originally just checked my regular deposits filter in the default and came across some odd behaviour now:

  1. Click create new custom report
  2. Click Type and Net is disabled
  3. Change Split By to Account
  4. Change Split By back to Category
  5. Click Type and Net is now available for selection
  6. Click Net

image
By way of explanation since I cut off categories, the far right is "Income" and the one in the middle is a category with Deposit and Payments categorised to it, net positive

For comparison, this is the same graph, just changed Type back to Payment

image

@youngcw
Copy link
Member

youngcw commented Feb 26, 2024

This is nice so far. I think it would be better long term to try to not use so much screen space for the create new report button. I would be a fan of a floating button, maybe at the bottom corner.

@carkom carkom added this to the v24.3.0 milestone Feb 27, 2024
@MatissJanis
Copy link
Member

This is nice so far. I think it would be better long term to try to not use so much screen space for the create new report button. I would be a fan of a floating button, maybe at the bottom corner.

+1

IMO a better location for the button would be at the top of the page (screenshot). Plus we should make it smaller and primary color.

And then we can get rid of the separator line between the graphs.

Screenshot 2024-02-28 at 08 01 57

@youngcw
Copy link
Member

youngcw commented Feb 28, 2024

sankey report now takes up a whole row in the dash

@carkom
Copy link
Contributor Author

carkom commented Feb 28, 2024

sankey report now takes up a whole row in the dash

Good catch. All set.

@youngcw
Copy link
Member

youngcw commented Feb 28, 2024

After this is there anything else you think NEEDS to be included? or is this about ready to remove the feature flag?

@carkom
Copy link
Contributor Author

carkom commented Feb 28, 2024

After this is there anything else you think NEEDS to be included? or is this about ready to remove the feature flag?

There's two things I'd like to finish before removing feature flag.

  1. incorporating "yearly" interval choice (I have a branch about 75% done)
  2. create "disabledList" to make the functionality more consistent and the code easier to read (This branch is ready to submit)

My assumption is that the next release should have these PRs merged and a further PR to remove feature flag. Cheers!

@carkom carkom requested a review from MatissJanis February 29, 2024 18:15
@MatissJanis
Copy link
Member

You mean the networth and cash flow cards?

Yes

@MatissJanis
Copy link
Member

Screenshot 2024-02-29 at 19 41 12

Found one more bug here: the spacing between the header and the other content is too small.

@carkom
Copy link
Contributor Author

carkom commented Feb 29, 2024

Both fixed. Cheers!

@trafico-bot trafico-bot bot removed the ⚠️ Changes requested Pull Request needs changes before it can be reviewed again label Feb 29, 2024
MatissJanis
MatissJanis previously approved these changes Feb 29, 2024
Copy link
Member

@MatissJanis MatissJanis left a comment

Choose a reason for hiding this comment

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

Just a small nitpick on my end. Overall: LGTM!

(but please wait for another approval before merging as quite a few others were looking into this PR too)

marginTop: 10,
}}
>
<ButtonLink to="/reports/custom" type="primary">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is currently navigating to the last loaded custom report rather than to a new custom report if used after looking at a saved report. I think we want it to open a default report each time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not experiencing the same behavior. If I open a saved report then click "reports" in the main menu sidebar then click this button, I get a new default report. Is your experience different? How can I duplicate what you're seeing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it starts happening if you load a custom report. Then go back to the cards and rename it from there. Then after the rename, click the button. (and it loads with the old name, prior to the rename)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got it sorted, can you test again?

@trafico-bot trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed ✅ Approved labels Feb 29, 2024
@trafico-bot trafico-bot bot added 🔍 Ready for Review and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Mar 1, 2024
@carkom
Copy link
Contributor Author

carkom commented Mar 1, 2024

I've also fixed a bug where the "rename" menu option was transfering the most recently used name and error messages to new card menu clicks.

Ready for review!

@Teprifer
Copy link

Teprifer commented Mar 1, 2024

Appreciate adding the saved report name to the header, 2 verrry minor suggestions 'for another day' :)

  1. Remove the 's' in reports to make it singular as it's a single report being viewed.
  2. Add the (modified) tag when doing so with right save/load menu.

e.g.

image

@carkom
Copy link
Contributor Author

carkom commented Mar 1, 2024

Appreciate adding the saved report name to the header, 2 verrry minor suggestions 'for another day' :)

1. Remove the 's' in reports to make it singular as it's a single report being viewed.

2. Add the (modified) tag when doing so with right save/load menu.

I removed the "s" as that's an easy one. I've added the second suggestion to my feedback doc for a future PR.

@carkom carkom requested a review from twk3 March 1, 2024 13:34
Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

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

The changes look good. Thanks @carkom

@carkom carkom merged commit 40296dc into actualbudget:master Mar 1, 2024
19 checks passed
@trafico-bot trafico-bot bot added the ✨ Merged Pull Request has been merged successfully label Mar 1, 2024
@carkom carkom deleted the reportUpdates branch March 1, 2024 18:41
@trafico-bot trafico-bot bot removed the ✅ Approved label Mar 1, 2024
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
* Button changes and time filters

* rename on dashboard

* notes

* fix time filters

* Sort Categories

* Page title

* category sort order

* move button

* featureflag

* Highlight report name

* sankey fix

* VRT

* remove doubled element

* update

* fixes

* fixes

* create button fix and rename card fix

* Title change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants