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

fix: add copy button to some dropdown accounts #7689

Merged
merged 4 commits into from
Sep 4, 2024

Conversation

NeOMakinG
Copy link
Collaborator

@NeOMakinG NeOMakinG commented Sep 4, 2024

Description

There are a few places where we want to add the copy button to the dropdown accounts components:

  • Send modal
  • Defi Overview
  • rFOX addresses

I also removed the account auto selection based on the highest amount account, because it was causing a bug inside AccountDropdown where this component didn't know the accountId changed, it was used as default value but not as a reactive value

As this logic is buggy and almost everyone agrees that we should ditch it, I removed it and the bug naturally disappeared

⚠️ We shouldn't show this for UTXOs accounts

Follow up of #7663 as AccountDropdown was a specific case

Issue (if applicable)

closes #7624

Risk

Low

High Risk PRs Require 2 approvals

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

Testing

  • Go to Send modal, select an asset and add an address, the account selected should be kopiable
  • Go to any defi opportunity, in the overview view, the account selected should be kopiable
  • Every rFOX AccountDropdown components should be kopiable
  • For all those cases, in case the account is an UTXO, it should not be kopiable

Engineering

n/a

Operations

n/a

  • 🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

image
image
image
image

@NeOMakinG NeOMakinG requested a review from a team as a code owner September 4, 2024 09:48
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

see inline comments

src/pages/RFOX/components/AddressSelection.tsx Outdated Show resolved Hide resolved
src/features/defi/components/Overview/Overview.tsx Outdated Show resolved Hide resolved
src/pages/RFOX/components/RFOXHeader.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tested locally:

  • DeFi ✅

https://jam.dev/c/8b98c3aa-8a6e-4cc5-a684-120ba8d46b46

  • Send input ✅

https://jam.dev/c/a34bf5b7-d7cb-4135-bfc0-45d160b1186f

  • RFOX 🚫

Multi-account RUNE address isn't copied, the 0th account_index is instead

https://jam.dev/c/2fcd4ebf-b8c5-419e-aa78-fa57938f73d8

src/pages/RFOX/components/AddressSelection.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Retested locally, confirmed RFOX is now happy:

  • RUNE Addy

https://jam.dev/c/ce329792-051d-4148-a80a-eabccf4fe242

  • FOX.ETH / FOX.ARB addy:

https://jam.dev/c/a00d342f-e294-4823-9db4-5656e24aeb64

@gomesalexandre gomesalexandre merged commit bc1ed24 into develop Sep 4, 2024
3 checks passed
@gomesalexandre gomesalexandre deleted the dropdown-accounts branch September 4, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Addresses in app should be kopiable
2 participants