feat: Native Send Component #1874
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
brendan-defi
force-pushed
the
bf/ock-send
branch
from
bf2eb1f to
bb6562d
Compare
brendan-defi
force-pushed
the
bf/ock-send
branch
from
342bbfb to
95ee15c
Compare
brendan-defi
force-pushed
the
bf/ock-send
branch
from
95ee15c to
43a0f8a
Compare
| </div> | ||
| <div className="text-right"> | ||
| {showAction ? ( | ||
| <span |
There was a problem hiding this comment.
Why <span > here? Probably should be a button if it's clickable
There was a problem hiding this comment.
can't have a button within a button, and sometimes the whole container is a button
| 'overflow-hidden text-ellipsis whitespace-nowrap text-left', | ||
| )} | ||
| > | ||
| {token.name.trim()} |
There was a problem hiding this comment.
Curious what the need for trim() is here?
There was a problem hiding this comment.
some tokens have whitespace at the front / end for style /s. we don't want that
| context, | ||
| }); | ||
|
|
||
| const activeStep = useMemo(() => { |
There was a problem hiding this comment.
I see what's going on here but it's quite hard to reason about having not built it myself.
I think the easiest thing would probably be breaking this into internal subcomponents for each step (we'd have to think about the right names, not sure about that). What do you think about that?
There was a problem hiding this comment.
e.g. it'd be easier to reason about that you're rendering the recipient search/options if this was in a component:
<>
<SendAddressInput
selectedRecipientAddress={context.selectedRecipientAddress}
recipientInput={context.recipientInput}
setRecipientInput={context.setRecipientInput}
handleRecipientInputChange={context.handleRecipientInputChange}
/>
{context.validatedRecipientAddress && (
<SendAddressSelector
address={context.validatedRecipientAddress}
senderChain={context.senderChain}
handleAddressSelection={context.handleAddressSelection}
/>
)}
</>
There was a problem hiding this comment.
yeah this is exactly the kind of thing i was hoping to get your thoughts on. i can see why this would be confusing.
is the suggestion to put all the conditionals into the sub-components, so it looks like (not real component names):
if (!isInitialized) {
return null;
}
return (
<FundWallet />
<AddressSelection />
<TokenSelection />
<AmountInputAndConfirmation />
)
or keep the conditionals here, but just pull all the return values into sub-components?
| import { Address, Avatar, Identity, Name } from '@/identity'; | ||
| import { background, border, cn, pressable } from '@/styles/theme'; | ||
| import { useCallback } from 'react'; | ||
| import type { Address as AddressType, Chain } from 'viem'; |
There was a problem hiding this comment.
Would avoid the rename if possible
There was a problem hiding this comment.
don't think it's possible right? I need the Address component from identity
| handleAddressSelection: (address: AddressType) => void; | ||
| }; | ||
|
|
||
| export function SendAddressSelector({ |
There was a problem hiding this comment.
How do you feel about SendAddressOption?
There was a problem hiding this comment.
as teh component name? i like Selector better because it signals that the user needs to interact with it
|
|
||
| export function SendAddressSelector({ | ||
| address, | ||
| senderChain, |
There was a problem hiding this comment.
I'd probably drop the senderChain here and get it from context (or global context) since it doesn't seem like something you're going to want to pass to each option, even if you use subcomponents?
There was a problem hiding this comment.
right now i'm taking a props-maxi approach, almost entirely avoiding using context in the subcomponents. i think this is better devex because it enables easier customization. alternative would be to export the provider. not sure what's better
| import { cn, color, text } from '@/styles/theme'; | ||
| import { useSendContext } from '@/wallet/components/WalletAdvancedSend/components/SendProvider'; | ||
|
|
||
| export function SendAmountInput({ |
There was a problem hiding this comment.
Can you move this to the bottom so it's easier to read when scanning? (e.g. exports at bottom)
There was a problem hiding this comment.
like just function declaration and then at the bottom do the exports?
tbh i find that way much more confusing because i like to know immediately whether the thing i'm looking at is "part of the reason for the file" or "just a helper in the file", but not a hill i'll die on
|
|
||
| export function SendButton({ | ||
| label = 'Continue', | ||
| senderChain, |
There was a problem hiding this comment.
I'd drop chain here as well
| errorOverride, | ||
| cryptoAmount, | ||
| selectedToken, | ||
| callData, |
There was a problem hiding this comment.
I'd drop and get from context - otherwise people should just use <Transaction />
| 'disabled' | 'pendingOverride' | 'successOverride' | 'errorOverride' | ||
| >; | ||
|
|
||
| export function SendButton({ |
There was a problem hiding this comment.
Could you move to bottom so it's easier to find the export?
|
|
||
| const defaultSuccessHandler = useCallback( | ||
| (receipt: TransactionReceipt | undefined) => { | ||
| // SW will have txn id so open in wallet |
There was a problem hiding this comment.
Had a bit of a hard time understanding this context
// SW will have txn id so open in wallet
There was a problem hiding this comment.
sorry, i just copied directly from Transaction.
smart wallet transactions don't have the same structure as EOAs. so the way we link to them is different
| useExchangeRate({ | ||
| token: selectedToken, | ||
| selectedInputType, | ||
| setExchangeRate, | ||
| setExchangeRateLoading, | ||
| }); |
There was a problem hiding this comment.
Where does this get set?
There was a problem hiding this comment.
useExchangeRate sets the exchange rate value.
exchangeRate is read by SendAmountInput
| ethBalance: number | undefined; | ||
| tokenBalances: PortfolioTokenWithFiatValue[] | undefined; | ||
|
|
||
| // Recipient Address Context |
There was a problem hiding this comment.
I like the comments, they're helpful
What changed? Why?
Notes to reviewers
How has it been tested?