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

More wallet hooks: useWalletStatus & useWalletSupport #1646

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Nov 24, 2024

Description

I noticed this

It's also going to be tempting to start making useWallets a monolith again, but that's part of what made the code so buggy. Meaning, it's going to be convenient to push all code into useWallets for anything that uses useWallets but it's going to be inconvenient to maintain.

-- #1643 (comment)

also applies to what I did in #1559. This PR removes that code from useWallets and moves it into their own wallet hooks.

Based on #1638, #1640

Additional Context

useWalletSupport is a bit of a ridiculous "hook" but I think it's indeed better to have it separated so we can use it only where we need it (which is only inside the WalletCard component).

Checklist

Are your changes backwards compatible? Please answer below:

yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:

  • 7. Wallet cards still show correct status and support.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:

n/a

Did you introduce any new environment variables? If so, call them out explicitly here:

no


export default function WalletCard ({ wallet, draggable, onDragStart, onDragEnter, onDragEnd, onTouchStart, sourceIndex, targetIndex, index }) {
const image = useWalletImage(wallet)
const status = useWalletStatus(wallet)
const support = useWalletSupport(wallet)
Copy link
Member

Choose a reason for hiding this comment

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

Oh boy that's like a breathe of fresh air.

Choose a reason for hiding this comment

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

Very good

@ekzyis
Copy link
Member Author

ekzyis commented Nov 24, 2024

Keeping this as a draft until #1638 is merged and I rebased this again on master

@ekzyis ekzyis marked this pull request as ready for review November 24, 2024 00:50
@huumn huumn merged commit b0207a2 into master Nov 24, 2024
6 checks passed
@huumn huumn deleted the more-wallet-hooks branch November 24, 2024 02:19
@ekzyis ekzyis mentioned this pull request Nov 25, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants