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

US-2157: Round USD conversions #890

Merged
merged 17 commits into from
Apr 12, 2024
Merged

US-2157: Round USD conversions #890

merged 17 commits into from
Apr 12, 2024

Conversation

rodrigoncalves
Copy link
Collaborator

@rodrigoncalves rodrigoncalves commented Feb 7, 2024

As suggested by @TravellerOnTheRun, I decided to not only modify the existing formatTokenValues function, but split into 2 different functions (formatTokenValue and formatFiatValue) that each use a generic formatNumber function. This function expands the current implementation and now it should cover all different cases present in the application.

Below are a few screenshots showing the implementation in action. Please let me know if I missed anything.

@rodrigoncalves rodrigoncalves self-assigned this Feb 7, 2024
@TravellerOnTheRun
Copy link
Collaborator

@rodrigoncalves
I've looked through the code and I don't like what I see:
First of all, the RBTC fee values will always show as 0.00 as it was before, because the value is toFixed(2). I already added the function which formatTokenValues which already contains the logic for showing the values in a relatively nice-looking manner. How about you edit that? We can discuss how many digits we want after 0 but let's make a single source of truth for formatting values?

@rodrigoncalves
Copy link
Collaborator Author

@rodrigoncalves I've looked through the code and I don't like what I see: First of all, the RBTC fee values will always show as 0.00 as it was before, because the value is toFixed(2). I already added the function which formatTokenValues which already contains the logic for showing the values in a relatively nice-looking manner. How about you edit that? We can discuss how many digits we want after 0 but let's make a single source of truth for formatting values?

Of course! That's why this PR is in draft, I'm still playing around with it. The formatTokenValues function seems to be a nice way to deal with token amounts due to different quantities of decimal places that we can have. However I didn't want to edit that function yet as all I need to do for a USD conversion is toFixed(2) and there you go. But I know for RBTC fee values cases (<$0.01), for example, it could be a problem.

Copy link
Collaborator

@TravellerOnTheRun TravellerOnTheRun left a comment

Choose a reason for hiding this comment

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

Some changes are still required for future proofing the app

src/screens/send/TransactionForm.tsx Outdated Show resolved Hide resolved
src/screens/send/TransactionForm.tsx Outdated Show resolved Hide resolved
src/screens/send/TransactionForm.tsx Outdated Show resolved Hide resolved
src/shared/utils/index.test.ts Show resolved Hide resolved
src/ux/requestsModal/ReviewBitcoinTransactionContainer.tsx Outdated Show resolved Hide resolved
src/screens/activity/ActivityRow.tsx Outdated Show resolved Hide resolved
src/screens/activity/ActivityRow.tsx Show resolved Hide resolved
src/components/token/index.tsx Outdated Show resolved Hide resolved
src/components/token/index.tsx Outdated Show resolved Hide resolved
src/ux/requestsModal/ReviewBitcoinTransactionContainer.tsx Outdated Show resolved Hide resolved
src/shared/utils/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jormelCoin jormelCoin left a comment

Choose a reason for hiding this comment

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

It should be fixed in this PR.

Screenshot 2024-03-21 at 11 43 00

@jormelCoin
Copy link
Contributor

I still see the NaN in Bitcoin transactions.

@jessgusclark, when you get a chance, could you, please take a look? Rodrigo says it works well for him but for me fails since I still see the NaN.

@jormelCoin jormelCoin added Tested Ready to move forward and removed Returned labels Apr 2, 2024
Copy link
Contributor

@jormelCoin jormelCoin left a comment

Choose a reason for hiding this comment

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

I carried out the tests and it worked very well both iOS and Android.

@jessgusclark jessgusclark merged commit 5e83d41 into develop Apr 12, 2024
1 check passed
@jessgusclark jessgusclark deleted the us-2157 branch April 12, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech debt Tech debt Tested Ready to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants