-
Notifications
You must be signed in to change notification settings - Fork 492
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
[VDG] Add colorful diffs in Transaction Preview #11704
[VDG] Add colorful diffs in Transaction Preview #11704
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The amount percentage difference is inverse?
both amount and fee
e.g. have a fee of 221 sats, hover over change avoidance, fee becomes 110 sats. It says the difference is 100.91%
It should be ~ -50%
Mmm, let me review the calculation. I did some last time changes that could be wrong. |
Yes, it was exactly the opposite. Good catch. Thank you! Please check if it works now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use NeutralDiffConverter
at amount
and InvertedDiffConverter
at fee
? in the TransactionSummary view
In the GUI, at Amount
, the brush is currently always orange, both for positive and negative difference
@@ -64,6 +64,10 @@ | |||
<SolidColorBrush x:Key="PrivacyLevelStrongBrush" Color="#3E992F" /> | |||
<SolidColorBrush x:Key="PrivacyLevelNoneBrush" Color="#B8B8B8" /> | |||
|
|||
<SolidColorBrush x:Key="PositiveBrush" Color="#5BB626" /> | |||
<SolidColorBrush x:Key="NegativeBrush" Color="#C3983B" /> | |||
<SolidColorBrush x:Key="ZeroBrush" Color="#C3983B" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ZeroBrush and NegativeBrush shouldn't be the same color right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can be, because in fact, there's no negative color defined in the specs. Should we define one? @soosr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just have a PositiveBrush
and a UncertainBrush
. Later if we need a negative brush, we can add one.
For amount it's the same because we can't decide if it's good or bad. For fees, we can :) |
I'd say to give it a neutral color then? |
Orange is the warning color it doesn't mean bad or good it just wants to get attention. For Example in settings when restart is needed. |
@SuperJMN Fix conflicts, please. |
ok. for me the confusing part is that at |
The rationale behind this is to reward to good rather than punish the bad thing. Which is followed software-wide. Although I see your point, during the UI Refreshment I will remember this. |
@SuperJMN Could you fix the conflict? |
@@ -27,4 +27,7 @@ public static class MoneyConverters | |||
|
|||
public static readonly IValueConverter ToFeeWithoutUnit = | |||
new FuncValueConverter<Money?, string?>(n => n?.ToFeeDisplayUnitRawString()); | |||
|
|||
public static readonly IValueConverter PercentageDifferenceConverter = | |||
new FuncValueConverter<double, string>(n => n.ToString("+#0.##%;-#0.##%;0%")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add conditional formatting here? Similarly what you did with USD amounts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Thanks for reporting! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK
This adds colored hints to the changing items when hovering over a suggestion, in the Transaction Preview. It helps users identify what they're getting applying a given suggestion.
Closes #11039