-
Notifications
You must be signed in to change notification settings - Fork 82
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
Issue 89 #133
Issue 89 #133
Conversation
PietroChitto
commented
Jan 5, 2024
- Reading real data in account page graph
- Reading real data in graph page graph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow that was fast! I checked your changes and it looks to me like that there are only a few things to modify. I commented directly for the specific things and then there is one that affects both graphs and it's how they are displayed, at least on my android device they start directly from the edge of the screen which makes them hard to interact and read.
See the screenshoots:
About the Graphs page: since we are in January it's not showing anything and my guess is because there is only one point in the graph, what we could do is make the graph go from December to December or find another graphical way to display what's the liquidity for this month
line1Data: accountTransactions, | ||
colorLine1Data: const Color(0xffffffff), | ||
line2Data: const <FlSpot>[], | ||
colorLine2Data: const Color(0xffffffff), | ||
colorBackground: blue5, | ||
maxDays: 30.0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably vary depending on the current month, for example now that we are in January it should be 31 and next month should change to 29 (if there is an easy way to keep track of leap years great otherwise we can just always consider 29)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
line2Data: <FlSpot>[], | ||
colorLine2Data: Color(0xffffffff), | ||
LineChartWidget( | ||
line1Data: accountTransactions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me like the graph is not showing the data that I was expecting, when I load the demo data and check the Revolut account for example I see a negative value. What I think the graph should show is how the account balance changed in the past month day-by-day (let me know if I wasn't clear and I'll write an example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This graph is like that in the dashboard filtered by account. If we want to take the balance day by day considering all the history, we must also take into account the transfer transactions from one account to another and we need to understand how to calculate the balance efficiently.
Suppose this situation:
-Inital amount of account A: 1000
-Initial amount of account B: 1000
- expense of 100 on 31th Dec on A
- transfert 50 from A to B on 1st Jan
- Income of 100 on 2nd Jan on A
The points for the two accounts for Jan should be:
A: (1, 850), (2, 950)
B: (1, 1050), (2, 1050)
Can you confirm this?
Another point is how to calculate the balance, now the only possible way is to calculate it strating from day 0 and considering all the transacions. Until there are few transactions this has a limited calculation impact (fine for a first implementation) but when there are a significant number of transactions this could have a considerable impact (to understand how it grows as transactions increase) and alternative will need to be considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was exactly what I had in my mind. About the balance, in the past we talked a little bit on how to do it so that we don't have to do many operations and we considered both having some kind of record inside the DB (ex. every month we save the balance) or use the cache but we didn't finalize our decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fine, now I implement it by recalculating it every time. If there will be records with balance in db in the future let's remember that every time an update transaction is performed, the monthly balances following the transaction must be recalculated
colorLine1Data: Color(0xffffffff), | ||
line2Data: <FlSpot>[], | ||
colorLine2Data: Color(0xffffffff), | ||
LineChartWidget( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably show some days at the bottom of the chart as a reference. See the Figma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, it is relative to the widget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems fine to me except one thing that I noticed in the account page, it seems to me that the chart there has the minimum of the List that we send as the bottom of the chart instead of zero. You can see the screenshot below in which it looks like my account should be empty but I have 38k.
The same behavior is present in all line charts in the application (Dashboard, account and graph pages). I propose to collect all these requirements and create a separate issue.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it looks good to me then. I'll merge this and open a new issue