Skip to content
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: new destination chain dropdown #1602

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

brtkx
Copy link
Contributor

@brtkx brtkx commented Mar 26, 2024

No description provided.

@cla-bot cla-bot bot added the cla-signed label Mar 26, 2024
Copy link

vercel bot commented Mar 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
arbitrum-token-bridge ✅ Ready (Inspect) Visit Preview Apr 17, 2024 1:59pm

return networksToShow
}

const groupedNetworks = []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should always have at least one core chain but just in case we only show groups if they have at least one chain

@@ -573,6 +625,14 @@ export function TransferPanelMain({

useUpdateUSDCTokenData()

type NetworkListboxProps = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from NetworkListbox.tsx which got removed.

Comment on lines +347 to +354
<SearchPanel.MainPage className="flex h-full max-w-[500px] flex-col py-4">
<NetworksPanel
chainIds={supportedChainIds}
selectedChainId={selectedChainId}
close={() => props.onClose(false)}
onNetworkRowClick={props.onChange}
/>
</SearchPanel.MainPage>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something odd with the width
there shouldn't be a horizontal scrollbar

image

onClick={onClick}
>
<span className="max-w-[220px] truncate text-sm leading-[1.1] md:max-w-[250px] md:text-xl">
From: {getNetworkName(selectedChainId)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image it's `To:` here hehe

Comment on lines +628 to +634
type NetworkListboxProps = {
disabled?: boolean
label: string
options: Chain[]
value: Chain
onChange: (value: Chain) => void
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be deleted because really we only need onChange for both from and to now

Comment on lines 637 to 638
from: Pick<NetworkListboxProps, 'onChange'>
to: Omit<NetworkListboxProps, 'label'>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would just do and delete the other props from to in the return object and getDestinationChains because they are called within NetworkSelectionContainer already

Suggested change
from: Pick<NetworkListboxProps, 'onChange'>
to: Omit<NetworkListboxProps, 'label'>
from: {
onChange: (value: Chain) => void
}
to: {
onChange: (value: Chain) => void
}

@@ -429,6 +480,7 @@ export function TransferPanelMain({
}
}, [nativeCurrency, ethL1Balance, ethL2Balance, erc20L1Balances])

const [isTestnetMode] = useIsTestnetMode()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you shouldn't need this

@@ -66,12 +69,53 @@ import {
Balances,
useSelectedTokenBalances
} from '../../hooks/TransferPanel/useSelectedTokenBalances'
import { useIsTestnetMode } from '../../hooks/useIsTestnetMode'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you shouldn't need this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants