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

[VDG] Privacy warnings Improved UX #11463

Merged
merged 20 commits into from Mar 1, 2024

Conversation

ichthus1604
Copy link
Collaborator

image

MaxHillebrand
MaxHillebrand previously approved these changes Sep 11, 2023
Copy link
Member

@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.

CACK

@soosr soosr dismissed MaxHillebrand’s stale review September 12, 2023 09:13

The merge-base changed after approval.

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.

Something is wrong:
image

Also please add a transition effect to fading.

@ichthus1604
Copy link
Collaborator Author

@soosr

Something is wrong

Fixed ;)

Also please add a transition effect to fading.

Done ;)

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.

Fade animation can glitch
fadebug

@Pule08
Copy link
Collaborator

Pule08 commented Oct 9, 2023

hi @ichthus1604
could you please update us about this PR? What is the status?

Thank you!

@ichthus1604
Copy link
Collaborator Author

@soosr Latest commit seems to have fixed the animation glitch.
Can you please test it again?

@ichthus1604
Copy link
Collaborator Author

@Pule08 I just fixed the remaining problems with this PR, if @soosr approves this can be merged.

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

cack

the fadeout is nice

tbh I have some doubt if the text reduction is an improvement for normal user. "Full privacy" what does it mean? Yes you could imply what it means by the effect when hovering over it, but it requires some thinking.
(adding a tooltip to the suggestion with previous text "only use private coins", might help?)

nit: for the change avoidance, instead of Send more by X BTC, maybe use Send X BTC more?

@soosr
Copy link
Collaborator

soosr commented Oct 16, 2023

tbh I have some doubt if the text reduction is an improvement for normal user. "Full privacy" what does it mean? Yes you could imply what it means by the effect when hovering over it, but it requires some thinking. (adding a tooltip to the suggestion with previous text "only use private coins", might help?)

The thing is it not only will only use private coins but also remove the change, remove the labels and so. A generic tooltip about what is the goal with that suggestion could help though. Add it in a separate PR.

nit: for the change avoidance, instead of Send more by X BTC, maybe use Send X BTC more?

nACK. It was one of the main points to have the USD value at the end of the row so the users could easily notice it without searching.

@yahiheb
Copy link
Collaborator

yahiheb commented Feb 12, 2024

@ichthus1604 What's up with this?

@yahiheb yahiheb added this to the v2.0.7 milestone Feb 19, 2024
@soosr
Copy link
Collaborator

soosr commented Feb 26, 2024

@ichthus1604 We should revive this PR

@ichthus1604
Copy link
Collaborator Author

@soosr this PR is working fine as far as I can tell, after merging latest master

I can't reproduce the animation glitch... maybe it was an Avalonia issue in a previous version?

My only concern is that light theme makes the Flyout look very ugly with current colors:

image

@soosr
Copy link
Collaborator

soosr commented Feb 29, 2024

I can't reproduce the animation glitch... maybe it was an Avalonia issue in a previous version?

Me neither.

My only concern is that light theme makes the Flyout look very ugly with current colors:

Even horrible on master, UI refreshment will fix it.

Two more thing to implement:

@ichthus1604
Copy link
Collaborator Author

@soosr

Two more thing to implement:

#11463 (comment)

Done ;)

When a warning is faded out, could make the text crossed out too?

Done ;)

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.

tACK

@soosr soosr merged commit a769f28 into zkSNACKs:master Mar 1, 2024
2 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VDG] Change avoidance: Display suggested amount in btc not usd
6 participants