Playground: Headless components #5535
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Your org has enabled the Graphite merge queue for merging into mainAdd 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. |
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
github-actions
bot
added
the
Playground
kien-ngo
requested review from
joaquim-verges,
gregfromstl and
jnsdls
as code owners
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| AccountBalance | ||
| </h2> | ||
| <p className="max-w-[600px] text-lg"> | ||
| Display the current native of the wallet. |
There was a problem hiding this comment.
There appears to be a word missing in this description text. It should read either "Display the current native balance of the wallet" or "Display the current native token of the wallet" to properly convey the component's purpose.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
size-limit report 📦
|
kien-ngo
force-pushed
the
kien/headless-ui-playground
branch
from
f0b150e
to
1c90623
Compare
| <AccountBalance | ||
| chain={ethereum} | ||
| showFiatValue="USD" | ||
| loadingComponent={<span>Loading...</span>} | ||
| /> |
There was a problem hiding this comment.
The code example is inconsistent with the preview component shown above it. The example is missing the formatFn={formatAccountBalanceForButton} prop that's demonstrated in the preview. Adding this prop would make the example match the actual implementation and help developers understand how to properly format balance values.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## kien/account-balance-in-usd #5535 +/- ##
===============================================================
+ Coverage 46.16% 46.18% +0.01%
===============================================================
Files 1085 1084 -1
Lines 58871 58846 -25
Branches 3949 3949
===============================================================
- Hits 27180 27178 -2
+ Misses 31007 30985 -22
+ Partials 684 683 -1
*This pull request uses carry forward flags. Click here to find out more.
|
kien-ngo
force-pushed
the
kien/account-balance-in-usd
branch
from
4b66fbd
to
5b02ff0
Compare
kien-ngo
force-pushed
the
kien/headless-ui-playground
branch
from
1c90623
to
16e3aa2
Compare
kien-ngo
force-pushed
the
kien/headless-ui-playground
branch
from
16e3aa2
to
dc3d2ee
Compare
kien-ngo
force-pushed
the
kien/headless-ui-playground
branch
from
dc3d2ee
to
39f870d
Compare
| <> | ||
| <div className="space-y-2"> | ||
| <h2 className="font-semibold text-2xl tracking-tight sm:text-3xl"> | ||
| NFTImage |
There was a problem hiding this comment.
The component heading shows NFTImage but the examples use NFTMedia. Consider updating the heading to match the actual component name to avoid confusion in the documentation.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| <> | ||
| <div className="space-y-2"> | ||
| <h2 className="font-semibold text-2xl tracking-tight sm:text-3xl"> | ||
| NFTImage |
There was a problem hiding this comment.
The component heading NFTImage does not match the actual component being used (NFTMedia). For consistency with the implementation, the heading should be updated to NFTMedia.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| return num.toString(); | ||
| } | ||
| if (num < 1_000_000) { | ||
| return formatLargeNumber(num, 1000000, "k"); |
There was a problem hiding this comment.
The divisor for formatting thousands (k suffix) appears incorrect - using 1000000 instead of 1000. This means numbers between 1,000-999,999 won't be properly formatted with the 'k' suffix. Recommend changing to:
return formatLargeNumber(num, 1000, 'k');Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| queryKey: [ | ||
| "account-avatar", | ||
| address, | ||
| { socialType }, | ||
| { resolverAddress, resolverChain }, |
There was a problem hiding this comment.
Simplify query key array to use direct values instead of objects: ['account-avatar', address, socialType, resolverAddress, resolverChain]
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
| lang="tsx" | ||
| /> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| function formatNumber(value: number, decimalPlaces: number) { | ||
| if (value === 0) return 0; | ||
| const precision = 10 ** decimalPlaces; | ||
| const threshold = 1 / 10 ** decimalPlaces; // anything below this if rounded will result in zero - so use ceil instead | ||
| const fn: "ceil" | "round" = value < threshold ? "ceil" : "round"; | ||
| return Math[fn]((value + Number.EPSILON) * precision) / precision; | ||
| } | ||
|
|
||
| function formatAccountBalance(props: AccountBalanceFormatParams): string { | ||
| if (props.fiatBalance && props.fiatSymbol) { | ||
| // Need to keep them short to avoid UI overflow issues | ||
| const formattedTokenBalance = formatNumber(props.tokenBalance, 1); | ||
| const formattedFiatBalance = formatFiatValue(props.fiatBalance, 0); | ||
| return `${formattedTokenBalance} ${props.tokenSymbol} (${props.fiatSymbol}${formattedFiatBalance})`; | ||
| } | ||
| const formattedTokenBalance = formatNumber( | ||
| props.tokenBalance, | ||
| props.tokenBalance < 1 ? 5 : 4, | ||
| ); | ||
| return `${formattedTokenBalance} ${props.tokenSymbol}`; | ||
| } | ||
|
|
||
| function formatFiatValue(value: number, decimals: number): string { | ||
| const num = formatNumber(value, decimals); | ||
| if (num < 1000) { | ||
| return num.toString(); | ||
| } | ||
| if (num < 1_000_000) { | ||
| return formatLargeNumber(num, 1000000, "k"); | ||
| } | ||
| if (num < 1000000000) { | ||
| return formatLargeNumber(num, 1000000, "M"); | ||
| } | ||
| return formatLargeNumber(num, 1000000000, "B"); |
There was a problem hiding this comment.
Move formatting utility functions to a shared utility file to reduce code duplication and bundle size
Spotted by Graphite Reviewer (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
Problem solved
Short description of the bug fixed or feature added
PR-Codex overview
This PR introduces new UI components and updates existing ones for enhanced account and NFT functionalities in a web application. It adds new sections and examples for Headless UI components, improving the overall user experience in connecting wallets and displaying account information.
Detailed summary
ArticleCardIndexfor "Headless UI" inpage.tsx.navLinks.ts.avatar.tsxandname.tsx.NftMediaBasic,NftNameBasic,NftDescriptionBasic,NftCardDemo) innft-examples.tsx.AccountAddressBasic,AccountAvatarBasic, etc.) inaccount-examples.tsx.ThirdwebProviderfor context management in UI pages.