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

Try to display balance in top bar. #38

Merged
merged 4 commits into from
Sep 14, 2019
Merged

Conversation

JoshOrndorff
Copy link
Contributor

This is an attempt to display a user's balance in the top bar along with the account selector. I wanted this for my marketplace dApp, and figured it would be generally useful.

The top bar correctly displays a user's balance when you select an account, and hides the balance display when no account is selected.

I can't understand why the display never updates when the user's balance changes (eg they transfer some tokens). In particular, I can't understand why the subscription callback is not called. Would be happy to pair program with someone on this.

Copy link
Contributor

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

No need for unsubBalance

setAccountAddress(initialAddress);
}, [setAccountAddress, initialAddress])
// Unsubscribe previous account's balance if subscription exists
unsubBalance && unsubBalance();
Copy link
Contributor

@Tbaut Tbaut Sep 13, 2019

Choose a reason for hiding this comment

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

This it not needed. As soon useEffect is called again (because one of the dependency changes) the return from the useEffect is called (so you are unsubscribed).

Just add a console.log in the return of the useEffect to see that it's called as soon as you change the selection. Hence you do not need to keep track of the unsubsciptions, and generally do not need the unsubBalance state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoshOrndorff JoshOrndorff marked this pull request as ready for review September 13, 2019 18:03
@JoshOrndorff
Copy link
Contributor Author

Thanks to Thibaut's help, this is now working and ready for further review and merge.

Copy link
Contributor

@riusricardo riusricardo left a comment

Choose a reason for hiding this comment

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

NIce! LGTM

@riusricardo riusricardo merged commit 56961f1 into master Sep 14, 2019
@riusricardo riusricardo deleted the joshy-balance-topbar branch September 14, 2019 14:51
@shawntabrizi
Copy link
Contributor

Functionality works as expected, but imo implementation is kind of ugly.

I would suggest we use a label: https://react.semantic-ui.com/elements/label/

On the right side of the account list.

I also suggest we add an "extra" react plugin which handles account balances better.

For example, something which can configure what the "unit" of the chain is (how many digits are decimals), and display big numbers using the Kilo, Mega, Giga format we are used to (or maybe directly scientific notation).

This could go into a folder called "utilities" next to the other components. Users would be able to reuse this component whenever displaying a balance in their own component.

@JoshOrndorff
Copy link
Contributor Author

Yeah, totally ugly. I agree.

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

4 participants