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

Add support for displaying custom token address #154

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jcheese1
Copy link

@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

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
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
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
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
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

<span style={{ minWidth: 32 }}>
{nFormatter(Number(balance?.formatted))}
</span>
{!hideSymbol && ` ${balance?.symbol}`}
{!hideSymbol || customTokenAddress ? ` ${balance?.symbol}` : null}
Copy link
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
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

packages/connectkit/src/components/ConnectKit.tsx Outdated Show resolved Hide resolved
@jcheese1 jcheese1 requested a review from lochie March 10, 2023 00:43
@jcheese1
Copy link
Author

@lochie added an example on testbench

@vercel
Copy link

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

bczak commented Apr 28, 2023

any update?

@jcheese1
Copy link
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