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

Currency rework #1180

Merged
merged 57 commits into from
Nov 30, 2022
Merged

Currency rework #1180

merged 57 commits into from
Nov 30, 2022

Conversation

goosewobbler
Copy link
Contributor

@goosewobbler goosewobbler commented Nov 16, 2022

  • Abstracting the USD and Ether / Currency display helpers to a displayValueData function
  • Shorthand display of large numbers, scaled decimals and lots of fixed edge cases
  • DisplayValue component uses displayValueData and generates appropriate markup for the supplied parameters
  • Moved test/app/Components to test/resources/Components to reflect the path of the tested files, updated test scripts accordingly

@goosewobbler goosewobbler added enhancement New feature or request UX labels Nov 24, 2022
resources/Components/DisplayValue/index.js Outdated Show resolved Hide resolved
</div>
<div className='signerBalancePrice'>
<div className='signerBalanceOk'>
<span className='signerBalanceCurrentPrice'>
{svg.usd(10)}{balance.price}
<DisplayValue type='fiat' value={`1e${decimals}`} valueDataParams={{ decimals, currencyRate, isTestnet, displayFullValue: true }} currencySymbol='$' />
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is value in this context? we used to show the price, what are we showing now?

Copy link
Contributor Author

@goosewobbler goosewobbler Nov 29, 2022

Choose a reason for hiding this comment

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

What price did we used to show? The value here is one unit of x (non-fiat currency), the other parameters being required to display that value in terms of fiat currency for a given currencyRate. We could probably wrap this for readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There you go, now it shows the price

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's just confusing is all. I would assume value would be what was previously the price but instead its 1 to the power of decimals which is not intuitive

resources/Components/DisplayValue/index.js Outdated Show resolved Hide resolved
resources/Components/DisplayValue/index.js Outdated Show resolved Hide resolved
resources/utils/displayValue.ts Outdated Show resolved Hide resolved
resources/utils/displayValue.ts Show resolved Hide resolved
@mholtzman
Copy link
Collaborator

@goosewobbler just left a couple comments. while I think this is a good step in the right direction, the interface in its current state is a bit complex and hard to follow. let's get this merged in and we can iterate over time into something that we find is easy to work with

@goosewobbler
Copy link
Contributor Author

@mholtzman Yeah, I definitely feel like this needs some iterating on. Already had a couple of cycles across myself and Jordan, could do with more. I'll reply to your comments and make some adjustments tomorrow.

@goosewobbler goosewobbler merged commit 36c8d18 into develop Nov 30, 2022
@goosewobbler goosewobbler deleted the currency-rework branch November 30, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants