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

refactor(WalletTable): migrate to Chakra UI #10032

Conversation

TylerAPfledderer
Copy link
Contributor

Description

Migrates the WalletTable component to Chakra UI. This PR also...

  • Abstracts the logic to a useHook
  • Pulls the WalletMoreInfo component to its own file
    • Also splits the structure for each category into a reusable component

Related Issue

Closes #9982

Part of #8632

@gatsby-cloud
Copy link

gatsby-cloud bot commented Apr 22, 2023

✅ ethereum-org-website-dev deploy preview ready

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Looks great @TylerAPfledderer. Just found one tiny bug on mobile, gj. LGTM

src/components/FindWallet/WalletTable/index.tsx Outdated Show resolved Hide resolved
src/components/FindWallet/WalletTable/index.tsx Outdated Show resolved Hide resolved
src/components/FindWallet/WalletTable/index.tsx Outdated Show resolved Hide resolved
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

@TylerAPfledderer looking at this again, I see that we are only showing 1 column between md and lg. I think there is still space to display the 3 columns on md.

We should show 1 column below the md breakpoint.

Now:
image

Before:
image

@TylerAPfledderer
Copy link
Contributor Author

TylerAPfledderer commented May 12, 2023

@pettinarip hmmm I see it in the deploy, and thought I would call out a Chakra bug with hideBelow style prop being used, but the issue isn't showing locally. And I cleaned the cache.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip could you update this PR to get to the latest version of the 'develop' branch? I don't think it up-to-date with a bump of the Chakra version that should solve this.

@TylerAPfledderer TylerAPfledderer force-pushed the refactor/migrate-wallet-table-to-chakra branch from c25905b to c88e283 Compare May 12, 2023 21:44
@pettinarip
Copy link
Member

@pettinarip could you update this PR to get to the latest version of the 'develop' branch? I don't think it up-to-date with a bump of the Chakra version that should solve this.

Done!

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Great @TylerAPfledderer. I see that the last issue was fixed after updating the chakra version 👍🏼

@pettinarip pettinarip merged commit d25fa57 into ethereum:dev May 15, 2023
@TylerAPfledderer TylerAPfledderer deleted the refactor/migrate-wallet-table-to-chakra branch May 16, 2023 12:45
@corwintines corwintines mentioned this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate WalletTable.tsx to Chakra
2 participants