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

toString instead of toLocaleString #226

Merged

Conversation

nguvictor
Copy link
Member

@nguvictor nguvictor commented Jan 24, 2024

This is workaround to get correct decimals to display, as the toLocaleString solution causes issues with number display
For example:
image
image

The problem is how Number and toLocaleString() loses precision. Example in dev console
image

Copy link

netlify bot commented Jan 24, 2024

Deploy Preview for chipper-sunburst-578cfe ready!

Name Link
🔨 Latest commit 0239f92
🔍 Latest deploy log https://app.netlify.com/sites/chipper-sunburst-578cfe/deploys/65b130c90b75ff00085efba3
😎 Deploy Preview https://deploy-preview-226--chipper-sunburst-578cfe.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@fliebenberg
Copy link
Contributor

Havent looked at the front-end code for some time, but why don't we use the displayAmount function here. We know the decimals for the price is fixed, so just display it the same way you display prices in the actual orderbook buys and sells.

Copy link
Member

@EvgeniiaVak EvgeniiaVak left a comment

Choose a reason for hiding this comment

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

window.innerWidth
1295

the clash still happens, I guess we need to split it in two rows when the containing div is narrow
Screenshot 2024-01-24 at 13 26 09

@Radstakes
Copy link
Contributor

I think having the spread on a row below would be fine, or we could remove the spread value and just show the percentage?

@EvgeniiaVak
Copy link
Member

EvgeniiaVak commented Jan 24, 2024

I agree with using utils.displayNumber here, with something large for noDigits (so it wouldn't shorten the large prices with Ks and Ms, maybe 18?). I don't think we need to set upfixedDecimals param though, if we specify it, it will result in a lot of extra zeros for most pairs, which would not look pretty.

@EvgeniiaVak
Copy link
Member

I think having the spread on a row below would be fine, or we could remove the spread value and just show the percentage?

even if we remove the spread value, if the price number is very long it might still overlap, moving spread to a row below (but only if they don't fit) would be better

@fliebenberg
Copy link
Contributor

I agree with using utils.displayNumber here, with something large for noDigits (so it wouldn't shorten the large prices with Ks and Ms, maybe 18?). I don't think we need to set upfixedDecimals param though, if we specify it, it will result in a lot of extra zeros for most pairs, which would not look pretty.

I think using displayNumber with noDigits set to 10 should work fine. It is very unlikely that prices will ever be more than 9 999 999 999. The priceMaxDecimals for any price higher than 99 999 will be 0. The most decimals a price could ever have is 8, so even in that case a noDigits of 10 should work ("0." and 8 decimals = 10 digits).
I agree that only displaying % spread when space is low should be fine.

@nguvictor nguvictor requested a review from EvgeniiaVak January 24, 2024 13:41
src/app/components/OrderBook.tsx Outdated Show resolved Hide resolved
@nguvictor nguvictor requested a review from EvgeniiaVak January 24, 2024 17:30
@EvgeniiaVak EvgeniiaVak merged commit 0769c94 into DeXter-on-Radix:main Jan 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants