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

Wallet explorer utility buttons. #3384

Merged
merged 31 commits into from Mar 28, 2020
Merged

Wallet explorer utility buttons. #3384

merged 31 commits into from Mar 28, 2020

Conversation

nopara73
Copy link
Contributor

Build on top of #3373, thus closes #3373.

It fixes what @yahiheb noted: #3373 (comment)

Dan Walmsley and others added 18 commits March 26, 2020 11:37
…lity-buttons

# Conflicts:
#	WalletWasabi.Gui/Controls/WalletExplorer/WalletExplorerView.xaml
#	WalletWasabi.Gui/Controls/WalletExplorer/WalletExplorerViewModel.cs
#	WalletWasabi.Gui/Controls/WalletExplorer/WalletViewModel.cs
Co-Authored-By: Lucas Ontivero <lontivero@users.noreply.github.com>
…el.cs

Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
Co-Authored-By: yahiheb <52379387+yahiheb@users.noreply.github.com>
@nopara73
Copy link
Contributor Author

Note commits 08d6bf8, 6733809, 7c91846 and e16be15 were done to satisfy CodeScene.

However for CodeScene only 6733809 and 7c91846 are necessary. With that being said I'd keep the remaining 2 commits too as I think they're good, especially 08d6bf8.

@nopara73
Copy link
Contributor Author

nopara73 commented Mar 28, 2020

IMO this PR can be merged. Most of it is done by @danwalmsley and @jmacato here: #3373, but since I created a new PR, I cannot add an approve to this, but take this comment as an ACK.

danwalmsley
danwalmsley previously approved these changes Mar 28, 2020
Copy link

@danwalmsley danwalmsley left a comment

Choose a reason for hiding this comment

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

Was a couple of issues with the selection logic, pushed a fix.

Also iv made Wallet protected as important not to be public, we really want to avoid in future developer accessing a Wallet that doesnt belong to the associated viewmodel.

WalletViewModel property iv made private also for the same reason and added an ExpandWallet method.

yahiheb
yahiheb previously approved these changes Mar 28, 2020
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

Dan Walmsley added 2 commits March 28, 2020 12:32
This reverts commit 252bea3.

# Conflicts:
#	WalletWasabi.Gui/ViewModels/WasabiWalletDocumentTabViewModel.cs
@danwalmsley danwalmsley dismissed stale reviews from yahiheb and themself via c164b03 March 28, 2020 15:39
@danwalmsley
Copy link

  1. eliminated need for another base class and passing viewmodels around (tieing them together.)
  2. added IWalletViewModel interface so that Wallet property can remain private.
  3. WalletExplorerViewModel owns all the selection logic, (SRP) it uses its dictionary to locate the WalletViewModel and expand it.

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

Tested ACK.

@danwalmsley danwalmsley merged commit 8d0747c into zkSNACKs:master Mar 28, 2020
@nopara73 nopara73 deleted the wvm branch March 30, 2020 06:15
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