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: network box redesign #1893

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

feat: network box redesign #1893

wants to merge 22 commits into from

Conversation

brtkx
Copy link
Contributor

@brtkx brtkx commented Sep 13, 2024

closes FS-1012

@cla-bot cla-bot bot added the cla-signed label Sep 13, 2024
Copy link

vercel bot commented Sep 13, 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 Dec 12, 2024 2:57pm

@brtkx brtkx marked this pull request as ready for review September 20, 2024 14:11
@@ -11,20 +11,25 @@ export function SafeImage(props: SafeImageProps) {
const [validImageSrc, setValidImageSrc] = useState<false | string>(false)

useEffect(() => {
let mounted = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to track this, otherwise there are side effects when changing networks and token logos not updating

@brtkx brtkx marked this pull request as ready for review December 9, 2024 14:24
Comment on lines +48 to +62
const tokenLogoSrc = useMemo(() => {
if (parentErc20Address) {
return (
tokensFromLists[parentErc20Address]?.logoURI ??
tokensFromUser[parentErc20Address]?.logoURI
)
}

return nativeCurrency.logoUrl
}, [
nativeCurrency.logoUrl,
parentErc20Address,
tokensFromLists,
tokensFromUser
])
Copy link
Member

Choose a reason for hiding this comment

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

same code as in TokenButton.tsx, can you reuse them please?

})
: undefined
}
symbolOverride={isCctpTransfer ? 'USDC.e' : undefined}
Copy link
Member

Choose a reason for hiding this comment

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

if it's CCTP, and it's withdrawal mode, the destination chain's (parent chain) symbol should be USDC, because the source chain (child chain) USDC would be the native USDC.

Comment on lines +64 to +83
const symbol = useMemo(() => {
if (symbolOverride) {
return symbolOverride
}

if (parentErc20Address) {
return (
tokensFromLists[parentErc20Address]?.symbol ??
tokensFromUser[parentErc20Address]?.symbol
)
}

return nativeCurrency.symbol
}, [
symbolOverride,
nativeCurrency.symbol,
parentErc20Address,
tokensFromLists,
tokensFromUser
])
Copy link
Member

Choose a reason for hiding this comment

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

this is not working for Arb One/Sepolia native USDC -> Orbit chain

note the empty token symbol

can you check if you need the usage of sanitizeTokenSymbol because it handles all these cases and was the reason why we had it in the first place

image

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.

2 participants