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

Conversion implementation #520

Merged
merged 39 commits into from
Aug 16, 2016
Merged

Conversion implementation #520

merged 39 commits into from
Aug 16, 2016

Conversation

2-am-zzz
Copy link
Contributor

@2-am-zzz 2-am-zzz commented Aug 5, 2016

Work in Progress. Fixes #278, Fixes #461, Fixes #362. Pulling a PR here for accountability and team checks.

Essentially does these things (or hopes to do so):

  1. Uses the Cryptonator API to pull data occasionally on some fiat currency to ether conversion. Thankfully they don't have limits, so we can ping liberally.

  2. Allows users to select from a currently pre-defined list of fiat currencies from the config menu.

  3. Shows users the equivalent fiat value when hovering over any ether value.

@danfinlay danfinlay added this to the Future Release milestone Aug 9, 2016
@danfinlay
Copy link
Contributor

Make sure you merge master into your long-running branch periodically so it doesn't diverge too far!

@2-am-zzz
Copy link
Contributor Author

screenshot from 2016-08-16 14 24 14

screenshot from 2016-08-16 14 24 32

screenshot from 2016-08-16 14 24 56

@2-am-zzz
Copy link
Contributor Author

This PR is finished! Waiting for additional comments or a merge @FlySwatter @kumavis @frankiebee

@kumavis
Copy link
Member

kumavis commented Aug 16, 2016

@Zanibas is the NaN issue resolved? (see tx history in screenshots )

@danfinlay
Copy link
Contributor

I don't think you need an = before the {value} USD.

I would also add the currency suffix to the accounts page, and remove the equal sign there too.

@2-am-zzz
Copy link
Contributor Author

@FlySwatter it was suggested in Vlad's design: is it no good?

@kumavis
Copy link
Member

kumavis commented Aug 16, 2016

@Zanibas I don't really like it, maybe use parens instead

100 ether
( 1000 USD )

@danfinlay
Copy link
Contributor

I don't think equals or parens are needed. Different font color + currency suffix will make it very obvious.

EthBalanceComponent.prototype.renderBalance = function (value, state) {
console.log("THIS IS VALUE")
console.log(value)
console.log(state.conversionRate)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused logs

@danfinlay
Copy link
Contributor

Tests are failing. probably linter issues.

This PR looks good! Great work, @Zanibas, this was a large feature, and you've done it very cleanly, nice tests, good use of our state tree, metamask controller, config manager, very solid contribution!

@2-am-zzz
Copy link
Contributor Author

Removed the equal sign, but now to account for failing APIs, we just hide that part of the UI (no conversion is shown at all). Showing a message indicating unavailable conversions was pretty ugly--we can revisit this if this is undesired.

@danfinlay
Copy link
Contributor

I like the solution of hiding the value when it's unavailable.

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.

None yet

3 participants