search

RIP-Comm / sossoldi

"Sossoldi" is a wealth management / personal finance / Net Worth tracking app, made with Flutter.
MIT License
262 stars 73 forks source link

Refactor Planning page #107

Closed 0xbaggi closed 1 year ago

0xbaggi commented 1 year ago

I started implementing the planning page with static data as requested in #79. I will be ready for implementation of all feature when all data will be available from DB.

Screenshot_20230630_120352 (1)

https://github.com/RIP-Comm/sossoldi/assets/39710037/4b95c3ef-4e39-447c-89a4-65d7a0f3fc49

GBergatto commented 1 year ago

Good job! I'm not sure about the effect when pressing on the pie chart. I would make it expand less, to keep it consistent with the pie chart inside the transactions page. Click here for the code. Also, I don't think we need to show the exact percentage when pressing on a sector.

GBergatto commented 1 year ago

Also, I don't think this PR should change pubspec.lock. BTW please use more expressive commit messages :)

0xbaggi commented 1 year ago

Good job! I'm not sure about the effect when pressing on the pie chart. I would make it expand less, to keep it consistent with the pie chart inside the transactions page. Click here for the code. Also, I don't think we need to show the exact percentage when pressing on a sector.

Thank you! I will disable the effect because if I remove the percentage it becomes useless.

GBergatto commented 1 year ago

Instead of having two commits followed by one to revert their effect, you should delete the first two all together. I believe this can be done using git reset. However, before you do, maybe ask someone with more experience to make sure it won't cause any damage.

mikev-cw commented 1 year ago

Hello, thank you for this work!! I agree with Gbergatto's excellent advices. I also agreed in keeping any kind of consistency with the pie chart on the transaction page. For all edits mentioned you want to update this PR, or prefer to merge and open a new one?

I would like to further emphasize what has already been mentioned: Please remember to get in PRs only files inside assets, lib and test folders + files flutter related like pubspec.yaml, and to use better commit descriptions. :)

0xbaggi commented 1 year ago

Hello, thank you for this work!! I agree with Gbergatto's excellent advices. I also agreed in keeping any kind of consistency with the pie chart on the transaction page. For all edits mentioned you want to update this PR, or prefer to merge and open a new one?

I would like to further emphasize what has already been mentioned: Please remember to get in PRs only files inside assets, lib and test folders + files flutter related like pubspec.yaml, and to use better commit descriptions. :)

I updated this PR with all the changes completed. I think it’s ready for the merge.