-
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
[UI] Fix CoinJoin group status tooltip #11990
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.
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.
LGTM
This PR #11641 |
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.
LGTM
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.
@adamPetho
By the usage, It seems to me GetConfirmationText()
is meant to be used only for generating the Confirmed text.
In the method UpdateCoinjoinGroup
I believe we should just get the confirmationtooltip of the transaction that has the lowest confirmation number.
Why? Because if we have the FeeRate for that transaction, its message will also contain the confirmation time (in the case of pending) so the coinjoin group can also mention that.
This would result in a better UX.
coinjoinGroup.ConfirmedTooltip = coinjoinGroup.Children.MinBy(x => x.Confirmations)?.ConfirmedTooltip ?? "";
What do you think?
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
why is this merged? |
@MarnixCroes I didn't "fix" it all this time, because IMO there is nothing to fix there. I don't think that's an issue. |
PR:
Master:
I remember fixing this issue before... or was it only a dream?
Anyways, here is the fix.