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] Send - move recipient entry to the first dialog #12657

Merged
merged 20 commits into from Mar 25, 2024

Conversation

wieslawsoltes
Copy link
Collaborator

@wieslawsoltes wieslawsoltes commented Mar 12, 2024

Fixes #12650

image

@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 12, 2024
@wieslawsoltes wieslawsoltes changed the title [VDG] Add labels TagBox xaml [VDG] Send - move recipient entry to the first dialog Mar 13, 2024
@wieslawsoltes wieslawsoltes changed the title [VDG] Send - move recipient entry to the first dialog [UI] Send - move recipient entry to the first dialog Mar 13, 2024
@wieslawsoltes wieslawsoltes marked this pull request as ready for review March 18, 2024 11:50
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.

tACK 1115183
tested several things, including pasting a BIP21 URI that contains label

one nit: fill everything in -> click continue -> preview tx page -> go back -> amount is auto selected
Is that desired: the last entry being auto selected makes more sense no?

@wieslawsoltes
Copy link
Collaborator Author

tACK 1115183 tested several things, including pasting a BIP21 URI that contains label

one nit: fill everything in -> click continue -> preview tx page -> go back -> amount is auto selected Is that desired: the last entry being auto selected makes more sense no?

I guess so, we previously had Amount the last element, now it would be labels

@MarnixCroes
Copy link
Collaborator

MarnixCroes commented Mar 18, 2024

thinking about it, I think it would be better if the Recipient entry is different than the other two.
(Can be on same dialog, but some difference)
#12650 (comment)

@turbolay
Copy link
Collaborator

In this PR the Label is still requested after the Amount. It can be changed in the future ofc not necessarily on this PR, as I understand that this PR is made primarily to remove the extra dialog, which it does perfectly.

But to me, it's a clear testimony of the misunderstanding that there is around this screen in the context of Bitcoin. The Label has to be asked before the Amount, because the Label can help you to suggest an Amount.

If you know the Label, you know how many coins you can select without revealing more information to the recipient or to anyone, therefore it can help to display dynamically a Private amount clickable label, which is something that would solve most of the problems of the send workflow.

So to make the implementation more future proof, and keep improving this screen in the future to actually fix real problems, I'd suggest to move the Label field above the Amount field.

@soosr
Copy link
Collaborator

soosr commented Mar 19, 2024

thinking about it, I think it would be better if the Recipient entry is different than the other two. (Can be on same dialog, but some difference) #12650 (comment)

Thanks for the suggestion, we are not going to change it. #12650 (comment)

In this PR the Label is still requested after the Amount. It can be changed in the future ofc not necessarily on this PR, as I understand that this PR is made primarily to remove the extra dialog, which it does perfectly.

But to me, it's a clear testimony of the misunderstanding that there is around this screen in the context of Bitcoin. The Label has to be asked before the Amount, because the Label can help you to suggest an Amount.

If you know the Label, you know how many coins you can select without revealing more information to the recipient or to anyone, therefore it can help to display dynamically a Private amount clickable label, which is something that would solve most of the problems of the send workflow.

So to make the implementation more future proof, and keep improving this screen in the future to actually fix real problems, I'd suggest to move the Label field above the Amount field.

You must keep in mind that moving the recipient label to the first place is going to break the user's muscle memory which should only be done if the benefits are clear.
It is not clear... You brought up this idea before and we decided not to do it because it requires a huge amount of refactoring around send, and apart from the fact it raised many questions, we are not even sure it is going to be better. I believe Manual Control is going to fix all of your concerns.

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.

cACK. @ichthus1604 please review this PR

@soosr soosr requested a review from ichthus1604 March 19, 2024 06:59
@ichthus1604
Copy link
Collaborator

Cosmetic:

vertical space between To and Amount is slightly larger than between Amount and Recipient:

image

Comment on lines 76 to 79
_suggestionLabels = new SuggestionLabelsViewModel(WalletVm.WalletModel, Intent.Send, 3)
{
Labels = { }
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Initializing Labels is not required here:

Suggested change
_suggestionLabels = new SuggestionLabelsViewModel(WalletVm.WalletModel, Intent.Send, 3)
{
Labels = { }
};
_suggestionLabels = new SuggestionLabelsViewModel(WalletVm.WalletModel, Intent.Send, 3);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 730ca6e

@ichthus1604
Copy link
Collaborator

Shift+Tab navigation gets broken when focusing the TagsBox. Probably not caused by this PR but this PR makes it evident.

@wieslawsoltes
Copy link
Collaborator Author

Cosmetic:

vertical space between To and Amount is slightly larger than between Amount and Recipient:

image

@ichthus1604
Fixed 3364be8 and some more cosmetics d5a3d30

@wieslawsoltes
Copy link
Collaborator Author

Shift+Tab navigation gets broken when focusing the TagsBox. Probably not caused by this PR but this PR makes it evident.

I can't see anything wrong with keyboard navigation in TagsBox

@wieslawsoltes
Copy link
Collaborator Author

Shift+Tab navigation gets broken when focusing the TagsBox. Probably not caused by this PR but this PR makes it evident.

@ichthus1604
ok managed to fix it c57ecbd

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.

tACK.

@ichthus1604 ichthus1604 merged commit f4ce5dd into zkSNACKs:master Mar 25, 2024
6 of 8 checks passed
@nostitos
Copy link

nostitos commented Mar 25, 2024

Will we be able to review the address and amount when we click the final button???

Will we be able to at least SEE what the sat/bytes will be used without going 2 menu away?

is going to break the user's muscle memory which should only be done if the benefits are clear.

Wasabi could have 10x more users than it has now, I don't think there's much muscle memory worth bike shedding over at this time.

@yahiheb yahiheb mentioned this pull request Apr 21, 2024
12 tasks
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] Send - move recipient entry to the first dialog
6 participants