Skip to content

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
@Zfinix Zfinix requested a review from andrzejchm February 21, 2022 20:36
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

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
@Zfinix Zfinix requested a review from andrzejchm February 23, 2022 00:40

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

3 participants