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

feat(coinmarket): possibility to proceed with unverified address #14320

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

adderpositive
Copy link
Contributor

@adderpositive adderpositive commented Sep 12, 2024

Issue

When a user gets to the second step in the Buy flow where they select a specific address to receive crypto and the Trezor is disconnected, Suite only allows to Show an unverified address and copy it so the user is stuck.

Design before

obrazek

Solution

Instead of “Show unverified address” in this dialog show “Proceed with unverified address” button, close the dialog, and allow a user to proceed to the partner’s checkout.

Design after

obrazek
obrazek

Related

Resolve #14310

@adderpositive adderpositive added feature Product related issue visible for end user +Invity Related to Invity project labels Sep 12, 2024
@adderpositive adderpositive self-assigned this Sep 12, 2024
@@ -572,6 +572,7 @@
"TR_COINMARKET_KYC_YES_REFUND": "KYC requested in exceptional cases. KYC not required for refunds. 🤝",
"TR_COINMARKET_KYC_NO_KYC": "KYC never required. Exceptional cases automatically refunded. 👍",
"TR_COINMARKET_KYC_DEX": "KYC never required. DEX swaps either succeed or fail. 👍",
"TR_COINMARKET_CONFIRM_ADDRESS": "Confirm address",
Copy link
Contributor

Choose a reason for hiding this comment

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

This string is generic enough that it could be used elsewhere in the app, let's just call it TR_CONFIRM_ADDRESS.

BTW you don't have to edit these files, adding a new string to messages.ts is enough.

Comment on lines 258 to 260
!device?.connected
? 'TR_COINMARKET_CONFIRM_ADDRESS'
: 'TR_CONFIRM_ON_TREZOR'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be easier to read without the negation:

device?.connected
    ? 'TR_CONFIRM_ON_TREZOR'
    : 'TR_COINMARKET_CONFIRM_ADDRESS'

Copy link
Contributor

Choose a reason for hiding this comment

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

A new component might be unnecessary. Could you use ConfirmUnverifiedModal instead? It could be updated to use the NewModal while you're at it.

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'm not sure about that. I think the ConfirmUnverifiedModal would become messier if more logic were added. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked, it wouldn't really change much. Minor change to props. Updating that modal to new design would be a nice bonus.

@Hermez-cz
Copy link

Change the modal component to the warning one please: https://www.figma.com/design/dYIRTk1qVcKsrJDqOzZ7SG/Modals?node-id=4037-2006&t=6JHZmmhTkl2jrwnV-1

Agreed with the UX designer that the yellow warning state should use only together with the icon as seen in the design.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just checked, it wouldn't really change much. Minor change to props. Updating that modal to new design would be a nice bonus.

Comment on lines +135 to +143
// close modals and reset addressVerified on device connection change
useEffect(() => {
dispatch({
type: COINMARKET_BUY.VERIFY_ADDRESS,
addressVerified: undefined,
});
dispatch(modalActions.onCancel());
}, [device?.connected, dispatch]);

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should be moved inside the modal. It is only related to ProceedUnverifiedAddressModal, isn't it?

If you use ConfirmUnverifiedModal, the useEffect is already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is related. But it is not possible to clear the addressVerified, if the modal is not opened.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I thought addressVerified was only set inside the modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is only set in the modal. But It is also necessary to set the value as undefined if the modal is not active and the connection of Trezor will change.

@adderpositive
Copy link
Contributor Author

adderpositive commented Sep 18, 2024

I have created a new component ConfirmUnverifiedModalContainer where I moved all logic of unverified modals. It handles receive, xpub, and proceed modals. What do you think about that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Product related issue visible for end user +Invity Related to Invity project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coinmarket - Allow buy without connected Trezor device
3 participants