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

Make error message layout consistent #560

Closed
wants to merge 2 commits into from

Conversation

w0xlt
Copy link
Contributor

@w0xlt w0xlt commented Feb 26, 2022

Currently, the error message for invalid change address is displayed on the right while in the "Pay to" and "Address" fields do not show the error message, as commented in #553 (comment).

This PR makes the address error feedback consistent across all fields.

Supersede #534

master
master_pay_to
master_addr_edit
PR
pr_pay_to
pr_addr_edit

@w0xlt w0xlt force-pushed the 3_error_message_addr branch 2 times, most recently from 5872209 to e820e44 Compare February 26, 2022 04:35
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 26, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK Rspigler
Concept ACK RandyMcMillan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #612 (refactor: Drop unused QFrames in SendCoinsEntry by hebasto)
  • #534 (Add translatable address error message by w0xlt)
  • #533 (Add more detailed address error message by w0xlt)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@katesalazar
Copy link
Contributor

I assume in this figure there is a static placeholder for the red label (normally empty):

I assume that's regular P2WPKH invoice address you are showing.

Would mean the destination address box be shorter. So what happens if destination address is a longer P2WSH address? Would it then clip while with the current code it would not?

@jarolrod jarolrod added the UI All about "look and feel" label Mar 10, 2022
@RandyMcMillan
Copy link
Contributor

Concept ACK

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@Rspigler
Copy link
Contributor

Rspigler commented Aug 1, 2022

tACK commit 9d75184

Tested all address types in the 'Pay To', 'Custom Change Address', and 'New Send Address' fields.

Just noting that it says "Warning: Invalid Bitcoin address" immediately, but only highlights red when you click out of the text box. I don't know if I have a preference there. But I like the change.

I get this message when opening:

QVariant::load: unknown user type with name BitcoinUnits::Unit.

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@hebasto
Copy link
Member

hebasto commented Mar 27, 2023

Closing this due to lack of activity. Feel free to reopen.

@hebasto hebasto closed this Mar 27, 2023
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Mar 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs rebase UI All about "look and feel"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants