Skip to content

Add support for displaying custom token address#154

Open
jcheese1 wants to merge 8 commits into
family:mainfrom
jcheese1:add_support_for_displaying_custom_token
Open

Add support for displaying custom token address#154
jcheese1 wants to merge 8 commits into
family:mainfrom
jcheese1:add_support_for_displaying_custom_token

Conversation

@jcheese1
Copy link
Copy Markdown

@jcheese1 jcheese1 commented Mar 8, 2023

CleanShot 2023-03-08 at 21 48 28@2x

CleanShot 2023-03-08 at 21 48 26@2x

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 8, 2023

@jcheese1 is attempting to deploy a commit to the LFE Team on Vercel.

A member of the Team first needs to authorize it.

@jcheese1
Copy link
Copy Markdown
Author

jcheese1 commented Mar 9, 2023

@lochie would be great if you can take a look at this PR. so close to having it on our app :D

Copy link
Copy Markdown
Member

@lochie lochie left a comment

Choose a reason for hiding this comment

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

This is a great feature! But I've left a few comments regarding some small changes.
Would also be great to see an example of implementation within the testbench so we can test the functionality before merging

Comment on lines -4 to -5
type Chain = { id: number; name: string; logo: ReactNode };
const supportedChains: Chain[] = [
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any reason why this type has been removed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

as const already infers it, so I thought its unnecessary

{nFormatter(Number(balance?.formatted))}
</span>
{!hideSymbol && ` ${balance?.symbol}`}
{!hideSymbol || customTokenAddress ? ` ${balance?.symbol}` : null}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't think these || customTokenAddress checks should be here, nothing should override the hide options.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

AFAIK !hideSymbol is not configurable on the consumer side yet? Please correct me if I'm wrong :D

Comment thread packages/connectkit/src/components/ConnectKit.tsx Outdated
Comment thread packages/connectkit/src/components/Common/ChainSelectList/index.tsx Outdated
@jcheese1 jcheese1 requested a review from lochie March 10, 2023 00:43
@jcheese1
Copy link
Copy Markdown
Author

@lochie added an example on testbench

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 10, 2023

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

Name Status Preview Comments Updated
connectkit-testbench ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 1:55AM (UTC)

@bczak
Copy link
Copy Markdown

bczak commented Apr 28, 2023

any update?

@jcheese1
Copy link
Copy Markdown
Author

@lochie would be great if we can get this in too :D

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants