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

[UI] Merge Wallet & Coinjoin settings #12564

Merged
merged 32 commits into from Mar 14, 2024

Conversation

wieslawsoltes
Copy link
Collaborator

@wieslawsoltes wieslawsoltes commented Feb 26, 2024

Fixes #12351

@soosr
Copy link
Collaborator

soosr commented Mar 11, 2024

@wieslawsoltes Do you need any help with this?

@wieslawsoltes
Copy link
Collaborator Author

@wieslawsoltes Do you need any help with this?

Don't need it just requires further refactoring

@wieslawsoltes wieslawsoltes marked this pull request as ready for review March 12, 2024 08:56
Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

  • click on Change coinjoin strategy -> navigate back (back or continue button) -> it navigates to the General tab, instead of the expected coinjoin tab

  • coinjoin strategy can be missing: click on change coinjoin strategy -> click back without changing anything
    image

@wieslawsoltes
Copy link
Collaborator Author

wieslawsoltes commented Mar 13, 2024

  • click on Change coinjoin strategy -> navigate back (back or continue button) -> it navigates to the General tab, instead of the expected coinjoin tab

Can't repro:

screencast.2024-03-13.10-12-02.mp4

@wieslawsoltes
Copy link
Collaborator Author

  • coinjoin strategy can be missing: click on change coinjoin strategy -> click back without changing anything
    image

What do you mean by missing ?

@soosr
Copy link
Collaborator

soosr commented Mar 13, 2024

  • coinjoin strategy can be missing: click on change coinjoin strategy -> click back without changing anything
    image

What do you mean by missing ?

The name of the selected profile should appear next to the Coinjoin strategy: text. Can repro:
image

@wieslawsoltes
Copy link
Collaborator Author

  • coinjoin strategy can be missing: click on change coinjoin strategy -> click back without changing anything
    image

What do you mean by missing ?

The name of the selected profile should appear next to the Coinjoin strategy: text. Can repro: image

Ah ok will fix it :)

@soosr
Copy link
Collaborator

soosr commented Mar 13, 2024

  • click on Change coinjoin strategy -> navigate back (back or continue button) -> it navigates to the General tab, instead of the expected coinjoin tab

Can't repro: https://github.com/zkSNACKs/WalletWasabi/assets/2297442/eac3b1cd-14c6-4c36-9985-90383ddc2903

Cannot repro either. @MarnixCroes Are you on the latest commit?

@MarnixCroes
Copy link
Collaborator

  • click on Change coinjoin strategy -> navigate back (back or continue button) -> it navigates to the General tab, instead of the expected coinjoin tab

Can't repro: https://github.com/zkSNACKs/WalletWasabi/assets/2297442/eac3b1cd-14c6-4c36-9985-90383ddc2903

Cannot repro either. @MarnixCroes Are you on the latest commit?

tried several things, can't repro anymore. ed97729

@wieslawsoltes
Copy link
Collaborator Author

Update coinjoin settings

  • coinjoin strategy can be missing: click on change coinjoin strategy -> click back without changing anything
    image

What do you mean by missing ?

The name of the selected profile should appear next to the Coinjoin strategy: text. Can repro: image

Ah ok will fix it :)

Fix 3347e20

Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

minor things

@MarnixCroes
Copy link
Collaborator

also, why the creation of two tabs for backup and general?
why not keep them on the same tab?

yahiheb
yahiheb previously approved these changes Mar 13, 2024
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.

I have noticed some weird behavior with the coinjoin strategy settings but not related to this PR. #12667

I tested this a bit, LGTM.

@wieslawsoltes wieslawsoltes changed the title [VDG] Merge Wallet & Coinjoin settings [UI] Merge Wallet & Coinjoin settings Mar 13, 2024
Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

last bit


<TabItem Header="Tools" IsVisible="{Binding !IsWatchOnly}">
<ScrollViewer HorizontalScrollBarVisibility="Disabled" VerticalScrollBarVisibility="Auto">
<v:WalletBackupView />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename also the file for consistency

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok made some more renaming 86efd19

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.

also, why the creation of two tabs for backup and general?
why not keep them on the same tab?

This suggestion makes more sense now that we have a Tools tab instead of backup since renaming a wallet is more suited to be in the Tools tab and that there are plans to add Delete Wallet and Resync Wallet actions.

As it is now the General tab is completely empty, so removing it and making the Coinjoin the default is better UX imo.

image

@soosr
Copy link
Collaborator

soosr commented Mar 14, 2024

This suggestion makes more sense now that we have a Tools tab instead of backup since renaming a wallet is more suited to be in the Tools tab and that there are plans to add Delete Wallet and Resync Wallet actions.

Wallet name is not an action, it's a property that can be changed. While the others are actions that fit in tools tab.

As it is now the General tab is completely empty, so removing it and making the Coinjoin the default is better UX IMO.

It's not empty for HW wallets.

Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

tACK. Good Job!

@soosr soosr merged commit 300aab1 into zkSNACKs:master Mar 14, 2024
8 checks passed
@wieslawsoltes wieslawsoltes deleted the vdg/12351-merge-wallet-settings branch March 14, 2024 22:04
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.

[UI] Merge Wallet & Coinjoin settings
4 participants