-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
base: develop
Are you sure you want to change the base?
Conversation
@@ -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", |
There was a problem hiding this comment.
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.
!device?.connected | ||
? 'TR_COINMARKET_CONFIRM_ADDRESS' | ||
: 'TR_CONFIRM_ON_TREZOR' |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this comment.
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.
// close modals and reset addressVerified on device connection change | ||
useEffect(() => { | ||
dispatch({ | ||
type: COINMARKET_BUY.VERIFY_ADDRESS, | ||
addressVerified: undefined, | ||
}); | ||
dispatch(modalActions.onCancel()); | ||
}, [device?.connected, dispatch]); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I have created a new component |
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
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
Related
Resolve #14310