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

peng/518 hide assets not holding #633

Merged
merged 4 commits into from
Apr 12, 2018
Merged

Conversation

nylira
Copy link
Contributor

@nylira nylira commented Apr 12, 2018

Issue

Closes #518

Screenshots

screen shot 2018-04-12 at 12 07 40 pm

@faboweb
Copy link
Collaborator

faboweb commented Apr 12, 2018

In the mocks by Gautier there was a button to show all assets. Do we want that?

@nylira
Copy link
Contributor Author

nylira commented Apr 12, 2018

I was thinking about that too, having a toggleable button. I concluded its less maintenance for us and simpler for the user to just hide 0 balance tokens. They can't do anything with that data so it'll just be visual noise and additional complexity.

@faboweb
Copy link
Collaborator

faboweb commented Apr 12, 2018

Your are right. Let's keep it like this then.

@codecov
Copy link

codecov bot commented Apr 12, 2018

Codecov Report

Merging #633 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #633   +/-   ##
========================================
  Coverage    85.62%   85.62%           
========================================
  Files           91       91           
  Lines         1524     1524           
  Branches        69       69           
========================================
  Hits          1305     1305           
  Misses         205      205           
  Partials        14       14
Impacted Files Coverage Δ
app/src/renderer/components/wallet/PageWallet.vue 85.29% <ø> (ø) ⬆️

@faboweb faboweb merged commit 89e1456 into develop Apr 12, 2018
@faboweb faboweb deleted the peng/518-hide-assets-not-holding branch April 12, 2018 12:08
@jbibla
Copy link
Collaborator

jbibla commented Apr 12, 2018

i just want to flag that i believe we agreed to hide and optionally reveal all the denominations on the network.

i'm bringing it up so we can improve our processes - not to revert any changes or propose i'm unsatisfied with this decision.

@faboweb
Copy link
Collaborator

faboweb commented Apr 12, 2018

Are you not satisfied with the discussion above?

@jbibla
Copy link
Collaborator

jbibla commented Apr 12, 2018

well, i would have preferred to still show all the denoms on the network. and i worked on a ticket to make this happen. so this is ok - there are good reasons on both sides. i'm just saying we already decided on this a while back, so i'd like us to note that we have an opportunity to improve the way we log and discuss our product design decisions.

@nylira
Copy link
Contributor Author

nylira commented Apr 13, 2018

@jolesbi I missed the original conversation around the ticket #518. I followed the description on the ticket ("We want to hide tokens, that have a balance of 0 so the user can focus on things that are important."). I didn't know that toggling the tokens was part of the spec, so I didn't implement it to save me some work.

That said, I still believe there's no point in showing tokens to the users they cannot access. On Ethereum wallets and blockchain explorers, they do not show that you have 0 balances of every ERC20 token. On websites that do show 0 balances of supported tokens (like the Poloniex crypto exchange), the page ends up being very long and overloaded with information. For Polo, it is required because each token has its own "receive address". But we only have one receive address in Cosmos.

(the page keeps going)
polo-step2a-big

@faboweb
Copy link
Collaborator

faboweb commented Apr 13, 2018

I added a ticket so we don't forget to talk about this later on.

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.

3 participants