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

Reduce number of decimals shown for monetary fiat prices to 2 #3926

Merged
merged 2 commits into from Feb 5, 2020
Merged

Reduce number of decimals shown for monetary fiat prices to 2 #3926

merged 2 commits into from Feb 5, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jan 30, 2020

Fixes #3449

Reduce number of decimals shown from 4 to 2 for fiat monetary values.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK

Thanks for your first PR, but you fixed the wrong precision.
Bildschirmfoto 2020-02-04 um 10 58 25
The amount should be rounded, having no decimals for fiat.

Please add to your PR also how you test your change. Thanks!

@ghost
Copy link
Author

ghost commented Feb 4, 2020

Thanks @ripcurlx for the review, I've pushed an update so decimals are removed for displayed fiat volume amounts.

I've updated the appropriate tests and ran the bisq:desktop test task. I also did sanity checks with a local build. Let me know if I missed something.

3926-0
3926-1
3926-2
3926-3

@ghost ghost requested a review from ripcurlx February 4, 2020 19:05
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

I tested it locally on Regtest and it works as expected.
I did find an existing bug while testing which was fixed in 3438535.
Before
Bildschirmfoto 2020-02-05 um 17 42 20
After
Bildschirmfoto 2020-02-05 um 17 44 06

The bug only occurred when switching between fiat and altcoin payment accounts in the create offer dialog.

@ripcurlx ripcurlx merged commit 872a704 into bisq-network:master Feb 5, 2020
@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 5, 2020

@milesap Congrats to your first merged PR!

Just a heads up that we changed recently our process on what kind of contribution is up for compensation and which is not.
To make it clear for development contributions the is:priority label is added to a PR if it is up for compensation. Please see our recent Bisq Q1 2020 Update for more details.

stejbac added a commit to stejbac/bisq that referenced this pull request Feb 8, 2020
Add a new method to DisplayUtils to restore the old rounding behaviour
of formatVolumeWithCode whenever a fractional volume is required. This
fixes a regression caused by bisq-network#3926 to remove unnecessarily displayed
decimals for fiat volumes - it appears that in every case but the avg.
dollar price on the BSQ dashboard a whole number should be shown.

Also add a relevant test.
ripcurlx pushed a commit that referenced this pull request Feb 11, 2020
Add a new method to DisplayUtils to restore the old rounding behaviour
of formatVolumeWithCode whenever a fractional volume is required. This
fixes a regression caused by #3926 to remove unnecessarily displayed
decimals for fiat volumes - it appears that in every case but the avg.
dollar price on the BSQ dashboard a whole number should be shown.

Also add a relevant test.
@ripcurlx ripcurlx added this to the v1.2.6 milestone Feb 11, 2020
@ghost ghost deleted the fiat-show-two-decimals branch February 13, 2020 17:39
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.

Fiat amount is rounded but two decimal places are displayed
2 participants