-
Notifications
You must be signed in to change notification settings - Fork 652
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: Various mint name with crypto fixes #2145
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 few comments but approving anyways, looks good 👏
export type Props = ButtonProps & { | ||
assetNetwork: Network | ||
} | ||
export type Props = ButtonProps |
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.
can we add EOF here?
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.
Done!
|
||
const onCloseModal = useCallback(() => { | ||
onCloseFatFingerModal() | ||
return onClose() |
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.
return onClose() | |
onClose() |
just to keep it consistent with the one above
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.
As we're replacing the onClose
method from the original one to this one, the BuyWithCryptoModal
expects the type of the method to return the same type as the original onClose
. I could either keep the return there or change the type in the BuyWithCryptoModal
what do you find best?
{formatPrice(Number(gasCost.total), gasCost.token)} | ||
<span> + </span> | ||
</> | ||
) : null} | ||
<img src={selectedToken?.logoURI} alt={selectedToken?.name} /> | ||
<Popup |
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.
Just a thought... it would be nice to abstract this in a component (or even just a function) that recieves the token and returns the wrapped one. (to avoid repetetition of the props on
, position
, style
, etc, that are all the same in this cases)
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.
Done! Created the TokenIcon
component.
This PR fixes the following:
BuyWithCryptoButton
to be set as Matic so the white logo is shown. The red logo was fusing with the red color of the button, preventing it from being seen.ChainButton
when buying with card to be just a simple button.FatFinger
modal when closing theBuyWithCryptoModal
modal when paying for the minting of a name.InfoTooltip
that talked about paying the gas fee with the native token to remove the need to name all native tokens.