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

fix: (balance) 🐛 fixed multiple balance load bug #217

Merged
merged 12 commits into from
Feb 23, 2022

Conversation

Zfinix
Copy link
Contributor

@Zfinix Zfinix commented Feb 17, 2022

Fixed bug causing the balanceList to load and update multiple times in _fetchWalletBalances() and in

  set selectedWalletIndex(int? value) {
    if (selectedWalletIndex != value) {
      Action(() => _selectedWalletIndex.value = value)();
      getBalances(selectedWallet.publicAddress); /// this line refreshes the balance list when the wallet index is set
    }
  }

fixes #210

@Zfinix Zfinix requested review from andrzejchm and wal33d006 and removed request for andrzejchm February 17, 2022 14:45
@Zfinix Zfinix self-assigned this Feb 17, 2022
@Zfinix Zfinix added the bug 🐛 Something isn't working label Feb 17, 2022
Copy link
Contributor

@andrzejchm andrzejchm left a comment

Choose a reason for hiding this comment

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

awesome work! just two minor comments ad we're good to go

Makefile Outdated Show resolved Hide resolved
Comment on lines 131 to 142
onTapDone: () async {
unawaited(
Navigator.of(context).pushAndRemoveUntil(
MaterialPageRoute(
builder: (_) => const AssetsPortfolioPage(),
),
(route) => false,
),
);

await StarportApp.walletsStore.getBalances(selectedWallet.publicAddress);
},
Copy link
Contributor

Choose a reason for hiding this comment

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

as a rule of a thumb, let's extract user interaction code to a separate method, outside of the build method for readability:

Suggested change
onTapDone: () async {
unawaited(
Navigator.of(context).pushAndRemoveUntil(
MaterialPageRoute(
builder: (_) => const AssetsPortfolioPage(),
),
(route) => false,
),
);
await StarportApp.walletsStore.getBalances(selectedWallet.publicAddress);
},
onTapDone: onTapDone,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

this one is still not updated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry about that didnt push the commit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can check it out now

Zfinix and others added 2 commits February 22, 2022 10:39

await StarportApp.walletsStore.getBalances(selectedWallet.publicAddress);
},
Future<void> _handleAssetTranserSheetDone(BuildContext context) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

the rule for naming the user event methods is to call them _onTap{what} _onLongPress{what}, onSwipe{{what} and then within the method we decide what to do with the action. this way its a bit easier to comprehend the intention of the method and eventually modifying the logic inside of it does not require us to rename it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see I mostly default to using the handle {what} method no problem will modify and follow this, Is there a way we can document these project rules and all.

Copy link
Contributor

@andrzejchm andrzejchm left a comment

Choose a reason for hiding this comment

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

🚀

@Zfinix
Copy link
Contributor Author

Zfinix commented Feb 23, 2022

🚀

Do i need to wait for @wal33d006 to approve?

@andrzejchm
Copy link
Contributor

I'll merge this one as Waleed is unavailable at the moment, but in the future let's give him a chance to review the code before merging :)

@andrzejchm andrzejchm merged commit 00a8928 into main Feb 23, 2022
@andrzejchm andrzejchm deleted the fix/duplicated-assets branch February 23, 2022 09:24
@Zfinix
Copy link
Contributor Author

Zfinix commented Feb 23, 2022

I'll merge this one as Waleed is unavailable at the moment, but in the future let's give him a chance to review the code before merging :)
Sure always 🙌🏾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicated assets on the Select assets page
2 participants