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

Page to add/edit transactions and account page #118

Merged
merged 15 commits into from
Dec 18, 2023
Merged

Conversation

GBergatto
Copy link
Contributor

@GBergatto GBergatto commented Oct 5, 2023

I've transformed the modal used to create/add transactions into a page (fix #90), called AddPage. I've also put the text for the label right after the price to fix #98. Finally, I've made some improvements to the list of transactions in the "List" tab of the "Transactions" page: the daily totals are now correct and the icons are colored based on the category of the transaction.

Screenshot_20231005_085710.jpg

In addition, I've transformed the modal that used to open when clicking on the cards in the "Your accounts" section of the homepage into a page. However, I haven't added the list of transactions to it, yet, and the total amount displayed on the page doesn't match the one on the card.

WARNING:
Clicking on the transactions in the "Last transactions" section of the page currently opens a blank modal.

I haven't edited this section because my plan is to replace it with the lib/pages/transactions_page/widgets/list_tab.dart widget, after having made it more versatile so that it can be reused into different parts of the app. This include adding a way to filter transactions by account (to use it in the account page) and to limit the number of transactions displayed (to use it in "Last transactions" on the homepage).
This is in line with #108 about reusing widgets.

If that's fine with you, I would like to merge this PR to keep momentum and avoid big merge conflicts down the line. Next, I'll work on fixing the issues mentioned.

Diplay icon background color based on category.
Show amount in the correct color based on type.
Totals of the day consider only income and expenses.
Rename to DetailsListTile.
Remove redundant InkWell.
Use named instead of positional arguments.
Break it down into separate widgets for legibility.
Display the correct switch for recurring payments based on the platform.
Rename "Notes" to "Label" and move it to the top (fixes RIP-Comm#98).
@theperu theperu requested a review from mikev-cw October 18, 2023 15:00
@lucaantonelli
Copy link
Collaborator

@GBergatto I've checked the PR, the new add transaction page looks good, but update fail since use the old modal page, we should update the onTap at row 189 showModalBottomSheet(... in custom_widgets\transactions_list.dart , just changing row 188 to that should do the work (removing the modal code block):
ref.read(transactionsProvider.notifier).transactionUpdateState().whenComplete(() => Navigator.of(context).pushNamed("/add-page"));

@GBergatto
Copy link
Contributor Author

@lucaantonelli I've just applied the changes you suggested. Now, clicking on a transaction list tile on the homepage will open the page to edit it instead of the empty modal.

@GBergatto
Copy link
Contributor Author

There's a noticeable delay when trying to switch to the homepage from another page. My guess is that having to load data for the graph, account sums, and transactions slows things down. Maybe this should be investigated in more details in the future

@lucaantonelli
Copy link
Collaborator

lucaantonelli commented Nov 9, 2023

I noticed some strange behaviour when changing amount field (in update, but something broken also when creating new transactions),
Apart from that LGTM.

error.mp4

@GBergatto
Copy link
Contributor Author

@lucaantonelli It has something to do with the decimal separator switching from comma to dot. I'll look into it

@mikev-cw
Copy link
Collaborator

mikev-cw commented Nov 10, 2023

@lucaantonelli It has something to do with the decimal separator switching from comma to dot. I'll look into it

@rcasula worked (hardly 🤣) on that months ago. See if he can possibly help, if you need.

There's a noticeable delay when trying to switch to the homepage from another page. My guess is that having to load data for the graph, account sums, and transactions slows things down. Maybe this should be investigated in more details in the future

Not detected honestly. Please explain what "noticeable" means to you.

PS: If you're gonna add other commits, edit this line to "3.13" for passing tests

@rcasula
Copy link
Contributor

rcasula commented Nov 11, 2023

I've tried it and LGTM! Great work

@GBergatto
Copy link
Contributor Author

@mikev-cw I checked and I don't think it has something to do with the decimal separator (fortunately) because it happens even when the separator doesn't change. I guess it's caused by the way we update the provider.

About the delay, it's last just a split second. If you want I can send you a screen recording of it. Consider that I've edited lib/providers/transactions_provider.dart to change the hard-coded limit of transactions from 5 to 50. So, maybe that has an impact on it.

@mikev-cw
Copy link
Collaborator

About the delay, it's last just a split second. If you want I can send you a screen recording of it. Consider that I've edited lib/providers/transactions_provider.dart to change the hard-coded limit of transactions from 5 to 50. So, maybe that has an impact on it.

I've tried with 100 transactions, and still not noticed any remarkable delay.
If you want, after fixing that strange behavior with the amount field (and resolve new conflicts, sorry for that!), send us a screen recording, and we'll try to go deeper on that!

@lucaantonelli
Copy link
Collaborator

LGTM, except known bugs it works fine.

@mikev-cw
Copy link
Collaborator

Cool!
Let's merge

@mikev-cw mikev-cw merged commit 44d85b2 into RIP-Comm:main Dec 18, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

not visible field while filling Transform accounts modal to a page
4 participants