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.

Sign up for GitHub

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

Jump to bottom

[VDG] Coin Control - Stage 1 #9417

Merged
merged 55 commits into from
Nov 23, 2022
Merged

[VDG] Coin Control - Stage 1 #9417

merged 55 commits into from
Nov 23, 2022

Conversation

SuperJMN
Copy link
Collaborator

@SuperJMN SuperJMN commented Oct 28, 2022 •
edited

This is the simplified (non-dynamic) version of the Coin Control feature (#9350).

image

The strategy with this bigger feature is the following:

  • Create the feature in several stages (different PRs).
  • Review each PR to make it easier to review and understand.
  • Implement a kind of feature toggle so only developers can test the feature while it evolves.

This PR corresponds to the Stage 1 of the feature.

What's included?

  • The coin list with the final appearance and the correct data.

What is not included?

  • Actual coin selection.
  • Filtering.
  • Selected amount, remaining amount...
  • User cannot continue.

What will not be included?

  • Dynamic updates of the coin list. The coin list and its properties are fetch when the dialog opens and will not be refreshed afterwards. This means that:
    • Edge cases like receiving coins while the dialog is open will not be covered.
    • Statuses (confirmed, is coinjoining, banned until...) will not vary at all.

If users want to refresh the list, they will have to do a close/open cycle.

How to test

  1. Press the LEFT ALT key while in the Transaction Preview screen (summary)
  2. A new hyperlink button will appear.
  3. Click on it.

Remember that you won't be able to do anything on it just yet.

@SuperJMN SuperJMN changed the title [VDG]Coin control - Stage 1 [VDG] Coin control - Stage 1 Oct 28, 2022
@SuperJMN SuperJMN changed the title [VDG] Coin control - Stage 1 [VDG] Coin Control - Stage 1 Oct 28, 2022
@SuperJMN SuperJMN marked this pull request as ready for review October 28, 2022 09:39
MaxHillebrand
MaxHillebrand previously approved these changes Oct 28, 2022
View reviewed changes
Copy link
Collaborator

@MaxHillebrand MaxHillebrand left a comment

Choose a reason for hiding this comment

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

tACK 55b6f2a

sorting works, collapse expand works, everything seems alright.

wieslawsoltes
wieslawsoltes reviewed Oct 31, 2022
View reviewed changes
WalletWasabi.Fluent/Views/CoinControl/Core/Cells/LabelsCellView.axaml
<Style Selector="controls|LabelsItemsPresenter.SemiPrivate">
<Setter Property="BorderBrush" Value="{DynamicResource PrivacyLevelMediumBrush}" />
<Setter Property="Foreground" Value="{DynamicResource PrivacyLevelMediumBrush}" />
<Setter Property="MaxLabelWidth" Value="1000" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we want such long labels here, wouldn't it cause layout issues in TDG ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why 1000 ?

Copy link
Collaborator Author

@SuperJMN SuperJMN Nov 21, 2022 •
edited

Choose a reason for hiding this comment

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

Let me explain the problem:

The MaxLabelWidth is set in the default style to avoid large labels.

image

But when it comes to well-known labels (not real labels) like "Private Coins" "Unknown People" and so, we don't want the character ellipsis to be applied. Thus, I needed to override the value with a number high enough. This is the simplest solution, I've found, since double.PositiveInfinity can't be specified in XAML, AFAIK.

If there's a better solution, please tell me :)

@soosr
Copy link
Collaborator

soosr commented Nov 21, 2022

CI failing

@SuperJMN SuperJMN requested a review from soosr November 22, 2022 08:57
@SuperJMN SuperJMN requested a review from soosr November 22, 2022 15:28
soosr
soosr approved these changes Nov 23, 2022
View reviewed changes
Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

LGTM

@wieslawsoltes
Copy link
Collaborator

LGTM

@molnard molnard merged commit 425ade9 into WalletWasabi:master Nov 23, 2022
@SuperJMN SuperJMN deleted the coin-control branch November 23, 2022 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wieslawsoltes wieslawsoltes wieslawsoltes approved these changes

@soosr soosr soosr approved these changes

@MaxHillebrand MaxHillebrand MaxHillebrand left review comments

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@SuperJMN @wieslawsoltes @soosr @MaxHillebrand @molnard