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

Update receive modal with more complete options per type #4122

Merged
merged 3 commits into from
Aug 15, 2023

Conversation

pete-watters
Copy link
Contributor

@pete-watters pete-watters commented Aug 11, 2023

Try out this version of the Hiro Wallet - download extension builds.

This PR:

  • Fixes bugs in the Receive modal where we show the copy icon when we should show QR
  • Adds copy & QR to keep consistency as per Update receive modal with more complete options per type #3307
  • Refactors the receive modal to:
    • group components under pages/receive
    • use the same component to share logic for receive and receive-collectibles

The refactor is in a separate commit so I can drop that easily if required.

I've tested it and it seems to be working as previously but with the new buttons:

Kapture.2023-08-11.at.14.09.21.mp4

@pete-watters pete-watters self-assigned this Aug 11, 2023
@pete-watters pete-watters added the effort:small Expected to take up to 1 day of integration work label Aug 11, 2023
@pete-watters pete-watters linked an issue Aug 11, 2023 that may be closed by this pull request
}}
title="Ordinal inscription"
/>
<ReceiveItem
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't pass dataTestIds to these two as I didn't add new tests for them.

I think it's basically the same as the tests above for HomePageSelectors.ReceiveBtcNativeSegwitQrCodeBtn and HomePageSelectors.ReceiveStxQrCodeBtn.

I can add some more tests if desired to make sure we catch each button scenario

@pete-watters pete-watters force-pushed the chore/3307/improve-receive-modal branch from 080aea4 to cb1bac1 Compare August 14, 2023 08:16
@vercel vercel bot temporarily deployed to Preview August 14, 2023 08:17 Inactive
@markmhendrickson
Copy link
Collaborator

Looking good! Let's add the QR code option for Stamps as well while we're at it?

Screenshot 2023-08-14 at 12 41 05

children: React.ReactNode;
title?: string;
}
export function ReceiveItems({ children, title }: ReceiveItemsProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another name for this could be ReceiveItemList which avoids two very similar looking component names (only the s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice idea, I'll update it thanks!

@pete-watters
Copy link
Contributor Author

Looking good! Let's add the QR code option for Stamps as well while we're at it?

Screenshot 2023-08-14 at 12 41 05

OK , no problem 👍 Do we have a design for that screen?

I can make it the same as the one for BTC address:
Screenshot 2023-08-14 at 14 41 25

Maybe I should alter that slightly:

  • Bitcoin Stamps address as title
  • Remove the warning for Ordinal?

@vercel vercel bot temporarily deployed to Preview August 14, 2023 13:44 Inactive
@pete-watters pete-watters force-pushed the chore/3307/improve-receive-modal branch from f41305a to 7e1a6f1 Compare August 14, 2023 15:14
@vercel vercel bot temporarily deployed to Preview August 14, 2023 15:14 Inactive
@pete-watters
Copy link
Contributor Author

@markmhx:

I added a modal for BTC Stamp, and a new route /receive/btc-stamp:
Screenshot 2023-08-14 at 16 14 23

I thought it cleaner to add a new route if we want to update the title. The easiest thing for now is probably not to change the title and use the same modal.

Let me know if this is OK.

@kyranjamie : Also let me know if this is OK. The PR should probably be un-approved if I push new commits

@markmhendrickson
Copy link
Collaborator

LGTM!

@pete-watters pete-watters added this pull request to the merge queue Aug 15, 2023
Merged via the queue into dev with commit fc33210 Aug 15, 2023
@pete-watters pete-watters deleted the chore/3307/improve-receive-modal branch August 15, 2023 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort:small Expected to take up to 1 day of integration work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update receive modal with more complete options per type
4 participants