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

[VDG] Select correct wallet when clicking notification #11005

Merged
merged 1 commit into from Jul 13, 2023

Conversation

SuperJMN
Copy link
Collaborator

This PR:
Selects the correct wallet when a notification is clicked.

Without this PR:
The NavBar doesn't update its selection to reflect the correct wallet when a notification of a transaction is clicked.

Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

tACK

I don't really know what happened because I remember I opened an issue for this #10683 and then it was fixed in #10695 but I also noticed that the problem was back. Thanks for fixing it!

@@ -43,6 +43,7 @@ void OnClick()
return;
}

MainViewModel.Instance.NavBar.SelectedWallet = MainViewModel.Instance.NavBar.Wallets.FirstOrDefault(x => x.Wallet == wallet);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do:

// TODO: temp solution, remove model creation
var walletModel = new WalletModel(wallet);
UiContext.Navigate().To(walletModel);

So @ichthus1604 will see that we need to work with WalletModels here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think @ichthus1604 can review this to see if it's the correct approach given the current status of the refactor. I've seen this workaround applied before, though :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think @SuperJMN's code is okay for a temporary workaround. WalletManagerViewModel needs to die on fire anyway.

Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

tACK

@yahiheb
Copy link
Collaborator

yahiheb commented Jul 11, 2023

I don't really know what happened because I remember I opened an issue for this #10683 and then it was fixed in #10695 but I also noticed that the problem was back. Thanks for fixing it!

This most likely was broken in #10576

@SuperJMN
Copy link
Collaborator Author

I don't really know what happened because I remember I opened an issue for this #10683 and then it was fixed in #10695 but I also noticed that the problem was back. Thanks for fixing it!

This most likely was broken in #10576

Quite possibly. The NavBar code was really messed up. Refactoring it was quite hard and since we don't have tests covering this up, regressions can happen and will happen 😑.

Copy link
Collaborator

@ichthus1604 ichthus1604 left a comment

Choose a reason for hiding this comment

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

utACK.

@soosr soosr merged commit de80c5d into zkSNACKs:master Jul 13, 2023
6 of 8 checks passed
@SuperJMN SuperJMN deleted the fixes/select-wallet-notification branch July 24, 2023 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants