-
Notifications
You must be signed in to change notification settings - Fork 158
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: add fund button #1322
feat: add fund button #1322
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@steveviselli-cb is attempting to deploy a commit to the Coinbase Team on Vercel. A member of the Team first needs to authorize it. |
@@ -28,7 +28,7 @@ export const pressable = { | |||
secondary: | |||
'cursor-pointer bg-ock-secondary active:bg-ock-secondary-active hover:bg-[var(--bg-ock-secondary-hover)]', | |||
shadow: 'shadow-ock-default', | |||
disabled: 'opacity-[0.38]', | |||
disabled: 'opacity-[0.38] pointer-events-none', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removes hover effects for disabled buttons. Confirmed with @mindapivessa.
import type { WalletDropdownFundLinkReact } from '../types'; | ||
import { WalletDropdownFundLinkButton } from './WalletDropdownFundLinkButton'; | ||
|
||
export function WalletDropdownFundLink({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -13,7 +13,7 @@ export function Spinner({ className }: SpinnerReact) { | |||
<div | |||
className={cn( | |||
'animate-spin border-2 border-gray-200 border-t-3', | |||
'rounded-full border-t-blue-500 px-2.5 py-2.5', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed with @mindapivessa that this was the wrong color.
Nit - can we increase the sidebar width so smart wallet doesn't become 2 lines? @steveviselli-cb |
@fakepixels that's just because I resized the browser window to be small for the video, it's not normally like that. |
const overrideClassName = cn( | ||
pressable.default, | ||
// Disable hover effects if there is no funding URL | ||
!fundingUrlToRender && 'pointer-events-none', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this change, https://github.com/coinbase/onchainkit/pull/1322/files#diff-639b39dc7c8fa5ec11491e030f9add3c94620cae4c45a8e0a9412103659e41dfR31, this shouldn't be necessary right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alessey na it is necessary. I don't use pressable.disabled
on the actual link because that would make the background color change like this:
So I disable the pointer events on the link to prevent the hover effect from pressable.default
which is on the link, and then apply pressable.disabled
to the span inside the link so that the text is dimmed to look disabled.
@steveviselli-cb will smart wallet be a part of a different PR? |
@mindapivessa nope smart wallet is in this one too. You can see it in the second video. |
What changed? Why?
<FundButton />
component integrated with Coinbase Onramp<FundButton />
to PlaygroundPlayground demo with ProjectId is correctly configured:
Onramp.mov
Playground demo with ProjectId is NOT configured:
No.Project.ID.mov
Disabled wallet dropdown button treatment:
Correct spinner color i.e. not blue
Notes to reviewers
How has it been tested?