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

#2955: ✨ Added total spending and revenue in category summar… #2982

Merged
merged 8 commits into from
Jul 18, 2023

Conversation

balaji119
Copy link
Contributor

@balaji119 balaji119 commented Jul 16, 2023

…y page

Issue: #2955

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

Category summary page does not show total spending and total income

What is the new behavior?

Category summary page shows total spending and total income

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code on Windows
  • Tested code on Android
  • Tested code on iOS
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes

I have not tested on Windows as I am facing the following error

EP0700: Registration of the app failed. [0x80073CF0] error 0x80070003: Opening file from location: AppxManifest.xml failed with error: The system cannot find the path specified.

Other information

@NPadrutt
Copy link
Contributor

NPadrutt commented Jul 16, 2023

thanks @balaji119 for the PR and welcome!

Instead of an observable, wouldn't it be easier to just make two calculated properties here, and then bind them to a template in the xaml with something like this?

Text="{Binding SelectedAccount.CurrentBalance, StringFormat={extensions:Translate CurrentBalanceTemplate}, Converter={StaticResource AmountFormatConverter}}"

The template would be easy translatable and the calculated properties could be unit tested. Those two things are possible with the current solution, but more difficult I would think. Or is there a special purpose to the Observable Object I overlook?

@balaji119
Copy link
Contributor Author

@NPadrutt, if we keep it as a simple calculated property then when the filter is updated with a different date range then the updated values will not reflect in the UI.

@NPadrutt
Copy link
Contributor

NPadrutt commented Jul 16, 2023

Instead of assigning the value directly, you could just call RaisePropeetyChanged(nameof(yourProperty)) when the data is reloaded to update the bindings and recalculate the value.

@balaji119
Copy link
Contributor Author

@NPadrutt what you mentioned is the simple and right way to do it and made the change. You can review it? And I have added unit tests too, but I don't feel that's the right way to do it. Could you help me out here?

@balaji119
Copy link
Contributor Author

@NPadrutt Can you review again now?

@NPadrutt
Copy link
Contributor

@balaji119 I left you one suggestions. Other than that it looks good to me :)

@NPadrutt NPadrutt enabled auto-merge July 18, 2023 12:18
@NPadrutt
Copy link
Contributor

thanks @balaji119
pr is queued to merge! thanks for your contribution! :)

@NPadrutt
Copy link
Contributor

fix #2955

@NPadrutt NPadrutt added this pull request to the merge queue Jul 18, 2023
Merged via the queue into MoneyFox:master with commit fb75191 Jul 18, 2023
3 checks passed
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

3 participants