-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add more onramp services #70
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
import EmbedPageWrapper from '../../EmbedPageWrapper'; | ||
|
||
type Props = { | ||
searchParams: { [key: string]: string | string[] | undefined }; |
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.
Note: once we migrate to turbopack, this can reimport SearchParamsProps
type (it need to be awaited)
/** | ||
* This code is run on every query param change, | ||
* we don't want to sanitize every query param change. | ||
* It should only be executed once per user per session. | ||
*/ | ||
if (searchParams.sanitized !== 'true') { | ||
addOrbitChainsToArbitrumSDK(); | ||
await sanitizeAndRedirect(searchParams, `${PathnameEnum.BUY}/${params.slug}`); | ||
} | ||
|
||
return ( | ||
<BridgePageWrapper searchParams={searchParams} redirectPath={`/bridge/buy/${params.slug}`} /> | ||
); |
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.
We can reuse bridgePageUtils
no?
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 is not needed for now because parent are already using use client
, but I would explicitely say that it requires it here
} | ||
return formatUSD(ethToUSD(Number(utils.formatEther(BigNumber.from(balanceState.value))))); | ||
}, [balanceState, ethToUSD, showPriceInUsd]); | ||
const isBalanceLessThan15Usd = Number(balanceInUsd?.replaceAll(/[$<]/g, '')) < 15; |
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 compare against balanceState
instead?
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.
We want to do <15 USD so it has to be the value in USD, rather than ETH balance value
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.
We can compare to ethToUSD(...)
, we would be able to compare without replacing anything
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.
oh perfect thanks
|
||
if (embedMode) { | ||
if (pathname === PathnameEnum.EMBED_BUY && showBuyPanel) { | ||
if (pathname.startsWith(PathnameEnum.EMBED_BUY) && showBuyPanel) { |
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.
We have this condition in few places, maybe we can abstract that away? (only the first part)
|
||
export function LinkoutOnrampPanel({ serviceSlug }: { serviceSlug: string }) { | ||
const { embedMode } = useMode(); | ||
const service = onrampServices.find((s) => s.slug === serviceSlug); |
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/FYI: When we have array of something we want to map and at the same time access. It's usually easier to have 2 objects:
- One Map<id, services>
- One array of only ids
className={twMerge( | ||
'relative flex w-full flex-col items-center justify-center rounded-md bg-gray-9 p-4 overflow-hidden', | ||
)} | ||
onClick={() => {}} |
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.
why do we have an empty onClick?
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.
we use the Link
anchor to navigate, but it is a button
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.
Then we can remove the onClick here
|
||
const allOnrampOnClick = useCallback(() => { | ||
router.push( | ||
'/projects?chains=arbitrum-one_arbitrum-nova_apechain_cheese_degen_ebi-xyz_pop-apex_reya_sanko_xai_xchain&subcategories=fiat-on-ramp', |
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.
Chains should not be hardcoded here. Let's simply redirect to the correct subcategory - https://portal.arbitrum.io/projects?subcategories=fiat-on-ramp
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 was given this link by product, let me check with them if they are ok with all chains
<div | ||
className={twMerge( | ||
'w-full overflow-hidden rounded-lg pb-8 text-white sm:max-w-[600px]', | ||
'bg-gray-1 rounded-md border border-white/30 px-6 py-7 pb-8 text-white w-full sm:max-w-[600px] min-h-[600px] flex flex-col', |
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.
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.
And I think same with the tile background colors. I feel they should be translucent so that they derive their colors from background's theme.
Else we have a primary button theme as well but I dont think that should be used here since it would conflict with the icons' themes.
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.
thanks for flagging, this wasn't considered in the design at all, so i'll relay that to design team and let them figure it out and apply the changes when they provide them
className={twMerge( | ||
'relative flex w-full flex-col items-center justify-center rounded-md bg-gray-9 p-4 overflow-hidden', | ||
)} | ||
onClick={() => {}} |
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.
Let's track all the onramp clicks in posthog.
return ( | ||
<div | ||
className={twMerge( | ||
'relative bg-gray-1 rounded-md border border-gray-8 px-5 pt-14 pb-6 text-white -mx-4 md:mx-0 w-[calc(100%_+_40px)] md:w-full', |
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.
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.
flagged to design
Summary
Steps to test