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

Add onClick handlers to the schedule and transaction icons in the transaction list (#547) #1228

Merged

Conversation

joel-jeremy
Copy link
Contributor

@joel-jeremy joel-jeremy commented Jun 29, 2023

For #547

In addition to the arrow icon, I have also updated the recurring/calendar icon to open the edit schedule page.

@netlify
Copy link

netlify bot commented Jun 29, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 25303d8
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/649ddc250d82850009e5ad99
😎 Deploy Preview https://deploy-preview-1228--actualbudget.netlify.app/
📱 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.

@netlify
Copy link

netlify bot commented Jun 29, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 8255e1a
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/649f3c37defc5600086f3386
😎 Deploy Preview https://deploy-preview-1228--actualbudget.netlify.app/
📱 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 29, 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.37 MB -> 2.38 MB (+1.03 KB) +0.04%
Changeset
File Δ Size
src/components/accounts/TransactionList.js 📈 +366 B (+7.77%) 4.6 KB -> 4.96 KB
src/components/accounts/TransactionsTable.js 📈 +1.58 KB (+4.58%) 34.6 KB -> 36.18 KB
src/components/accounts/Account.js 📈 +40 B (+0.09%) 41.51 KB -> 41.55 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.07 MB -> 1.07 MB (+1.03 KB) +0.09%

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 19.58 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

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

@joel-jeremy joel-jeremy force-pushed the transaction-schedule-transfer-icon-onclick branch from 805a97b to e0b62c5 Compare June 29, 2023 23:35
@joel-jeremy joel-jeremy force-pushed the transaction-schedule-transfer-icon-onclick branch 2 times, most recently from d4aa225 to 816a871 Compare June 30, 2023 15:23
@joel-jeremy
Copy link
Contributor Author

joel-jeremy commented Jun 30, 2023

Trying to figure out why useCachedSchedules() is returning null when run in test (TransactionsTable.test.js).

@j-f1
Copy link
Contributor

j-f1 commented Jun 30, 2023

Trying to figure out why useCachedSchedules() is returning null when run in test (TransactionsTable.test.js).

useCachedSchedules() is defined as useContext(SchedulesContext), so it’s likely the thing isn’t being rendered inside a <SchedulesProvider> — I think you’ll need to provide that in the test, or handle the schedule array being null, or maybe mock useCachedSchedules in tests.

@joel-jeremy joel-jeremy force-pushed the transaction-schedule-transfer-icon-onclick branch from 4b68703 to b81b6fd Compare June 30, 2023 19:34
let ScheduledIcon = () => (
<Button bare style={style} onClick={onScheduleIconClick}>
{recurring ? (
<ArrowsSynchronize style={colorStyle} />
Copy link
Contributor

Choose a reason for hiding this comment

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

The icons will need to have a width property set on their style or else they will size themselves to be too big and the buttons will appear blank.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I just pass the same style used in the button to the icons?

Copy link
Contributor

Choose a reason for hiding this comment

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

You’d need to pass in a smaller width/height since the button has padding defined on it that reduces the available space for the icon. With the size you’ve got now, the button content area is 13×13px.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also fwiw the old arrow buttons were 10px wide

Copy link
Contributor

Choose a reason for hiding this comment

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

…and there wasn’t that 5px padding between the icon and the text (for transfers, schedules did have it. ugh not sure which one to pick here, the schedule icon does look bad™ without the spacing, transfer arrow looks fine but when you hover it is a bit odd)

Copy link
Contributor Author

@joel-jeremy joel-jeremy Jun 30, 2023

Choose a reason for hiding this comment

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

Adjusted icon sizes to match the existing ones:
image

Hovered:
image

Existing:
image

I think having the same padding for the schedule and transfer icons resulting in the texts lining up looks better. But let me know if I should adjust it to retain the existing padding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having the text line up definitely looks better — good point.

@MatissJanis
Copy link
Member

(I'll leave this to Jed to approve)

Really like this change! Especially jumping to the transfer account. That's super convenient.

@youngcw
Copy link
Contributor

youngcw commented Jun 30, 2023

Schedule link is working well for me.

The transfer link makes the account page force scroll to the bottom of the ledger. If I navigate back to the original account it does the same thing. This state sticks around and the only way to get it to go away is to close the tab and re-open the page. This is happening on both chrome and firefox for me

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.

Great improvement!

@j-f1 j-f1 merged commit d325e6b into actualbudget:master Jun 30, 2023
15 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved labels Jun 30, 2023
@joel-jeremy
Copy link
Contributor Author

joel-jeremy commented Jun 30, 2023

I am encountering the issue @youngcw mentioned. I am thinking maybe it's due to the exposed flag in the <Cell exposed={true}>?

@j-f1
Copy link
Contributor

j-f1 commented Jun 30, 2023

Oop, that comment must have come in between opening the page and approving. I didn’t see anything like that locally, but i only tested with the transfer being the first transaction. Maybe it has something to do with scrolling?

@joel-jeremy
Copy link
Contributor Author

I don't encounter it locally as well but it happens in the netlify demo deployment.

TomAFrench added a commit to TomAFrench/actual that referenced this pull request Jul 4, 2023
* master: (34 commits)
  chore: add types to `crdt` package (actualbudget#1076)
  Fix layout of the management app with the demo bar in place (actualbudget#1267)
  ♻️ (select) removing 2x usages of the Select component (actualbudget#1259)
  Fix transaction list scrolling behavior (actualbudget#1260)
  🐛  fix link-schedule option in transaction table (actualbudget#1250)
  cleared/uncleared background update (actualbudget#1265)
  Fix calculation of how many table rows to render (actualbudget#1262)
  ♻️  moving more components out of common.tsx (actualbudget#1257)
  ♻️  moving some components from common.tsx to separate files (actualbudget#1248)
  🐛  fix toggling of balances in all-accounts view (actualbudget#1252)
  🐛 (mobile) reduce the size of account cards (actualbudget#1247)
  Fix electron export issue (actualbudget#1242)
  Saved Filters Page (actualbudget#1122)
  🔧  cancel previous CI runs if a new push is made (actualbudget#1251)
  .gitattributes Check line endings for tsx files (actualbudget#1246)
  Fix importing transfers from YNAB4/5 (actualbudget#1224)
  Auto-close the local/nordigen picker modal after creating an account (actualbudget#1219)
  Run “Handle completed feature requests” in pull_request_target (actualbudget#1243)
  Add electron options to bug-report.yml (actualbudget#1239)
  Add onClick handlers to the schedule and transaction icons in the transaction list (actualbudget#1228)
  ...
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
…nsaction list (actualbudget#1228)

For actualbudget#547 

In addition to the arrow icon, I have also updated the
recurring/calendar icon to open the edit schedule page.
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.

[Feature] Clicking on arrow of transfer should take you to corresponding transaction in other account
4 participants