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

[UI] Distinguish between 1 and multiple addresses for copy button #12224

Merged

Conversation

SuperJMN
Copy link
Collaborator

@SuperJMN SuperJMN commented Jan 10, 2024

Context: Transaction Info / Addresses item.

  • When there's only one address, the copy button behaves like the other elements in the Transaction Info view.
  • When there's multiple addresses, the copy button appears per item.

Fixes #11985

Note: This is the easiest way I have figured out to get a good UX, although I'm not generally fine with incresing complexity for such a little gain.

@SuperJMN SuperJMN changed the title [UI] Distinguish between one or more addresses for copy button [UI] Distinguish between 1 or multiple addresses for copy button Jan 10, 2024
@SuperJMN SuperJMN changed the title [UI] Distinguish between 1 or multiple addresses for copy button [UI] Distinguish between 1 and multiple addresses for copy button Jan 10, 2024
yahiheb
yahiheb previously approved these changes Jan 11, 2024
Copy link
Collaborator

@yahiheb yahiheb 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
Copy link
Collaborator

soosr commented Jan 11, 2024

@SuperJMN Pushed some changes to remove the ugly part, please check if you are happy with the current code.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Jan 11, 2024
@SuperJMN
Copy link
Collaborator Author

SuperJMN commented Jan 11, 2024

@SuperJMN Pushed some changes to remove the ugly part, please check if you are happy with the current code.

Your changes are smart and clear. Definitely an improvement over my initial approach. Great job!

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.

tack 0a3a1c2

bit of a nit: if there's more than 1 address, I need to hover over the address for the copy button to show up.
could it have the same behavior as single address: hover over the context line for copy to show up?
maybe this is out of scope for this PR as this also applies for TXID's at coinjoins details

yahiheb
yahiheb previously approved these changes Jan 11, 2024
Copy link
Collaborator

@yahiheb yahiheb 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
Copy link
Collaborator

soosr commented Jan 12, 2024

bit of a nit: if there's more than 1 address, I need to hover over the address for the copy button to show up.
could it have the same behavior as single address: hover over the context line for copy to show up?

Not possible.

maybe this is out of scope for this PR as this also applies for TXID's at coinjoins details

Make sense to me to add it here. @SuperJMN Can you apply the same modifications?

@MarnixCroes
Copy link
Collaborator

maybe this is out of scope for this PR as this also applies for TXID's at coinjoins details

Make sense to me to add it here. @SuperJMN Can you apply the same modifications?

not sure what the difference should be in GUI now with ad4c97f, or is it not working?

@SuperJMN
Copy link
Collaborator Author

maybe this is out of scope for this PR as this also applies for TXID's at coinjoins details

Make sense to me to add it here. @SuperJMN Can you apply the same modifications?

not sure what the difference should be in GUI now with ad4c97f, or is it not working?

It only applies when there's only one address. But I don't know if that's even possible. @soosr is this OK?

@MarnixCroes
Copy link
Collaborator

MarnixCroes commented Jan 12, 2024

It only applies when there's only one address.

on master, hovering over the content line of single cj, already gives the copy button. So then there is no change in behavior right?

@SuperJMN
Copy link
Collaborator Author

It only applies when there's only one address.

on master, hovering over the content line of single cj, already gives the copy button. So then there is no change in behavior right?

For multiple addresses, no 😅

@soosr
Copy link
Collaborator

soosr commented Jan 15, 2024

maybe this is out of scope for this PR as this also applies for TXID's at coinjoins details

Make sense to me to add it here. @SuperJMN Can you apply the same modifications?

not sure what the difference should be in GUI now with ad4c97f, or is it not working?

It only applies when there's only one address. But I don't know if that's even possible. @soosr is this OK?

You were right, it is not even possible. Coinjoin group can never have 1 cj item...

@pull-request-size pull-request-size bot added size/S and removed size/M labels Jan 15, 2024
@soosr soosr merged commit a978e01 into WalletWasabi:master Jan 15, 2024
3 checks passed
@SuperJMN SuperJMN deleted the fixes/transaction-info-addresss-copy-button branch January 15, 2024 10:39
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.

[UI] Same behavior for Copy button in Transaction Details page
4 participants