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

fixed currency-display #5619

Merged
merged 1 commit into from
Oct 30, 2018
Merged

Conversation

hackmod
Copy link
Contributor

@hackmod hackmod commented Oct 26, 2018

partial revert commit 99acde4

  • call getValueFromWeiHex() with fromCurrency=nativeCurrency
@@ -24,7 +25,7 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {

   const toCurrency = currency || currentCurrency
   const convertedValue = getValueFromWeiHex({
-    value, toCurrency, conversionRate, numberOfDecimals, toDenomination: denomination,
+    value, fromCurrency: nativeCurrency, toCurrency, conversionRate, numberOfDecimals, toDenomination: denomination,
   })
   const displayValue = formatCurrency(convertedValue, toCurrency)
   const suffix = hideLabel ? undefined : toCurrency.toUpperCase()

* call getValueFromWeiHex() with fromCurrency=nativeCurrency
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

@hackmod can you provide more detail about why/where this is needed? If anything we could pass fromCurrency: currency, no?

@hackmod
Copy link
Contributor Author

hackmod commented Oct 27, 2018

I can confirm that the minimal fix is the following with clean setup,. so strange..

--- a/ui/app/components/currency-display/currency-display.container.js
+++ b/ui/app/components/currency-display/currency-display.container.js
@@ -24,7 +24,7 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {

   const toCurrency = currency || currentCurrency
   const convertedValue = getValueFromWeiHex({
-    value, toCurrency, conversionRate, numberOfDecimals, toDenomination: denomination,
+    value, fromCurrency: 'ETH', toCurrency, conversionRate, numberOfDecimals, toDenomination: denomination,
   })
   const displayValue = formatCurrency(convertedValue, toCurrency)
   const suffix = hideLabel ? undefined : toCurrency.toUpperCase()

screenshot

with this fix

image

without this fix

image

@@ -16,6 +17,7 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {
const {
value,
numberOfDecimals = 2,
nativeCurrency = ETH,
Copy link
Contributor

Choose a reason for hiding this comment

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

We can pull this from state above (in mapStateToProps) and then we shouldn't need a default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. as you already mentioned and logically we don't have to apply this fix at all.
but as you can see, without this fix, currency-display do not show the correct value...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

drop last change

Copy link
Contributor Author

@hackmod hackmod Oct 30, 2018

Choose a reason for hiding this comment

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

last change seems no effect and formal fix looks redundant but it show correct value.

as you already pointed out fromCurrency: currency might be work logically but for currency == undefined case, currency-input show wrong value.

@@ -95,6 +95,7 @@ export default class CurrencyInput extends PureComponent {
return (
<CurrencyDisplay
className="currency-input__conversion-component"
nativeCurrency={nativeCurrency}
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted above, we can pull this from state

@hackmod hackmod force-pushed the currency-display branch 2 times, most recently from b9404c2 to cb37bbf Compare October 30, 2018 08:12
@whymarrh whymarrh merged commit ac07936 into MetaMask:develop Oct 30, 2018
@whymarrh
Copy link
Contributor

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants