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: add earn balance cards #1873

Merged
merged 9 commits into from
Jan 24, 2025
Merged

feat: add earn balance cards #1873

merged 9 commits into from
Jan 24, 2025

Conversation

abcrane123
Copy link
Contributor

@abcrane123 abcrane123 commented Jan 24, 2025

What changed? Why?

Screenshot 2025-01-24 at 9 43 48 AM

Screenshot 2025-01-24 at 9 45 05 AM

Notes to reviewers

How has it been tested?

Copy link

vercel bot commented Jan 24, 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 24, 2025 8:30pm
onchainkit-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 24, 2025 8:30pm
onchainkit-routes ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 24, 2025 8:30pm

@abcrane123 abcrane123 marked this pull request as ready for review January 24, 2025 18:11
Comment on lines 12 to 20
const handleKeyDown = useCallback(
(e: React.KeyboardEvent) => {
if (e.key === 'Enter') {
e.preventDefault();
onActionPress();
}
},
[onActionPress],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't the browser just handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it should the linter got mad let me double check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

const [depositAmount, setDepositAmount] = useState('');
const [withdrawAmount, setWithdrawAmount] = useState('');

const { convertedBalance } = useGetTokenBalance(address, usdcToken);
Copy link
Contributor

Choose a reason for hiding this comment

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

May be fine for testing but will probably have to make this more generic given we will try and support all vaults not just USDC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah lets leave as is and revisit when we're hooking up the actions

export function DepositBalance({ className }: DepositBalanceReact) {
const { convertedBalance, setDepositAmount } = useEarnContext();

const handleUseMaxPress = useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove use since this is not a hook? Also, what is a press? Thoughts on something like handleMaxAmount

Copy link
Contributor Author

@abcrane123 abcrane123 Jan 24, 2025

Choose a reason for hiding this comment

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

oh yeah use max is the text of the button so i thought it made sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohhh I see the buttons called "Use Max"

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a question for Mind, but I feel like the button should just say "Max" - I think this is what most DEX's do

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2025-01-24 at 10 55 10 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

I was a little confused by that too, so I'd probably advocate for handleMaxAmount. Use is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good i'll update the function name and check with mind on the text

cpcramer
cpcramer previously approved these changes Jan 24, 2025
@abcrane123 abcrane123 merged commit 240b802 into main Jan 24, 2025
16 checks passed
@abcrane123 abcrane123 deleted the alissa.crane/earn-balances branch January 24, 2025 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants