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.

Sign up for GitHub

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

Jump to bottom

Scheduled transactions for the month to show up in Account's running balance #1354

Merged
merged 13 commits into from
Jul 20, 2023
Merged

Scheduled transactions for the month to show up in Account's running balance #1354

merged 13 commits into from
Jul 20, 2023

Conversation

joel-jeremy
Copy link
Contributor

Relates to #550

@netlify
Copy link

netlify bot commented Jul 17, 2023 •
edited
Loading

✅ Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit bbddd09
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/64b711a723199f00088ce18c
😎 Deploy Preview https://deploy-preview-1354.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.

@joel-jeremy joel-jeremy force-pushed the feat-550-schedule-running-balance branch from c62c66b to d1cbbd9 Compare July 17, 2023 16:38
@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2023 •
edited
Loading

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
13 2.35 MB -> 2.35 MB (+512 B) +0.02%
Changeset
File Δ Size
src/components/accounts/Account.js 📈 +1.1 KB (+3.77%) 29.07 KB -> 30.16 KB
src/components/transactions/TransactionList.js 📈 +39 B (+0.76%) 5.03 KB -> 5.07 KB
src/components/transactions/TransactionsTable.js 📈 +254 B (+0.64%) 38.74 KB -> 38.99 KB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/main.js 1.04 MB -> 1.05 MB (+512 B) +0.05%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/457.chunk.js 387.21 KB 0%
static/media/Inter.var.woff2 317.25 KB 0%
static/media/Inter-italic.var.woff2 239.29 KB 0%
static/media/Inter-roman.var.woff2 221.86 KB 0%
static/media/bg.svg 116.73 KB 0%
static/js/reports.chunk.js 20.8 KB 0%
static/js/969.chunk.js 12.94 KB 0%
static/js/resize-observer-polyfill.chunk.js 8.12 KB 0%
static/css/main.css 6.08 KB 0%
index.html 1.68 KB 0%
asset-manifest.json 1.47 KB 0%
static/media/browser-server.js 963 B 0%

@github-actions
Copy link
Contributor

github-actions bot commented Jul 17, 2023 •
edited
Loading

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
2 1.96 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 1007.82 KB 0%
xfo.xfo.kcab.worker.js 1004.04 KB 0%

@j-f1 j-f1 linked an issue Jul 17, 2023 that may be closed by this pull request
[Feature] Scheduled transactions for the month to show up in Account's running balance #550
Closed
j-f1
j-f1 reviewed Jul 17, 2023
View reviewed changes
packages/desktop-client/src/components/accounts/Account.js
Comment on lines -982 to +1190
{allTransactions =>
allTransactions == null ? null : (
<SelectedProviderWithItems
name="transactions"
items={allTransactions}
fetchAllIds={this.fetchAllIds}
registerDispatch={dispatch => (this.dispatchSelected = dispatch)}
>
<View style={[styles.page]}>
<AccountHeader
{(allTransactions, allBalances) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if allTransactions is null?

packages/desktop-client/src/components/accounts/Account.js Outdated Show resolved Hide resolved
packages/desktop-client/src/components/accounts/Account.js Outdated Show resolved Hide resolved
@joel-jeremy joel-jeremy force-pushed the feat-550-schedule-running-balance branch from 29f1248 to 9e7a3ca Compare July 18, 2023 22:13
@joel-jeremy joel-jeremy force-pushed the feat-550-schedule-running-balance branch from 9e7a3ca to bbddd09 Compare July 18, 2023 22:26
j-f1
j-f1 reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Mostly looks fine to me except for the one comment about allTransactions being null above.

@joel-jeremy
Copy link
Contributor Author

I have added a guard in <AllTransactions> to never return a null it will return an empty array instead. If I understand correctly, if allTransactions is an empty array, transactions table will render an "empty" (No transactions) content.

j-f1
j-f1 approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying! LGTM

@j-f1 j-f1 merged commit 7340b48 into actualbudget:master Jul 20, 2023
15 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jul 20, 2023
@carkom
Copy link
Contributor

carkom commented Jul 28, 2023 •
edited
Loading

Sorry for all the edits to the comment. Not as big of a deal as I first thought. Cheers!

I can tell that #1232 (merged on July 17th) is effected not sure if there's more.

@j-f1 @MatissJanis

@carkom
Copy link
Contributor

carkom commented Jul 28, 2023 •
edited
Loading

Okay, upon further investigation, it seems like only a couple lines are missing from 1232. They are, however, lines that break the functionality of the PR. I'll open a new PR to fix them.

@carkom carkom mentioned this pull request Jul 28, 2023
Merged
@carkom
Copy link
Contributor

carkom commented Jul 28, 2023

I'm finding that this PR creates a really confusing user experience with sorting and balances.

With balance shown, sorting by any colomn does not hide the balance column but rather just blanks out all rows. Then the menu drop down doesn't show anything about hiding the balance column until you clear all sorting. Let's discuss and fix it in #1406.

@carkom
Copy link
Contributor

carkom commented Jul 28, 2023

You've changed the showBalance={!!balances} call in the header element but then not gone through and updated all the functions that were effected by that change (onSearch, onApplyFilter, onSort, etc.).

FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
@joel-jeremy
Include scheduled transactions for the month in Account's running bal…
c97c86c 
…ance (actualbudget#1354)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j-f1 j-f1 j-f1 approved these changes

Assignees
No one assigned
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Scheduled transactions for the month to show up in Account's running balance
3 participants
@joel-jeremy @carkom @j-f1