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

Show wallet gas and total value, show neo/gas prices #291

Merged
merged 5 commits into from
Nov 1, 2017

Conversation

slipo
Copy link
Contributor

@slipo slipo commented Oct 31, 2017

What current issue(s) from Trello/Github does this address?
None

What problem does this PR solve?
Currently the app just shows the NEO wallet value and excludes GAS.

How did you solve this problem?
Now the app will show the quoted NEO/GAS prices and the NEO/GAS wallet values as four separate data items. In addition it shows a sum total wallet value.

How did you make sure your solution works?
There are automated tests. I also manually checked the app to see the values.

Are there any special changes in the code that we should be aware of?
It's switching from bittrex API to coinmarketcap because the latter has the GAS USD market.

Is there anything else we should know?

  • Unit tests written?
    yes!

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 43.607% when pulling bebd949 on slipo:dev into d549bbe on CityOfZion:dev.

@shawnmclean
Copy link
Contributor

shawnmclean commented Oct 31, 2017

usd gas

I'm approving this. The design, I'm not sure if anyone else wants to give any feedback on that.

@evgenyboxer
Copy link
Contributor

PR looks good.
I’m terms of the design, I think it’s better to drop the value of a single Neo/Gas.

Under each asset just write the total, without the wording “NEO/GAS Wallet”, as it is inferred from the context.

I agree that having a total for both is a good idea. In terms of the wording, maybe instead of “Wallet”, just “Total”.

@slipo
Copy link
Contributor Author

slipo commented Oct 31, 2017

Is this what we're looking for? If so I'll push it out.

designchangesneon

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 43.586% when pulling d4b43a3 on slipo:dev into d549bbe on CityOfZion:dev.

@slipo
Copy link
Contributor Author

slipo commented Oct 31, 2017

OK, those changes were looking pretty good to me so I pushed them out. Let me know if there's anything else to do.

@shawnmclean
Copy link
Contributor

Looks much cleaner!

@shawnmclean shawnmclean merged commit e190314 into CityOfZion:dev Nov 1, 2017
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.

4 participants