Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Turning on thousand separator for FIAT values #1934

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

alfetopito
Copy link
Contributor

@alfetopito alfetopito commented Nov 26, 2021

Summary

Closes #1876

It's also locale aware.
Meaning, if your locale uses , as decimals separator and . as thousand separator you'll see 13.232.432,11 and so on

Left: after; Right: before
Screenshot from 2021-11-26 12-29-31

To Test

  1. On mainnet, pick a pair and insert a high enough amount
  2. The usd estimation will be split every 1k by a ,

Background

Optional: Give background information for changes you've made, that might be difficult to explain via comments

@alfetopito alfetopito self-assigned this Nov 26, 2021
@alfetopito alfetopito requested review from a team November 26, 2021 20:35
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@elena-zh
Copy link

Hey @alfetopito , great changes!
I retested numbers in different locales, and it uses different separators (spaces/commas/periods) for different regions.
The only nitpick I could find is that in Switzerland they use apostrophe as thousand separator, but we still use commas in our app for this region
nitlick

Another issue I'd like to bring up here is that now we show differently USD and token amounts:

  • RU locale:
    image
  • EN locale
    image
  • GE locale:
    image

I think, we should use the same amounts formatting across the app for both tokens and USD amounts.
Please let me know if I need to create a separate issue for this.

Thanks!

Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

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

noice, would say that Elena's comment about the non-conformity between the 2 amounts is a bit strange but not that bad. the separator looks god

@alfetopito
Copy link
Contributor Author

Hey @alfetopito , great changes! I retested numbers in different locales, and it uses different separators (spaces/commas/periods) for different regions. The only nitpick I could find is that in Switzerland they use apostrophe as thousand separator, but we still use commas in our app for this region nitlick

Hmm, that'd be up to Javascript's locale settings, besides a special case handling for Switzerland, not much to do from our side.

Another issue I'd like to bring up here is that now we show differently USD and token amounts:

* RU locale:
  ![image](https://user-images.githubusercontent.com/70885163/143855733-f258b775-abd1-407c-8192-512e452d97dd.png)

* EN locale
  ![image](https://user-images.githubusercontent.com/70885163/143855830-ee92dc1a-ee4c-4114-8994-3b0ae56224c6.png)

* GE locale:
  ![image](https://user-images.githubusercontent.com/70885163/143855888-d7448bba-c28a-48aa-b69b-cd247a01aa86.png)

I think, we should use the same amounts formatting across the app for both tokens and USD amounts. Please let me know if I need to create a separate issue for this.

Thanks!

Regarding that, the point is not to touch the user input field.
Formatting that would likely lead to issues as it already happened in the past.

If I recall correctly, on the confirmation modal they are formatted, just not in the swap input.

One idea to not impact the actual input contents is to format it using CSS, but anyway, out of the scope of this change

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Great change!

Happy u took the initiative to fix this.
Eventually we would need to fix the amounts as Elena says. But thats out of this one's scope.

Maybe we can schedule some work for protofire in next sprint with this.

@alfetopito alfetopito merged commit 8663505 into develop Nov 29, 2021
@alfetopito alfetopito deleted the 1876/thousand-separator-usd-estimation branch November 29, 2021 21:31
@elena-zh
Copy link

I have added a note to implement all nicely formatted amounts into another related task #1897 (comment)

@ramirotw ramirotw mentioned this pull request Dec 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a commas to USD estimation
4 participants