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

CategoryTransactionType for CategoryTransaction model, Charts in GraphsPage and various bugfixes and improvements #166

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

napitek
Copy link

@napitek napitek commented Apr 21, 2024

Description

This PR can be divided into two main points:

  • CategoryTransactionType for categories:
    until now the income and expense type was only for transactions. It makes sense to have the type for categories as well.

    the "CategoryTransaction" model has changed, you will need to reset the DB before proceeding

  • GraphsPage refactoring with addition of charts in reference to the issue Graphs Page  #49

Notes

The BarChart is not quite as functional as I would like. i put it in and you can click on the bars to change the month and update the category data.
I wasn't too convinced about being able to change the month at the bottom versus the data updating at the top.
In the future we could integrate the month_selector.

Copy link
Collaborator

@theperu theperu left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for all the work that you have done. I found one issue, when I tap on a bar month in which I have zero euro spent (ex. I tap on May which is in the future) then the widget updates but I can't see the bars anymore. The only way to restore it is if I close and reopen the app. Let me know if you are able to reproduce this and/or you need a screen recording.
I think that there are some conflicts that need to be resolved before merging

@napitek
Copy link
Author

napitek commented Apr 29, 2024

Hi @theperu. Yes I just looked and I noticed what you say.
I'll fix it and commit it.
Although as said before, that graph needs to be reviewed and discussed for a moment.

@napitek
Copy link
Author

napitek commented Apr 29, 2024

the GraphsPage needs a month_selector, which cannot be exclusively the final chart.
Since one already exists, I would put that in for now.

Copy link
Collaborator

@theperu theperu left a comment

Choose a reason for hiding this comment

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

I finally found the time to look at your changes. I notice another two things that might need an improvement. If a category as no transaction in the current month I see NaN instead of 0, when this happens I also see the bars for the previous months to 0 even if there are transactions on it. You can easily reproduce this by having only one income category with multiple transactions in the previous months but not in the current one

EDIT: Also the month bars are all the same height in each month

@napitek
Copy link
Author

napitek commented May 19, 2024

I finally found the time to look at your changes. I notice another two things that might need an improvement. If a category as no transaction in the current month I see NaN instead of 0, when this happens I also see the bars for the previous months to 0 even if there are transactions on it. You can easily reproduce this by having only one income category with multiple transactions in the previous months but not in the current one

I didn't notice this bug. I'll take care of it in these days

EDIT: Also the month bars are all the same height in each month

The bars are all the same because the graph does not yet calculate the "average".
I don't think there is even a method in general that does that (maybe I'm wrong and haven't seen it).
It will have to be implemented.

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.

None yet

2 participants