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

Show a confirmation of successfully sending BTC or BSQ from wallet #5071

Merged
merged 2 commits into from Jan 14, 2021
Merged

Show a confirmation of successfully sending BTC or BSQ from wallet #5071

merged 2 commits into from Jan 14, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 10, 2021

As requested, this shows a message of successful sending.
The confirmation details include amount, recipient address and txId.
Includes a link to open txId details in the user's external block explorer.
To be consistent it does the same for both BSQ and BTC.

Fixes #5067

Screenshots:

SendBsqTxId

Note the technicality with sending BSQ is you cannot look up the details in a BSQ explorer until +1 bitcoin confs have passed.

@m52go any thoughts on improving the note text shown in the above popup?

https://github.com/jmacxx/bisq/blob/8f24f0ab9c6d12143c09ecd1bb3bcbaa082bb3e4/core/src/main/resources/i18n/displayStrings.properties#L2699


SendBtcTx

@wiz
Copy link
Contributor

wiz commented Jan 10, 2021

Nice!! Not related to this PR but it would be cool to have 2 links to the block explorer, one for clearnet and one for Tor

@pazza83
Copy link

pazza83 commented Jan 10, 2021

Thanks for doing this so quickly.

Would also be good if mining fees where displayed.

@ghost
Copy link
Author

ghost commented Jan 10, 2021

Mining fees are displayed on the "Confirm withdrawal request" popup immediately prior. There's no need to display them after sending.
Screenshot from 2021-01-10 14-10-23

The confirmation details include amount, recipient address and txId.
Includes a link to open txId details in the user's external block explorer.
@m52go reworded the BSQ conf explanation note.
Added "don't show again" checkbox.
@ghost
Copy link
Author

ghost commented Jan 11, 2021

Added "Don't show again" checkbox.

image

@ripcurlx ripcurlx added this to the v1.5.5 milestone Jan 11, 2021
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

NACK - it does work as expected, but I would suggest two small changes to use a more lightweight container in one case and improve the UX after the success popup is closed. Thanks for picking up this issue!

protected void addContent() {
GridPane.setColumnSpan(
addMultilineLabel(gridPane, ++rowIndex, note, 0), 2);
gridPane.add(new Label(""), 0, ++rowIndex); // spacer
Copy link
Contributor

Choose a reason for hiding this comment

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

I normally use for this cases Region which is more lightweight as Label

e.g.

Suggested change
gridPane.add(new Label(""), 0, ++rowIndex); // spacer
Region spacer = new Region();
spacer.setMinHeight(20);
gridPane.add(spacer, 0, ++rowIndex);

Would have the same visual appearance.

@@ -373,6 +375,12 @@ private void showPublishTxPopup(Coin receiverAmount,
@Override
public void onSuccess(Transaction transaction) {
log.debug("Successfully sent tx with id {}", txWithBtcFee.getTxId().toString());
String key = "showTransactionSentBsq";
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to get rid of the error message on the empty fields after the successful transfer as well.
For the BsqSendView this would mean changing in the showPublishTxPopup resultHandler into

() -> {
       receiversAddressInputTextField.setValidator(null);
       receiversAddressInputTextField.setText("");
       receiversAddressInputTextField.setValidator(bsqAddressValidator);
       amountInputTextField.setValidator(null);
       amountInputTextField.setText("");
       amountInputTextField.setValidator(bsqValidator);
       });

Copy link
Contributor

Choose a reason for hiding this comment

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

Not beautiful, but it works 😉

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @ripcurlx, implemented your suggestions. 👍

Disable validation errors when clearing edit fields
Implement spacer field using Region
@ripcurlx ripcurlx merged commit 01bfbe7 into bisq-network:master Jan 14, 2021
@ghost ghost mentioned this pull request Jan 24, 2021
@ghost ghost deleted the fix_issue_5067 branch May 29, 2022 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disorienting warnings during BSQ sending of funds
4 participants