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

Sort Transactions on Accounts Page #1232

Merged
merged 59 commits into from
Jul 18, 2023
Merged

Conversation

carkom
Copy link
Contributor

@carkom carkom commented Jun 30, 2023

This is a pretty simple and straight forward PR. I've added sorting to the transactions table.

@netlify
Copy link

netlify bot commented Jun 30, 2023

Deploy Preview for actualbudget ready!

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

@github-actions
Copy link
Contributor

github-actions bot commented Jun 30, 2023

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.33 MB -> 2.34 MB (+4.57 KB) +0.19%
Changeset
File Δ Size
src/icons/v1/ArrowDown.js 🆕 +405 B 0 B -> 405 B
src/icons/v1/ArrowUp.js 🆕 +404 B 0 B -> 404 B
src/components/accounts/Account.js 📈 +2.88 KB (+11.01%) 26.18 KB -> 29.06 KB
src/components/transactions/TransactionsTable.js 📈 +2.46 KB (+6.82%) 36.13 KB -> 38.59 KB
src/components/accounts/Header.js 📈 +183 B (+2.05%) 8.7 KB -> 8.88 KB
src/components/transactions/TransactionList.js 📈 +75 B (+1.48%) 4.96 KB -> 5.03 KB
src/components/table.tsx 📈 +262 B (+1.18%) 21.65 KB -> 21.91 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.03 MB -> 1.03 MB (+4.57 KB) +0.43%

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 Jun 30, 2023

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

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.

Good work so far. I’m noticing an issue with the demo budget when I sort by date (reversed). My suggestion below might help but you might also need to change the sorter to produce an unambiguous result by falling back to sorting by UUID as a fallback if the two transactions match?

Screen.Recording.2023-06-30.at.11.18.10.mov

@carkom
Copy link
Contributor Author

carkom commented Jul 1, 2023

Something from a recent merge is causing my page to freak out...bouncing between category and amount as if they are being clicked on but they aren't. @j-f1 can you help me find the bug?

@carkom
Copy link
Contributor Author

carkom commented Jul 3, 2023

I’m noticing an issue with the demo budget when I sort by date (reversed).

I think the usememo addition fixed this. Can you double check it?

@carkom
Copy link
Contributor Author

carkom commented Jul 16, 2023

@Kidglove57, I think I found a good solution. I copied the logic from filters and searches. Sorting removes the balance column but remembers your last show/hide choice of the balance column option. Sort date desc acts as a "delete all sorts" button and allows the balance to show again. However, filter and search also need to be empty for this to work, I still think we need to keep them as separate elements that are controlled independently. Let me know if this is closer to what you meant.

@Kidglove57
Copy link

Kidglove57 commented Jul 17, 2023

@carkom Thank you very much for working out a solution - much appreciated.

  1. My concern was to provide a cohesive experience for those coming from the current fixed view (Date:Desc) experience. Your update achieves this perfectly since, as you say, clicking Date:Desc restores the expected view after sorting by other columns. Or, of course, clicking "Remove all Sorting" would achieve the same thing once users have discovered that option.
  2. I have run through each column sort using my live budget file which includes 3 years of data and over 20 accounts. All sorts worked for me as expected. The only column which looked a bit odd was Payees. The "real" payees are sorted perfectly but the transfers between accounts in the payee column did not list in any discernable order. I can live with that. Here is an example from the Payee column with Payee:Ascending selected:
Payee Ascending@2x
  1. Is there a reason why the "Remove All Sorting" option appears in the individual accounts view but not in All Accounts or For Budget?
  2. The interaction between Filters and Sorting is I agree problematic since they operate independently. For example:
  • Creating a filter and then sorting by, say, payee works well. If I then click "Remove all Sorting" the filter goes haywire. However, if I then click Date:Desc the filter is restored. Or I can just click Date:Desc and not use the Remove all Sorting at all.

@carkom
Copy link
Contributor Author

carkom commented Jul 17, 2023

1. My concern was to provide a cohesive experience for those coming from the current fixed view (Date:Desc) experience. Your update achieves this perfectly since, as you say, clicking Date:Desc restores the expected view after sorting by other columns. Or, of course, clicking "Remove all Sorting" would achieve the same thing once users have discovered that option.

👍

2. I have run through each column sort using my live budget file which includes 3 years of data and over 20 accounts. All sorts worked for me as expected. The only column which looked a bit odd was Payees. The "real" payees are sorted perfectly but the transfers between accounts in the payee column did not list in any discernable order. I can live with that. Here is an example from the Payee column with Payee:Ascending selected:

This is due to the actual name of the payee being null in the data table. I'm not sure there's an easy way to aggregate payee name with transfer_acct name and then sort them all together. I think it best to push this to a new PR.

3. Is there a reason why the "Remove All Sorting" option appears in the individual accounts view but not in All Accounts or For Budget?

Fixed

4. The interaction between Filters and Sorting is I agree problematic since they operate independently. For example:
* Creating a filter and then sorting by, say, payee works well. If I then click "Remove all Sorting" the filter goes haywire. However, if I then click Date:Desc the filter is restored. Or I can just click Date:Desc and not use the Remove all Sorting at all.

This was a bug. I've fixed it.

I've setup filters, searches and sorting to all operate independently so that taking action on one element doesn't automatically change any of the the other 2 elements. This is out the idea to avoid assuming what a user wants to do. For instance, creating/deleting a filter will no longer clear the search box or any sorting you've done and clearing the sorting will not clear filters or search box and clearing search box will not change filters or sorting.

@Kidglove57
Copy link

Thanks so much @carkom. And for the clear explanations.

I have tested again with my complex file and it now all works beautifully for me. I'm loving this extra functionality.

I agree about the separate PR for transfer account name issue.

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.

Looks mostly good to me, just a few minor nits

@carkom
Copy link
Contributor Author

carkom commented Jul 18, 2023

If nothing to add. This is ready for merge. Cheers!

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.

LGTM but I want @MatissJanis to take a look too.

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.

Thanks! This is a great addition to Actual!

@MatissJanis MatissJanis merged commit 77ef864 into actualbudget:master Jul 18, 2023
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jul 18, 2023
@carkom carkom deleted the sortAccounts branch August 2, 2023 16:15
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
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.

4 participants