Change address / amount error background #553

pull w0xlt wants to merge 1 commits into bitcoin-core:master from w0xlt:change_error_background changing 1 files +1 −1
  1. w0xlt commented at 9:51 pm on February 18, 2022: contributor

    This PR proposes a small change in QLineEdit when there is an error in the input.

    master
    image
    PR
    image

    This also shows good results when combined with other open PRs.

  2. qt: change QLineEdit error background fe7c81e34e
  3. DrahtBot commented at 11:53 pm on February 18, 2022: contributor

    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 jarolrod, shaavan, GBKS

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

    Conflicts

    No conflicts as of last run.

  4. DrahtBot cross-referenced this on Feb 18, 2022 from issue Point out position of invalid characters in Bech32 addresses by luke-jr
  5. DrahtBot cross-referenced this on Feb 18, 2022 from issue Network Watch tool by luke-jr
  6. luke-jr commented at 1:26 am on February 19, 2022: member

    This also shows good results when combined with other open PRs.

    #537

    Well, now the background would be theme-dependent, and #537 will need to figure out a reasonable contrasting colour…

  7. promag commented at 0:01 am on February 22, 2022: contributor

    IMO this improves text contrast.

    However, other improvements could be made as well, some suggestions:

    • consistent error message near the invalid fields, for instance, the error message for invalid change address is displayed on the right while the upcoming bech32 error shows on the top
    • the amount field only turns red when clicking send and it’s zero
    • the send button remains enabled while a field is invalid, maybe it should be disabled
  8. jarolrod commented at 7:13 am on February 23, 2022: member

    ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25

    I’m ACK on this change specifically, there could be an accessibility improvement to this change (ping @RandyMcMillan)

    I disagree that, in the case of #537, this “shows good results when combined with other open PRs” :)

  9. w0xlt cross-referenced this on Feb 26, 2022 from issue Make error message layout consistent by w0xlt
  10. w0xlt commented at 5:13 am on February 26, 2022: contributor

    consistent error message near the invalid fields, for instance, the error message for invalid change address is displayed on the right while the upcoming bech32 error shows on the top @promag I addressed this suggestion in https://github.com/bitcoin-core/gui/pull/560

  11. DrahtBot cross-referenced this on Mar 2, 2022 from issue Warn when sending to already-used Bitcoin addresses by luke-jr
  12. jarolrod added the label UI on Mar 10, 2022
  13. RandyMcMillan commented at 10:21 am on March 16, 2022: contributor

    @jarolrod - thanks for pinging me on this - my apologizes for not seeing it sooner… @w0xlt,

    Just to define the issue with more clarity,

    We can see using this color contrast calculator that in the light theme (on Mac) that the black text on the rgb(255,128,128) is “ok” kinda… :)

    Screen Shot 2022-03-16 at 5 59 59 AM

    But when we switch to the dark theme (on Mac) the contrast becomes unsatisfactory from a contrast perspective and is apparent just by looking at it.

    Screen Shot 2022-03-16 at 6 00 29 AM

  14. RandyMcMillan commented at 10:50 am on March 16, 2022: contributor

    When fe7c81e34e2e16c4a5ec967645ebb49e161d3a25 is tested against light/dark theme changes (on Mac) we get…

    Screen Shot 2022-03-16 at 6 41 19 AM

    Screen Shot 2022-03-16 at 6 42 05 AM

    which doesn’t work because the text color is changed based on system theme…

  15. RandyMcMillan commented at 11:32 am on March 16, 2022: contributor

    Screen Shot 2022-03-16 at 7 26 55 AM

    Screen Shot 2022-03-16 at 7 27 09 AM

    Screen Shot 2022-03-16 at 7 27 35 AM

  16. RandyMcMillan commented at 11:36 am on March 16, 2022: contributor

    Here are a couple patches to experiment with palleteRole in Qt: You will have to call the text field to update during the system theme change - other wise it will only update when it receives focus.

     0diff --git a/src/qt/guiconstants.h b/src/qt/guiconstants.h
     1index 60e8ee2420..8b37c5dfb4 100644
     2--- a/src/qt/guiconstants.h
     3+++ b/src/qt/guiconstants.h
     4@@ -25,7 +25,7 @@ static const int STATUSBAR_ICONSIZE = 16;
     5 static const bool DEFAULT_SPLASHSCREEN = true;
     6 
     7 /* Invalid field background style */
     8-#define STYLE_INVALID "border: 3px solid #FF8080"
     9+#define STYLE_INVALID "color: palette(highlighted-text); background-color: pallet(shadow); border: 3px solid #FF8080"
    10 
    11 /* Transaction list -- unconfirmed transaction */
    12 #define COLOR_UNCONFIRMED QColor(128, 128, 128)
    
     0diff --git a/src/qt/guiconstants.h b/src/qt/guiconstants.h
     1index 60e8ee2420..dca896ca7e 100644
     2--- a/src/qt/guiconstants.h
     3+++ b/src/qt/guiconstants.h
     4@@ -25,7 +25,7 @@ static const int STATUSBAR_ICONSIZE = 16;
     5 static const bool DEFAULT_SPLASHSCREEN = true;
     6 
     7 /* Invalid field background style */
     8-#define STYLE_INVALID "border: 3px solid #FF8080"
     9+#define STYLE_INVALID "color: palette(highlighted-text); background-color: pallet(window); border: 3px solid #FF8080"
    10 
    11 /* Transaction list -- unconfirmed transaction */
    12 #define COLOR_UNCONFIRMED QColor(128, 128, 128)
    
  17. w0xlt commented at 4:57 am on April 12, 2022: contributor
    @RandyMcMillan Thanks for the detailed review and suggestions. I will apply and test them.
  18. jarolrod approved
  19. jarolrod commented at 2:10 am on June 28, 2022: member

    Revisited this, and it is in fact an improvement. Specifically, there’s is an improvement for text contrast when in dark mode. Additionally it makes the UI language around a sending error consistent.

    improvement with text contrast in dark mode:

    master PR
  20. shaavan approved
  21. shaavan commented at 12:38 pm on July 1, 2022: contributor

    ACK fe7c81e34e2e16c4a5ec967645ebb49e161d3a25

    • This change has improved over the current way of handling erroneous values.
    • Objectively, this PR shows definite accessibility improvements, as shown by @RandyMcMillan’s thorough review.
    • On a subjective note, I find the PR’s way of handling errors cleaner than the master’s way.

    Screenshot:

    Master PR
    Screenshot from 2022-07-01 14-43-58 Screenshot from 2022-07-01 17-48-54
  22. w0xlt commented at 12:02 pm on July 14, 2022: contributor
    @RandyMcMillan I tried your suggestions (palette(window), palette(shadow)) and also palette(base), but none of them worked. However I didn’t test on MacOS, I did it on Ubuntu by setting the theme using Qt5 Settings.
  23. Rspigler commented at 9:49 pm on August 1, 2022: contributor
    I actually prefer the red highlighting. IMO it’s more contrasting and more of an alert
  24. DrahtBot commented at 1:48 am on July 16, 2023: contributor

    There hasn’t been much activity lately. What is the status here?

    Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost 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.

  25. DrahtBot added the label CI failed on Jul 28, 2023
  26. DrahtBot removed the label CI failed on Aug 7, 2023
  27. DrahtBot added the label CI failed on Aug 18, 2023
  28. DrahtBot removed the label CI failed on Aug 23, 2023
  29. DrahtBot added the label CI failed on Aug 31, 2023
  30. DrahtBot removed the label CI failed on Sep 4, 2023
  31. DrahtBot added the label CI failed on Sep 13, 2023
  32. DrahtBot removed the label CI failed on Sep 18, 2023
  33. hebasto commented at 2:50 pm on September 22, 2023: member

    @GBKS

    Mind sharing your designer’s opinion regarding this PR approach.

  34. DrahtBot added the label CI failed on Oct 18, 2023
  35. DrahtBot removed the label CI failed on Oct 21, 2023
  36. DrahtBot added the label CI failed on Oct 25, 2023
  37. DrahtBot commented at 2:34 pm on February 5, 2024: contributor

    🤔 There hasn’t been much activity lately and the CI seems to be failing.

    If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

  38. GBKS commented at 2:41 pm on February 5, 2024: none

    ACK fe7c81e

    Just got the DrahtBot notification and realized I was asked for my input. Sorry, for the delay on this. I agree with this change. The text remains more legible. Also visually less jarring with the outline.

    If one wanted to keep a red background, it could also be a red with transparency (15-25% might be enough). That would avoid the thick border, still draw enough attention, and not impact contrast so much.

  39. hebasto merged this on Feb 7, 2024
  40. hebasto closed this on Feb 7, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 21:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me