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

Web port of the react native edit transaction view #1340

Merged
merged 42 commits into from
Aug 8, 2023

Conversation

Cldfire
Copy link
Contributor

@Cldfire Cldfire commented Jul 15, 2023

Relates to #604.

Original React Native experience:

RPReplay_Final1689392915.MP4

Experience ported to web in iOS PWA:

RPReplay_Final1689392569.MP4

Everything except split transaction editing is here and functional. I didn't implement split transactions because:

  • I don't use them personally
  • It's significantly more work for significantly less gain
  • I don't think the way it was implemented in React Native was particularly ergonomic, and it would likely benefit from a redesign before being implemented again for web
  • It's a logical way to keep this MR smaller than it otherwise would be

Other future improvements possible:

  • Customize the payee / account / category autocomplete modals more for mobile, they could benefit from more touch-friendly layouts (particularly the size of the item rows, to make them easier to tap)
  • Add a toggle switch component instead of the checkbox for "cleared"
  • Fix the mobile account view so it works when you navigate to it directly instead of first loading the accounts view and then tapping into the transaction list. Right now if you go there without having loaded the accounts view first it breaks. (I already fixed this for the edit transaction view.)
  • Style polish
  • Implement split transaction editing ([Feature] Split transactions in mobile app #1352)
  • Move the + symbol somewhere more centrally located so a transaction could be entered with one hand (left or right). (from @shall0pass)
  • Translate the class components into functional ones
  • Combine the serializeTransaction and deserializeTransaction functions added here for mobile with the desktop counterparts in a way that works for both
  • Add some automated testing

@netlify
Copy link

netlify bot commented Jul 15, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 5cfa762
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/64d16ab5b42fad000882050a
😎 Deploy Preview https://deploy-preview-1340.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 Jul 15, 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
16 → 14 2.1 MB → 2.12 MB (+18.39 kB) +0.86%
Changeset
File Δ Size
src/components/mobile/MobileAmountInput.js 🆕 +5.13 kB 0 B → 5.13 kB
../../node_modules/react-redux/es/connect/selectorFactory.js 🆕 +3.83 kB 0 B → 3.83 kB
src/components/mobile/MobileForms.js 🆕 +1.97 kB 0 B → 1.97 kB
src/icons/v2/PencilWriteAlternate.js 🆕 +914 B 0 B → 914 B
../../node_modules/react-redux/es/utils/shallowEqual.js 🆕 +649 B 0 B → 649 B
src/icons/v1/Trash.js 🆕 +383 B 0 B → 383 B
src/components/transactions/MobileTransaction.js 📈 +15.01 kB (+166.40%) 9.02 kB → 24.04 kB
src/components/autocomplete/AccountAutocomplete.js 📈 +1.77 kB (+92.65%) 1.91 kB → 3.68 kB
src/components/autocomplete/CategorySelect.tsx 📈 +1.87 kB (+70.84%) 2.64 kB → 4.51 kB
src/components/autocomplete/PayeeAutocomplete.js 📈 +1.94 kB (+29.31%) 6.63 kB → 8.57 kB
src/components/modals/EditField.js 📈 +488 B (+14.82%) 3.22 kB → 3.69 kB
src/components/autocomplete/Autocomplete.tsx 📈 +1.42 kB (+11.12%) 12.78 kB → 14.2 kB
src/components/FinancesApp.tsx 📈 +660 B (+8.47%) 7.61 kB → 8.25 kB
../loot-core/src/shared/transactions.ts 📉 -7 B (-0.13%) 5.26 kB → 5.25 kB
src/components/accounts/MobileAccountDetails.js 📉 -27 B (-0.82%) 3.22 kB → 3.2 kB
src/components/accounts/MobileAccount.js 📉 -261 B (-4.29%) 5.94 kB → 5.68 kB
webpack/runtime/get javascript chunk filename 📉 -34 B (-7.16%) 475 B → 441 B
View detailed bundle breakdown

Added

No assets were added

Removed

Asset File Size % Changed
static/js/281.chunk.js 28.55 kB → 0 B (-28.55 kB) -100%
static/js/876.chunk.js 26.2 kB → 0 B (-26.2 kB) -100%

Bigger

Asset File Size % Changed
static/js/main.js 854.72 kB → 938.68 kB (+83.96 kB) +9.82%

Smaller

Asset File Size % Changed
static/js/narrow-components.chunk.js 32.2 kB → 24.62 kB (-7.58 kB) -23.55%
static/js/wide-components.chunk.js 159.63 kB → 156.68 kB (-2.95 kB) -1.85%
asset-manifest.json 2.07 kB → 1.78 kB (-296 B) -13.97%

Unchanged

Asset File Size % Changed
static/js/171.chunk.js 396.24 kB 0%
static/media/Inter-italic.var.woff2 239.29 kB 0%
static/media/Inter-roman.var.woff2 221.86 kB 0%
static/js/383.chunk.js 117.35 kB 0%
static/js/reports.chunk.js 40.96 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 5.82 kB 0%
index.html 1.66 kB 0%
static/media/browser-server.js 963 B 0%

@github-actions
Copy link
Contributor

github-actions bot commented Jul 15, 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.97 MB 0%
Changeset
File Δ Size
packages/loot-core/src/shared/transactions.ts 📉 -7 B (-0.10%) 6.85 kB → 6.84 kB
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 1012.12 kB 0%
xfo.xfo.kcab.worker.js 1004.04 kB 0%

@Cldfire
Copy link
Contributor Author

Cldfire commented Jul 15, 2023

I'd appreciate any tips on fixing up the tests; I tried running yarn test locally, but it hangs after loot-core:

Screenshot 2023-07-15 at 12 34 06 AM

@j-f1
Copy link
Contributor

j-f1 commented Jul 15, 2023

This is awesome! I will take a look at the tests and do some more code review later today.

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.

I definitely need to spend a bunch of time reading through the larger chunks of code, but here are some initial things that jumped out at me.

@j-f1 j-f1 requested a review from trevdor July 15, 2023 16:50
@trevdor
Copy link
Contributor

trevdor commented Jul 29, 2023

Here's how the layout squishes two different ways depending on which keyboard is open (editing date or amount):
(iPhone 13 mini)

edit

@Cldfire
Copy link
Contributor Author

Cldfire commented Jul 29, 2023

Interesting! It looks like you're not using Safari which must be cause of the difference. I was aware of the layout issues but I was unable to replicate that situation on my iPhone.

I'll fix the layout this weekend so the fields can't squish together like that.

@trevdor
Copy link
Contributor

trevdor commented Jul 29, 2023

Not getting the cryptic error I got the first time. 🤷🏻‍♂️ Never mind!
And you're right, in Safari, both regular and full-screen "Add to Home" mode I can't repro.
Thanks for looking at it -- if you come up with a fix, great, but I wouldn't consider this a blocker for such an important feature when it works in the mode folks are most likely to use it on iOS.

@Cldfire
Copy link
Contributor Author

Cldfire commented Jul 31, 2023

f430014 should fix the layout issues up in Firefox.

before after
IMG_0686 IMG_0685

@Cldfire Cldfire requested a review from trevdor July 31, 2023 04:32
@Cldfire
Copy link
Contributor Author

Cldfire commented Aug 3, 2023

I just pushed e6fc12a which makes a couple small tweaks to use a type="date" input for the date field, resulting in a much-improved native date picker experience when editing transaction dates:

RPReplay_Final1691091738.mov

@trevdor
Copy link
Contributor

trevdor commented Aug 4, 2023

Hey @Cldfire! This looks awesome. One color tweak in the nav bar (excuse my un-resized huge images):

This branch:
Screenshot 2023-08-04 at 1 39 26 PM

Before:
Screenshot 2023-08-04 at 1 51 21 PM

After that, @MatissJanis this looks good for whenever we can roll it into a release!
(Pardon my slowness...been away at a family reunion.)

@MatissJanis
Copy link
Member

The footer is getting addressed here: #1456

It's a regression. :)

So feel free to ignore that piece of feedback in this PR. We just need the e2e tests to pass

Copy link
Contributor

@trevdor trevdor left a comment

Choose a reason for hiding this comment

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

Thanks Matiss for the regression pointer.

Looks good!

@MatissJanis MatissJanis added this to the v23.9.0 milestone Aug 5, 2023
@MatissJanis
Copy link
Member

@Cldfire would you mind taking a look at the failing e2e test and the merge conflict? Once CI passes we're happy to merge. 🎉

@Cldfire
Copy link
Contributor Author

Cldfire commented Aug 7, 2023

Waiting on #1486 to merge before proceeding here.

EDIT: actually ended up just cherry-picking the commit for now.

@Cldfire
Copy link
Contributor Author

Cldfire commented Aug 7, 2023

@Cldfire would you mind taking a look at the failing e2e test and the merge conflict? Once CI passes we're happy to merge. 🎉

CI is green and merge conflicts are gone 😄 once #1486 merges this can then be merged.

@trevdor trevdor merged commit e9137fc into actualbudget:master Aug 8, 2023
15 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed 🔍 Ready for Review labels Aug 8, 2023
@Cldfire Cldfire deleted the jarek/port-rn-edit-transaction branch August 8, 2023 03:40
@Cldfire Cldfire changed the title Initial port of react native edit transaction view Web port of the react native edit transaction view Oct 18, 2023
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
* Set `role="button"` on downshift autocomplete items
This avoids content observation behavior in WebKit on touch devices that delays the onClick event (and therefore reaction to user input).
* Disable split transaction editing for now
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] Adding transactions on responsive mobile
5 participants