Skip to content
This repository was archived by the owner on Apr 4, 2022. It is now read-only.

Add a default formatting of amount #782

Merged
merged 3 commits into from
Oct 20, 2021
Merged

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Oct 19, 2021

Summary

Closes #629

  • To display see/buy amounts shorter

To test

@henrypalacios henrypalacios self-assigned this Oct 19, 2021
@henrypalacios henrypalacios added app:Explorer Explorer App Protofire task to the protofire team labels Oct 19, 2021
@github-actions
Copy link

@elena-zh
Copy link

Hi @henrypalacios , great!

I'd like to add a label with a whole amount displaying on hover, like we do in the Cowswap app
full amount

Or, as another option, add a tooltip with this value, as labels are not displayed in a mobile view.

Btw, I found a great mismatch in prices displayed in the Cowswap for the USDT token and in the Explorer.
usdt

However, it must be related to Cowswap BE issue, I guess. @alfetopito , could you please take a look?

Thanks!

@alfetopito
Copy link
Contributor

Henry, regarding what I talked during the meeting, here's how CowSwap deals with the amounts:

formatSmart https://github.com/gnosis/cowswap/blob/develop/src/custom/utils/format.ts#L110

Many things here are particular to CowSwap though such as the CurrencyAmount class which do not apply to the Explorer

What can be useful though is the helper _buildSmallLimit https://github.com/gnosis/cowswap/blob/develop/src/custom/utils/format.ts#L42

And also how dex-js's formatSmart is called with the small limit calculation

@alfetopito
Copy link
Contributor

Hi @henrypalacios , great!

I'd like to add a label with a whole amount displaying on hover, like we do in the Cowswap app full amount

Or, as another option, add a tooltip with this value, as labels are not displayed in a mobile view.

Great suggestion Elena, I agree, it would be great to have full amounts as a title/tooltip

Btw, I found a great mismatch in prices displayed in the Cowswap for the USDT token and in the Explorer. usdt

However, it must be related to Cowswap BE issue, I guess. @alfetopito , could you please take a look?

Can you share the order where that happened?

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Looking much better already!

screenshot_2021-10-19_09-36-24
TOP: dev; BOTTOM: this PR

Now I'm uncertain regarding my comment to reduce the displayed decimals from 8 to 4.
8 is looking good.
What do you guys think?

@elena-zh
Copy link

Tooltips look great!

As for the Qty of decimals, I'd vote for 8 as I expect from the app to see more detailed information without need of an additional action (hover on an amount or so).
Besides, prices show 8 decimals.

And a new tiny issue with this PR: lines vertical alignment has become different in a mobile view:
image

@henrypalacios
Copy link
Contributor Author

henrypalacios commented Oct 20, 2021

I agree with @elena-zh, with 8 decimal places I can see the detail without resorting the tooltip.
(I stayed for a while to choose how it looks better if with 6 or 8 decimals 😅)

@alfetopito Maybe the doubt that I have now is, how to deal with these cases, are you left with the tooltip?
Screenshot from 2021-10-20 11-59-43

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Great job

@alfetopito
Copy link
Contributor

I agree with @elena-zh, with 8 decimal places I can see the detail without resorting the tooltip. (I stayed for a while to choose how it looks better if with 6 or 8 decimals sweat_smile)

Yeah, let's stay with 8

@alfetopito Maybe the doubt that I have now is, how to deal with these cases, are you left with the tooltip? Screenshot from 2021-10-20 11-59-43

I don't see a problem with that. I prefer to be consistent and show it always.

@alfetopito alfetopito changed the base branch from release/2.3.0 to develop October 20, 2021 20:16
@alfetopito
Copy link
Contributor

BTW, I've changed the base to develop since 2.3.0 has been deployed and there's no need for a hotfix for this.
It can go with other stuff on 2.4.0

@alfetopito alfetopito merged commit a58b545 into develop Oct 20, 2021
@alfetopito alfetopito deleted the 629-display-amount-better branch October 20, 2021 20:18
@alfetopito
Copy link
Contributor

@alfetopito , here it is https://protocol-explorer.dev.gnosisdev.com/rinkeby/orders/0x2b10c4f99535c4fa7800e5a7bd366280c6a24cbf83e5278c5fae5f3998116ca6ff714b8b0e2700303ec912bd40496c3997ceea2b616ea078

The issue in on CowSwap side.
The data displayed on the Explorer matches the data returned by the API.

I assume the issue could be because USDT usually has 6 decimals precision, but this contract has 18.
Maybe the rinkeby token list has the wrong decimals configured

@elena-zh
Copy link

@alfetopito , thanks! I will create an issue in the Cowswap repo then.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
app:Explorer Explorer App Protofire task to the protofire team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User orders list: display amounts better
3 participants