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

Make transaction list sortable by cleared status #1994

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

jaarasys-henria
Copy link
Contributor

@jaarasys-henria jaarasys-henria commented Nov 29, 2023

Make transactions list sortable by cleared status. Makes cleared status column behave like any other column that it makes sense to sort by. Simple implementation with no negatives to mention. Unless you count the new header text taking up a little more horizontal screen space than the empty header it replaces as a downside.

I tried testing with different screen sizes and ratios in Mozilla Firefox and behavior looked sane to me.

Let me know if there's something I can do to enhance my suggested feature and I'll get it done. I hope I could properly deduce a correct implementation from the surrounding code. React is not my forte :)

Copy link

netlify bot commented Nov 29, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit eb2dbcd
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/65bd162d6fa029000864cf11
😎 Deploy Preview https://deploy-preview-1994.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.

@jaarasys-henria jaarasys-henria marked this pull request as ready for review November 29, 2023 18:09
@youngcw
Copy link
Member

youngcw commented Nov 29, 2023

can you use just a "C" or "R" instead of "Cleared" that would not create as much empty space

@jaarasys-henria
Copy link
Contributor Author

jaarasys-henria commented Nov 29, 2023

can you use just a "C" or "R" instead of "Cleared" that would not create as much empty space

Fair point. I actually considered and tried it, but didn't really like it. It felt a bit unclear IMO. Old YNAB4 has a nice, stylized 'C' letter. Maybe I could retry what you suggested, but with some added styling/formatting to make it look more like a symbol for something instead of just looking like a stray letter up there in the column header. That might sway me to love it.

@youngcw
Copy link
Member

youngcw commented Nov 29, 2023

Something as simple as an italicized letter could make it not feel like a bug

@jaarasys-henria
Copy link
Contributor Author

Something as simple as an italicized letter could make it not feel like a bug

I'll see what I can come up with! :)

@shall0pass
Copy link
Contributor

Another thought. Could the button default to the down position? Normally when I sort by cleared status, I'm looking for the uncleared items. Currently I need to click twice to bring those to the top. I know it doesn't follow the default behavior of the other columns, but it seems like to me it would be more expected. Thoughts?

@jaarasys-henria
Copy link
Contributor Author

Another thought. Could the button default to the down position? Normally when I sort by cleared status, I'm looking for the uncleared items. Currently I need to click twice to bring those to the top. I know it doesn't follow the default behavior of the other columns, but it seems like to me it would be more expected. Thoughts?

Fair point. I fiddled with both options, but by some already forgotten fuzzy gut feeling logic, landed on this one. I have no strong feelings one way or the other, so might just as well change it.

@jaarasys-henria
Copy link
Contributor Author

Feedback acknowledged and implemented.

  • Needlessly long 'cleared' text is now just a '✓', which I feel strikes a nice balance between aesthetics and ease of implementation. I gave it some five px of wiggle room compared to the old values. I read React-stuff from the interwebz and came up with all kinds of convoluted schemes of passing styling via props, but then realized simple things are often the best, removed all of them and leave ya'll with this simple change :)
  • sort order now starts as ascending with falsey checkmarks first.

@youngcw
Copy link
Member

youngcw commented Nov 30, 2023

I like that better

@youngcw
Copy link
Member

youngcw commented Dec 6, 2023

Can you look into why the tests are failing?

@jaarasys-henria
Copy link
Contributor Author

jaarasys-henria commented Dec 11, 2023

Can you look into why the tests are failing?

Could there be some issue with PRs from forked repos and the workflow?

I just synced my PR to see how it goes this time, but same result. The Wait for PR build to succeed doesn't seem to detect a successful Netflify build of the PR, even though Netlify deployment logs don't seem to indicate any issue with the build step. It seems like it should succeed

It doesn't mark the 'wait for PR build' step as a failure though, even if it times out. Instead it lets the next step run and then we get an error with no matching workflow run found with any artifacts. For some reason it can't grab the build-stats build artifact. Netlify logs look like it should get created properly and then uploaded at the end of build.yml .

I wonder why this could be, since the build clearly succeeds and the live preview is online. Based on the deployment log lines here there should be something there for the next stage to enjoy.

This commit seems to touch stuff around those parts.

With no access to testing the Netlify API response and other related things, I'm not sure what's going on. Any clues where to look next?

@youngcw
Copy link
Member

youngcw commented Dec 11, 2023

At least half of our PRs come from forks, so I doubt its because of that.
@MatissJanis Any idea why the tests are failing?

Copy link
Contributor

github-actions bot commented Dec 11, 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
10 4.98 MB → 4.98 MB (+1.24 kB) +0.02%
Changeset
File Δ Size
src/components/accounts/Account.jsx 📈 +1.02 kB (+2.61%) 39.04 kB → 40.06 kB
src/components/transactions/TransactionsTable.jsx 📈 +228 B (+0.40%) 55.14 kB → 55.36 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/wide.js 239.9 kB → 241.14 kB (+1.24 kB) +0.52%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/resize-observer.js 18.37 kB 0%
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/ButtonLink.js 379 B 0%
static/js/BalanceTooltip.js 6.06 kB 0%
static/js/FiltersMenu.js 28.92 kB 0%
static/js/narrow.js 80.18 kB 0%
static/js/ReportRouter.js 1.84 MB 0%
static/js/index.js 2.64 MB 0%

Copy link
Contributor

github-actions bot commented Dec 11, 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
1 1.18 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.18 MB 0%

@MatissJanis
Copy link
Member

@youngcw as a maintainer you'll want to manually trigger CI jobs for folks that are first-time contributors in Github. And then also retry the bundle size comparison job.

Screenshot 2023-12-11 at 19 57 29

@youngcw
Copy link
Member

youngcw commented Dec 11, 2023

Looks like its just the VRT that needs updated. The steps to update the snapshots is here https://github.com/actualbudget/actual/blob/master/packages/desktop-client/README.md#visual-regression

@jaarasys-henria
Copy link
Contributor Author

jaarasys-henria commented Dec 11, 2023

I think I did see something about approving some workflow stage somewhere and it almost rang a bell. But just almost. Cheers for pitching in @MatissJanis! Nice to see this moving forward.

I'll look into the VRT thing later.

@Jackenmen
Copy link
Contributor

Hi, I just wanted to let you know that this seems to ignore whether the cleared status is in "locked" state and treats it as cleared:
image

Copy link
Contributor

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jan 14, 2024
@jaarasys-henria
Copy link
Contributor Author

@Jackenmen I'll look into it! Sry for the silence. Been quite a busy period.

@jaarasys-henria jaarasys-henria force-pushed the sortable_cleared branch 2 times, most recently from 9dc68df to 71abe2d Compare February 1, 2024 19:21
@jaarasys-henria
Copy link
Contributor Author

jaarasys-henria commented Feb 1, 2024

Hi, I just wanted to let you know that this seems to ignore whether the cleared status is in "locked" state and treats it as cleared.

Is this more to your liking, @Jackenmen?

Also, it looks like someone has to push the magic button for the size comparison check like @youngcw or @MatissJanis did earlier.

@Jackenmen
Copy link
Contributor

I'm not going to look at the code but the deploy preview works as I would expect it to, yeah!

@youngcw
Copy link
Member

youngcw commented Feb 1, 2024

Nice

@youngcw
Copy link
Member

youngcw commented Feb 1, 2024

I think my only concern at this point is that the column headers are not slightly misaligned from the data with the addition of the check mark.

@jaarasys-henria
Copy link
Contributor Author

jaarasys-henria commented Feb 1, 2024 via email

@jaarasys-henria
Copy link
Contributor Author

I think my only concern at this point is that the column headers are not slightly misaligned from the data with the addition of the check mark.

This is now addressed as well 👍

@youngcw
Copy link
Member

youngcw commented Feb 2, 2024

Looks good. Thanks!

@youngcw youngcw merged commit 86007e3 into actualbudget:master Feb 6, 2024
19 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Feb 6, 2024
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.

5 participants