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: Connect components in React Native [part 2] #3438

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

joaquim-verges
Copy link
Member

@joaquim-verges joaquim-verges commented Jun 21, 2024

Connected button and modal implementation in RN


PR-Codex overview

The focus of this PR is to refactor and optimize the codebase by moving and updating imports, modifying function parameters, and improving file organization.

Detailed summary

  • Updated wallets property in useAutoConnect.ts
  • Moved TokenInfo import in various files
  • Updated import paths for socialIcons
  • Refactored useSendToken in web and native
  • Updated defaultTokens import paths

The following files were skipped due to too many changes: packages/thirdweb/src/react/core/hooks/connection/ConnectButtonProps.ts, packages/thirdweb/src/react/web/ui/ConnectWallet/screens/TokenSelector.tsx, packages/thirdweb/src/react/web/ui/ConnectWallet/screens/ViewFunds.tsx, packages/thirdweb/src/react/web/ui/components/WalletImage.tsx, packages/thirdweb/src/react/web/ui/ConnectWallet/screens/Buy/swap/PayWithCrypto.tsx, packages/thirdweb/src/react/web/ui/ConnectWallet/ConnectButton.tsx, packages/thirdweb/src/react/native/ui/connect/ErrorView.tsx, packages/thirdweb/src/react/native/ui/connect/SuccessView.tsx, packages/thirdweb/src/react/web/ui/ConnectWallet/screens/Buy/BuyScreen.tsx, packages/thirdweb/src/react/native/ui/components/ChainIcon.tsx, packages/react-native-adapter/README.md, packages/thirdweb/src/react/web/wallets/shared/ConnectWalletSocialOptions.tsx, packages/thirdweb/src/react/web/ui/ConnectWallet/Details.tsx, packages/thirdweb/src/react/web/hooks/wallets/useAutoConnect.ts, packages/thirdweb/src/react/native/ui/components/RNImage.tsx, packages/thirdweb/src/react/native/ui/components/Address.tsx, packages/thirdweb/src/react/native/ui/components/input.tsx, packages/thirdweb/src/react/native/ui/connect/InAppWalletUI.tsx, packages/thirdweb/src/react/native/ui/components/Header.tsx, packages/thirdweb/src/react/web/ui/ConnectWallet/defaultTokens.ts, packages/thirdweb/src/react/native/ui/connect/ExternalWalletsList.tsx, packages/thirdweb/src/react/native/ui/components/WalletImage.tsx, packages/thirdweb/src/react/native/ui/connect/ReceiveScreen.tsx, packages/thirdweb/src/react/core/hooks/wallets/useSendToken.ts, packages/thirdweb/src/react/web/ui/ConnectWallet/screens/SendFunds.tsx, packages/thirdweb/package.json, packages/thirdweb/src/react/web/ui/components/ChainIcon.tsx, packages/thirdweb/src/react/native/ui/connect/TokenListScreen.tsx, packages/thirdweb/src/react/native/ui/connect/ConnectButton.tsx, packages/thirdweb/src/react/native/ui/connect/ConnectedButton.tsx, packages/thirdweb/src/react/native/ui/connect/SendScreen.tsx, packages/thirdweb/src/react/native/ui/connect/ConnectModal.tsx, packages/thirdweb/src/react/native/ui/icons/svgs.ts, packages/thirdweb/src/react/native/ui/connect/ConnectedModal.tsx, packages/thirdweb/src/react/web/ui/ConnectWallet/icons/dataUris.ts, pnpm-lock.yaml

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented Jun 21, 2024

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

Name Status Preview Comments Updated (UTC)
thirdweb_playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 1, 2024 8:58am
thirdweb-www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 1, 2024 8:58am
wallet-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 1, 2024 8:58am

Copy link

changeset-bot bot commented Jun 21, 2024

⚠️ No Changeset found

Latest commit: cd436b3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @joaquim-verges and the rest of your teammates on Graphite Graphite

@github-actions github-actions bot added the TS SDK Involves changes to the v5 TypeScript SDK. label Jun 21, 2024
@joaquim-verges joaquim-verges changed the title joaquim/rn-connect-ui-2 feat: Connect Jun 21, 2024
Copy link

graphite-app bot commented Jun 21, 2024

Your org requires the Graphite merge queue for merging into main

Add the label “merge-queue” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

@joaquim-verges joaquim-verges changed the title feat: Connect feat: Connect components in React Native [part 2] Jun 21, 2024
@joaquim-verges joaquim-verges added the DO NOT MERGE This pull request is still in progress and is not ready to be merged. label Jun 21, 2024 — with Graphite App
@joaquim-verges joaquim-verges marked this pull request as ready for review June 21, 2024 14:33
Copy link

codspeed-hq bot commented Jun 21, 2024

CodSpeed Performance Report

Merging #3438 will improve performances by 10.39%

Comparing 06-21-joaquim_rn-connect-ui-2 (cd436b3) with joaquim/rn-ui-connect (20fd0b0)

Summary

⚡ 1 improvements
✅ 8 untouched benchmarks

Benchmarks breakdown

Benchmark joaquim/rn-ui-connect 06-21-joaquim_rn-connect-ui-2 Change
encode tx (prepared method) 379.6 µs 343.9 µs +10.39%

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 84 lines in your changes missing coverage. Please review.

Project coverage is 62.52%. Comparing base (20fd0b0) to head (cd436b3).

Additional details and impacted files
@@                    Coverage Diff                    @@
##           joaquim/rn-ui-connect    #3438      +/-   ##
=========================================================
+ Coverage                  62.27%   62.52%   +0.24%     
=========================================================
  Files                        900      898       -2     
  Lines                      69113    68903     -210     
  Branches                    3675     3687      +12     
=========================================================
+ Hits                       43040    43080      +40     
+ Misses                     25382    25134     -248     
+ Partials                     691      689       -2     
Flag Coverage Δ *Carryforward flag
legacy_packages 65.59% <ø> (ø) Carriedforward from 20fd0b0
packages 61.94% <62.50%> (+0.29%) ⬆️

*This pull request uses carry forward flags. Click here to find out more.

Files Coverage Δ
...react/core/hooks/transaction/useSendTransaction.ts 46.81% <100.00%> (ø)
...web/src/react/core/hooks/wallets/useAutoConnect.ts 61.20% <100.00%> (ø)
...ges/thirdweb/src/react/core/utils/defaultTokens.ts 100.00% <100.00%> (ø)
...ges/thirdweb/src/react/core/utils/isSmartWallet.ts 90.00% <ø> (ø)
...kages/thirdweb/src/react/core/utils/socialIcons.ts 100.00% <100.00%> (ø)
packages/thirdweb/src/react/core/utils/wallet.ts 28.71% <100.00%> (+2.18%) ⬆️
...react/web/hooks/transaction/useSendTransaction.tsx 32.03% <100.00%> (ø)
...b/src/react/web/ui/ConnectWallet/ConnectButton.tsx 56.54% <100.00%> (ø)
...hirdweb/src/react/web/ui/ConnectWallet/Details.tsx 26.85% <100.00%> (ø)
.../src/react/web/ui/ConnectWallet/WalletSelector.tsx 19.91% <100.00%> (ø)
... and 17 more

... and 6 files with indirect coverage changes

Copy link
Contributor

github-actions bot commented Jun 27, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
thirdweb (esm) 40.48 KB (0%) 810 ms (0%) 826 ms (+149.93% 🔺) 1.7 s
thirdweb (cjs) 90.48 KB (0%) 1.9 s (0%) 850 ms (+24.62% 🔺) 2.7 s
thirdweb (minimal + tree-shaking) 4.79 KB (0%) 96 ms (0%) 23 ms (-52.33% 🔽) 119 ms
thirdweb/chains (tree-shaking) 423 B (0%) 10 ms (0%) 86 ms (+275.98% 🔺) 96 ms
thirdweb/react (minimal + tree-shaking) 13.55 KB (0%) 271 ms (0%) 54 ms (+15.43% 🔺) 325 ms

Copy link

socket-security bot commented Jul 1, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

Comment on lines +19 to +25
useEffect(() => {
if (copySuccess) {
setTimeout(() => {
setCopySuccess(false);
}, 2500);
}
}, [copySuccess]);
Copy link
Member

Choose a reason for hiding this comment

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

have to clean up this timeout if it was set in the cleanup function

<TouchableOpacity
style={styles.addressContainer}
onPress={() => {
// @ts-ignore - missing native types
Copy link
Member

Choose a reason for hiding this comment

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

prefer @ts-expect-error

Suggested change
// @ts-ignore - missing native types
// @ts-expect-error - missing native types

Comment on lines +31 to +33
if (ensAvatar) {
return ensAvatar;
}
Copy link
Member

Choose a reason for hiding this comment

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

just keep this entirely outside of the useQuery, it doesn't need to be in here

"wallet-image-or-ens",
wallet.id,
wallet.getAccount()?.address,
ensAvatar,
Copy link
Member

Choose a reason for hiding this comment

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

remove

{balanceQuery.data
? Number(balanceQuery.data.displayValue).toFixed(3)
: "---"}{" "}
{balanceQuery.data?.symbol}{" "}
Copy link
Member

Choose a reason for hiding this comment

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

why the ending space? {" "}

>
{balanceQuery.data
? Number(balanceQuery.data.displayValue).toFixed(3)
: "---"}{" "}
Copy link
Member

Choose a reason for hiding this comment

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

is the --- the "loading" state?

const tokens = chain ? supportedTokens[chain.id] || [] : [];

return (
<>
Copy link
Member

Choose a reason for hiding this comment

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

empty fragment?

@joaquim-verges joaquim-verges merged commit 3590a41 into joaquim/rn-ui-connect Jul 1, 2024
29 of 30 checks passed
@joaquim-verges joaquim-verges deleted the 06-21-joaquim_rn-connect-ui-2 branch July 1, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE This pull request is still in progress and is not ready to be merged. TS SDK Involves changes to the v5 TypeScript SDK.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants