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: wallet island #1793

Merged
merged 207 commits into from
Jan 14, 2025
Merged

feat: wallet island #1793

merged 207 commits into from
Jan 14, 2025

Conversation

brendan-defi
Copy link
Contributor

What changed? Why?

Notes to reviewers

How has it been tested?

Copy link

vercel bot commented Jan 3, 2025

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

Name Status Preview Comments Updated (UTC)
onchainkit-coverage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 4:05am
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 4:05am
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 4:05am

className={icon.foreground}
/>
</svg>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: once we have a single arrow we should favor rotating it with css rather than including additional arrows

defaultAvatar
<div className={cn(border.default, 'h-full w-full border')}>
{defaultAvatar}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

adding a 1px border around defaultAvatar means that avatars display at 40x40 and default avatars render at 38x38 (i.e. 40 - 1px top + 1px left)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curious if you have a better way to do this. Without the border, I get a 1px outline coming from the background:
image

I tried (and paired with some others to try) to fix without a border, but this was the best that we came up with

Copy link
Contributor

Choose a reason for hiding this comment

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

this is because of the way the svg path is drawn, you are seeing the bleed from the edge of the path (which is styled to be white), you may be able to fix this by separating into a path for the white part and a circle behind it in the inverse. you could apply the border hack with a transparent border to the users img but you'd still end up changing the size from 40 to 38 unless you adjust the sizes to a non-standard tailwind size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, okay, i will try to fix this up as a fast-follow. thanks!

@alessey
Copy link
Contributor

alessey commented Jan 14, 2025

I'm a little confused by these changes. Seems like we are just creating a bunch of *Default components. I thought the goal with this component was to be able to compose the version you wanted. With default children, this would give us:
<Wallet/> which would render our default implementation with the dropdown
<Wallet><ConnectWallet type="rect | circle"/><WalletIsland | WalletDropdown draggable={true|false} /></Wallet>
with further composability available if you wanted to customize the ConnectWallet or WalletDropdown further and in the future, the WalletIsland.

Additionally, drag seems like a much more useful feature (with a more straightforward implementation) if it only applied to the WalletIsland or WalletDropdown child vs. the entire component.

@brendan-defi brendan-defi dismissed stale reviews from dschlabach and alessey via 29dc53e January 14, 2025 15:53
@cpcramer
Copy link
Contributor

Can we center WalletAdvancedDefault and WalletIsland (vertically and horizontally) in the playground?

@cpcramer
Copy link
Contributor

I feel like we should hide tokens with a balance of $0.00 - I have a ton of them in my wallet and it feels like spam

Screenshot 2025-01-14 at 8 58 14 AM

@alessey
Copy link
Contributor

alessey commented Jan 14, 2025

I feel like we should hide tokens with a balance of $0.00 - I have a ton of them in my wallet and it feels like spam

Screenshot 2025-01-14 at 8 58 14 AM

just the fiat balance is < 0, in this case I think it'd make sense to show < $0.01 or something similar

@cpcramer
Copy link
Contributor

I feel like we should hide tokens with a balance of $0.00 - I have a ton of them in my wallet and it feels like spam
Screenshot 2025-01-14 at 8 58 14 AM

just the fiat balance is < 0, in this case I think it'd make sense to show < $0.01 or something similar

Ok and looks like this is what CB Wallet defaults to

@brendan-defi
Copy link
Contributor Author

I'm a little confused by these changes. Seems like we are just creating a bunch of *Default components. I thought the goal with this component was to be able to compose the version you wanted.

We ended up creating some defaults, but all of the underlying components are composable, so someone could definitely do

<Wallet draggable={true/false}> // whole thing can be draggable or not, works nicely either way
  <ConnectWallet>
    ...custom ConnectWallet children
  </ConnectWallet>
  <WalletAdvanced>
    ...custom WalletIsland children
  </WalletAdvanced>
</Wallet>

Additionally, drag seems like a much more useful feature (with a more straightforward implementation) if it only applied to the WalletIsland or WalletDropdown child vs. the entire component.

This is an implementation decision. A dev could do this:

<Wallet draggable={false}>
  <ConnectWallet>
    ...custom ConnectWallet children
  </ConnectWallet>
  <Draggable>
    <WalletAdvanced>
      ...custom WalletAdvanced children
    </WalletAdvanced>
  </Draggable>
</Wallet>

This would leave a ConnectWallet "anchor", with a draggable dropdown/advanced box.

@brendan-defi
Copy link
Contributor Author

Can we center WalletAdvancedDefault and WalletIsland (vertically and horizontally) in the playground?

No. WalletAdvancedDefault is tall enough that on normal laptop screens it overflows the bottom of the screen when vertically centered.

And WalletIsland is meant to be defaulted to bottom-left, so that's the playground implementation.

@brendan-defi
Copy link
Contributor Author

brendan-defi commented Jan 14, 2025

I feel like we should hide tokens with a balance of $0.00 - I have a ton of them in my wallet and it feels like spam

We are using the Coinbase Wallet spam filter, with the setting that Coinbase Wallet uses itself, so these tokens are below the "likely spam" threshold. And we hide tokens with a zero-crypto balance. But we chose to keep tokens with a crypto balance but no fiat balance because some tokens aren't super liquid on dexes and don't have fiat prices, but would still be valuable to be seen.

@brendan-defi brendan-defi merged commit bb80419 into main Jan 14, 2025
16 checks passed
@brendan-defi brendan-defi deleted the bf/wallet-island branch January 14, 2025 17:22
@brendan-defi brendan-defi restored the bf/wallet-island branch January 15, 2025 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants