feat: deposit pending and success screens #1481
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tinaszheng
force-pushed
the
tina/pending-screens
branch
from
4f3f74d to
c50ca43
Compare
| @@ -38,14 +36,15 @@ export const LoadingSpinner: React.FC<LoadingSpinnerProps> = ({ | |||
| cx="19" | |||
| cy="19" | |||
| r="16" | |||
| stroke={stroke} | |||
| stroke="var(--color-accent)" | |||
There was a problem hiding this comment.
this updates for all loading spinners but this is the new design ANYWAY
|
|
||
| // TODO(deposit2.0): localization for this whole component | ||
| export const DepositStatus = ({ txHash, chainId, onClose }: DepositStatusProps) => { | ||
| const deposit = useAppSelector((store) => getDeposit(store, txHash, chainId)); |
There was a problem hiding this comment.
getDeposit isn't a selector so it's running every time anything in the state updates and doing the .find and .flat which could get slow in theory. Should probably be a parameterized selector.
src/state/transfersSelectors.ts
Outdated
| /** | ||
| * @returns saved chartConfig for TradingView | ||
| */ | ||
| export const getPendingDeposits = (state: RootState, dydxAddress?: DydxAddress): Deposit[] => { |
There was a problem hiding this comment.
probably should also be a real selector
src/state/transfers.ts
Outdated
| type Transfer = Deposit | Withdraw; | ||
|
|
||
| export interface TransferState { | ||
| accountToTransfers: { [account: DydxAddress]: Transfer[] }; |
There was a problem hiding this comment.
kinda mildly prefer transfersByTargetAddress
There was a problem hiding this comment.
oh i like. transfersByDydxAddress perhaps. because target address can be your evm address for withdrawals but we'd want to group by dydx account
|
|
||
| return { | ||
| ...state, | ||
| transfers: {}, |
There was a problem hiding this comment.
I don't think this is necessary since there's some logic jared or you added somewhere that will make a thing initialState if it's undefined when the app starts up / after migrations maybe possibly probably
There was a problem hiding this comment.
we've seen it in the past for whatever reason, better to be safe/explicit i guess /shrug
| }, [dydxAddress, pendingDeposits, skipClient, dispatch]); | ||
| } | ||
|
|
||
| function handleResponseStatus(status: StatusState) { |
There was a problem hiding this comment.
is typescript inferring the return type correctly? Could use LoadableStatus
| useEffect(() => { | ||
| if (!dydxAddress || !pendingDeposits.length) return; | ||
|
|
||
| for (let i = 0; i < pendingDeposits.length; i += 1) { |
There was a problem hiding this comment.
could this be a .map or .forEach instead ?
There was a problem hiding this comment.
dont be scared of for loops tyler!
|
|
||
| return transfersByAddress[dydxAddress].filter( | ||
| (transfer) => transfer.type === 'deposit' && transfer.status === 'pending' | ||
| ) as Deposit[]; |
There was a problem hiding this comment.
is there a way to do this without the cast? Not sure if we could utilize a transfer is Deposit filter function :/
There was a problem hiding this comment.
that works for individual transfer items but not for the filtered array itself :/ i like the idea of adding some explicit type guard functions tho
src/state/transfersSelectors.ts
Outdated
| ); | ||
|
|
||
| const selectAllTransfers = createAppSelector( | ||
| [(state: RootState) => state.transfers.transfersByDydxAddress], |
There was a problem hiding this comment.
can replace with getTransfersByAddress?
| </div> | ||
| <Button | ||
| onClick={onClose} | ||
| type={ButtonType.Submit} |
There was a problem hiding this comment.
I don't think Submit is necessary
This PR adds:
Up next (not in this PR):
sweepaction to complete, and that users will see success at the same time their account value changes.