[SDK] Add onClose callback to details modal #5605

kien-ngo · 2024-12-04T11:07:25Z

PR-Codex overview

This PR focuses on enhancing the ConnectButton component by adding an onClose callback for the details modal and improving the ChainIcon functionality, including test coverage and icon resolution.

Detailed summary

Added onClose callback to ConnectButton details modal options.
Improved fetchChainIcon logic for better icon resolution.
Enhanced test coverage for ChainIcon.
Created ChainActiveDot component for active chain indication.
Updated DetailsModal to utilize new callback and components.

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

vercel · 2024-12-04T11:07:28Z

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

Name	Status	Preview	Comments	Updated (UTC)
docs-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 14, 2024 11:43am
thirdweb_playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 14, 2024 11:43am
thirdweb-www	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 14, 2024 11:43am
wallet-ui	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 14, 2024 11:43am

changeset-bot · 2024-12-04T11:07:29Z

🦋 Changeset detected

Latest commit: 99cc2f5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
thirdweb	Patch
@thirdweb-dev/wagmi-adapter	Patch

Not sure what this means? Click here to learn what changesets are.

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

graphite-app · 2024-12-04T11:07:31Z

Your org has enabled 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.

kien-ngo · 2024-12-04T11:07:41Z

[SDK] Add onClose callback to details modal #5605 👈 (View in Graphite)
main

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

merge-queue - adds this PR to the back of the merge queue
hotfix - for urgent hot fixes, skip the queue and merge this PR next

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

_{An organization admin has enabled the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

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

github-actions · 2024-12-04T11:11:12Z

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
thirdweb (esm)	44.94 KB (0%)	899 ms (0%)	2.1 s (+18.99% 🔺)	3 s
thirdweb (cjs)	110.78 KB (0%)	2.3 s (0%)	3.8 s (-20.27% 🔽)	6.1 s
thirdweb (minimal + tree-shaking)	5.58 KB (0%)	112 ms (0%)	265 ms (-23.11% 🔽)	376 ms
thirdweb/chains (tree-shaking)	506 B (0%)	10 ms (0%)	27 ms (-49.53% 🔽)	37 ms
thirdweb/react (minimal + tree-shaking)	19.03 KB (0%)	381 ms (0%)	725 ms (+91.97% 🔺)	1.2 s

codecov · 2024-12-04T11:12:51Z

Codecov Report

Attention: Patch coverage is 24.10714% with 170 lines in your changes missing coverage. Please review.

Project coverage is 51.41%. Comparing base (ce0da51) to head (bbb3ab1).
Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
...hirdweb/src/react/web/ui/ConnectWallet/Details.tsx	6.19%	106 Missing ⚠️
...src/react/web/ui/ConnectWallet/NetworkSelector.tsx	0.00%	59 Missing ⚠️
.../thirdweb/src/react/web/ui/prebuilt/Chain/icon.tsx	89.47%	4 Missing ⚠️
...thirdweb/src/react/web/ui/components/ChainIcon.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5605      +/-   ##
==========================================
- Coverage   51.53%   51.41%   -0.13%     
==========================================
  Files        1094     1097       +3     
  Lines       57592    57716     +124     
  Branches     4755     4758       +3     
==========================================
- Hits        29681    29674       -7     
- Misses      27193    27327     +134     
+ Partials      718      715       -3

Flag	Coverage Δ		*Carryforward flag
legacy_packages	`65.68% <ø> (ø)`		Carriedforward from ce0da51
packages	`48.09% <24.10%> (-0.15%)`	⬇️

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

Files with missing lines	Coverage Δ
...b/src/react/web/ui/ConnectWallet/ConnectButton.tsx	`49.01% <ø> (ø)`
...web/src/react/web/ui/components/ChainActiveDot.tsx	`100.00% <100.00%> (ø)`
...b/src/react/web/ui/components/fallbackChainIcon.ts	`100.00% <100.00%> (ø)`
...thirdweb/src/react/web/ui/components/ChainIcon.tsx	`8.57% <0.00%> (-23.35%)`	⬇️
.../thirdweb/src/react/web/ui/prebuilt/Chain/icon.tsx	`61.01% <89.47%> (ø)`
...src/react/web/ui/ConnectWallet/NetworkSelector.tsx	`3.99% <0.00%> (-0.17%)`	⬇️
...hirdweb/src/react/web/ui/ConnectWallet/Details.tsx	`48.49% <6.19%> (-5.37%)`	⬇️

... and 1 file with indirect coverage changes

packages/thirdweb/src/react/web/ui/ConnectWallet/Details.tsx

gregfromstl

LGTM but I'm a little confused how this solves the original issue, also why we need the screen param to be passed to onClose (feels like something a little too internal to be exposing). I trust though

packages/thirdweb/src/react/web/ui/ConnectWallet/Details.tsx

packages/thirdweb/src/react/web/ui/prebuilt/Chain/icon.test.tsx

gregfromstl · 2024-12-13T03:10:39Z

.changeset/metal-mails-ring.md

+```tsx
+<ConnectButton
+  detailsModal={{
+    onClose: (screen: string) => {


Rather than burying this in the detailsModal prop, could we make it top level?

gregfromstl

Remove screen param at least

packages/thirdweb/src/utils/fetch.ts

graphite-app · 2024-12-14T11:37:12Z

packages/thirdweb/src/react/web/ui/components/ChainIcon.tsx

+/**
+ * @internal
+ */
+export const getSrcChainIcon = (props: {


Remove 'export' keyword since getSrcChainIcon is only used internally within this file and not imported elsewhere

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2024-12-14T11:40:35Z

packages/thirdweb/src/react/web/ui/components/ChainIcon.test.tsx

@@ -0,0 +1,3 @@
+import { describe } from "vitest";
+
+describe.runIf(process.env.TW_SECRET_KEY)("ChainIcon-legacy", () => {});


Remove the empty legacy test suite since it's not being used and there are already comprehensive tests in place for the ChainIcon component

Spotted by Graphite Reviewer (based on CI logs)

Is this helpful? React 👍 or 👎 to let us know.

kien-ngo marked this pull request as ready for review December 4, 2024 11:07

kien-ngo requested review from joaquim-verges, gregfromstl, MananTank, jnsdls and edwardysun as code owners December 4, 2024 11:07

github-actions bot added packages SDK Involves changes to the thirdweb SDK labels Dec 4, 2024

vercel bot deployed to Preview – thirdweb_playground December 4, 2024 11:10 View deployment

vercel bot deployed to Preview – wallet-ui December 4, 2024 11:14 View deployment

vercel bot deployed to Preview – docs-v2 December 4, 2024 11:17 View deployment

vercel bot deployed to Preview – thirdweb-www December 4, 2024 11:19 View deployment

graphite-app bot reviewed Dec 4, 2024

View reviewed changes

packages/thirdweb/src/react/web/ui/ConnectWallet/Details.tsx Show resolved Hide resolved

kien-ngo force-pushed the kien/onClose-callback-modal branch from 4e1ad8d to a1ddcc2 Compare December 4, 2024 13:52

vercel bot deployed to Preview – wallet-ui December 4, 2024 13:55 View deployment

vercel bot deployed to Preview – thirdweb_playground December 4, 2024 13:55 View deployment

vercel bot deployed to Preview – docs-v2 December 4, 2024 14:00 View deployment

vercel bot deployed to Preview – thirdweb-www December 4, 2024 14:02 View deployment

kien-ngo force-pushed the kien/onClose-callback-modal branch from a1ddcc2 to d15a998 Compare December 4, 2024 18:42

vercel bot deployed to Preview – wallet-ui December 4, 2024 18:44 View deployment

vercel bot deployed to Preview – thirdweb_playground December 4, 2024 18:44 View deployment

vercel bot deployed to Preview – docs-v2 December 4, 2024 18:51 View deployment

vercel bot deployed to Preview – thirdweb-www December 4, 2024 18:52 View deployment

gregfromstl approved these changes Dec 4, 2024

View reviewed changes

kien-ngo force-pushed the kien/onClose-callback-modal branch 2 times, most recently from a9d7d06 to 3a9de42 Compare December 12, 2024 12:52

graphite-app bot reviewed Dec 12, 2024

View reviewed changes

packages/thirdweb/src/react/web/ui/ConnectWallet/Details.tsx Outdated Show resolved Hide resolved

vercel bot deployed to Preview – wallet-ui December 12, 2024 12:55 View deployment

kien-ngo force-pushed the kien/onClose-callback-modal branch from 3a9de42 to c90ebc1 Compare December 12, 2024 12:56

vercel bot deployed to Preview – wallet-ui December 12, 2024 12:58 View deployment

vercel bot deployed to Preview – thirdweb_playground December 12, 2024 12:59 View deployment

vercel bot had a problem deploying to Preview – thirdweb-www December 12, 2024 13:01 Failure

graphite-app bot reviewed Dec 12, 2024

View reviewed changes

packages/thirdweb/src/react/web/ui/prebuilt/Chain/icon.test.tsx Outdated Show resolved Hide resolved

kien-ngo force-pushed the kien/onClose-callback-modal branch from c90ebc1 to bbb3ab1 Compare December 12, 2024 13:07

vercel bot deployed to Preview – wallet-ui December 12, 2024 13:10 View deployment

vercel bot deployed to Preview – thirdweb_playground December 12, 2024 13:10 View deployment

vercel bot deployed to Preview – docs-v2 December 12, 2024 13:18 View deployment

vercel bot deployed to Preview – thirdweb-www December 12, 2024 14:09 View deployment

gregfromstl reviewed Dec 13, 2024

View reviewed changes

gregfromstl requested changes Dec 13, 2024

View reviewed changes

kien-ngo force-pushed the kien/onClose-callback-modal branch from bbb3ab1 to f1d4fd5 Compare December 14, 2024 11:31

graphite-app bot reviewed Dec 14, 2024

View reviewed changes

packages/thirdweb/src/utils/fetch.ts Outdated Show resolved Hide resolved

vercel bot had a problem deploying to Preview – wallet-ui December 14, 2024 11:32 Failure

vercel bot had a problem deploying to Preview – thirdweb_playground December 14, 2024 11:32 Failure

vercel bot had a problem deploying to Preview – thirdweb-www December 14, 2024 11:32 Failure

vercel bot had a problem deploying to Preview – docs-v2 December 14, 2024 11:33 Failure

update

99cc2f5

kien-ngo force-pushed the kien/onClose-callback-modal branch from f1d4fd5 to 99cc2f5 Compare December 14, 2024 11:35

graphite-app bot reviewed Dec 14, 2024

View reviewed changes

vercel bot deployed to Preview – wallet-ui December 14, 2024 11:37 View deployment

vercel bot deployed to Preview – thirdweb_playground December 14, 2024 11:37 View deployment

graphite-app bot reviewed Dec 14, 2024

View reviewed changes

vercel bot deployed to Preview – docs-v2 December 14, 2024 11:42 View deployment

vercel bot deployed to Preview – thirdweb-www December 14, 2024 11:43 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] Add onClose callback to details modal #5605

[SDK] Add onClose callback to details modal #5605

kien-ngo commented Dec 4, 2024 •

edited by pr-codex bot

Loading

vercel bot commented Dec 4, 2024 •

edited

Loading

changeset-bot bot commented Dec 4, 2024 •

edited

Loading

graphite-app bot commented Dec 4, 2024

kien-ngo commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading

gregfromstl left a comment

gregfromstl Dec 13, 2024

gregfromstl left a comment

graphite-app bot Dec 14, 2024

graphite-app bot Dec 14, 2024

		@@ -0,0 +1,3 @@
		import { describe } from "vitest";

		describe.runIf(process.env.TW_SECRET_KEY)("ChainIcon-legacy", () => {});

[SDK] Add onClose callback to details modal #5605

Are you sure you want to change the base?

[SDK] Add onClose callback to details modal #5605

Conversation

kien-ngo commented Dec 4, 2024 • edited by pr-codex bot Loading

PR-Codex overview

Detailed summary

vercel bot commented Dec 4, 2024 • edited Loading

changeset-bot bot commented Dec 4, 2024 • edited Loading

🦋 Changeset detected

graphite-app bot commented Dec 4, 2024

Your org has enabled the Graphite merge queue for merging into main

kien-ngo commented Dec 4, 2024 • edited Loading

How to use the Graphite Merge Queue

github-actions bot commented Dec 4, 2024 • edited Loading

size-limit report 📦

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

gregfromstl left a comment

Choose a reason for hiding this comment

gregfromstl Dec 13, 2024

Choose a reason for hiding this comment

gregfromstl left a comment

Choose a reason for hiding this comment

graphite-app bot Dec 14, 2024

Choose a reason for hiding this comment

graphite-app bot Dec 14, 2024

Choose a reason for hiding this comment

kien-ngo commented Dec 4, 2024 •

edited by pr-codex bot

Loading

vercel bot commented Dec 4, 2024 •

edited

Loading

changeset-bot bot commented Dec 4, 2024 •

edited

Loading

kien-ngo commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading