wallet-ext: support nft standard #9673

pchrysochoidis · 2023-03-21T23:10:41Z

Description

nfts page show only objects that define the display property
removes any other support for nft metadata

before	after

Screen.Recording.2023-03-22.at.01.53.42.mov

Test Plan

manual testing

Type of Change (Check all that apply)

user-visible impact

Release notes

Sui wallet now supports NFT display standard. All other display nft metadata support is removed.

closes APPS-633

vercel · 2023-03-21T23:10:48Z

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

Name	Status	Preview	Comments	Updated
explorer	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 24, 2023 at 7:28PM (UTC)
explorer-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 24, 2023 at 7:28PM (UTC)
sui-wallet-kit	❌ Failed (Inspect)			Mar 24, 2023 at 7:28PM (UTC)

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
wallet-adapter	⬜️ Ignored (Inspect)			Mar 24, 2023 at 7:28PM (UTC)

Jordan-Mysten

A small tweak on how we're approaching it. General idea looks good though.

Jordan-Mysten · 2023-03-21T23:57:41Z

apps/wallet/src/ui/app/shared/text/index.tsx

@@ -54,6 +54,10 @@ const textStyles = cva([], {
            true: 'truncate',
            false: '',
        },
+        multilineTruncate: {


Can you just put this on the parent node instead of in the text component?

Jordan-Mysten · 2023-03-22T00:03:19Z

apps/core/src/utils/nftStandard.ts

+    }
+}
+
+export function processDisplay(display: Record<string, string> | null) {


We shouldn't use this kind of pattern, given display data can include anything outside of these fields that we'll still display in some contexts. Would rather just handle the display fields piecemeal. This also aligns with our migration away from view-model-like processing of data (which this is closer to).

Jordan-Mysten · 2023-03-22T00:05:23Z

apps/wallet/src/ui/app/hooks/useGetNFTMeta.ts

-            url: fields.url,
-        };
+        if (!is(details, SuiObjectData) || !details.display) return null;
+        return processDisplay(details.display);


If you nuke the processDisplay from core, you could just move it here basically, since this is already basically a view-model approach to this problem (which is fine for getting this out, just don't want to proliferate it by moving it to core).

Jordan-Mysten · 2023-03-22T00:05:53Z

apps/wallet/src/ui/app/hooks/useGetObject.ts

 import { useQuery, type UseQueryResult } from '@tanstack/react-query';

 export function useGetObject(
-    objectId: string
+    objectId: string,
+    options?: SuiObjectDataOptions


Why not just always fetch all the data for the sake of improved cache hits?

Jordan-Mysten · 2023-03-22T00:06:40Z

apps/wallet/src/ui/app/hooks/useOwnedNFT.ts

@@ -15,7 +15,7 @@ export function useOwnedNFT(
    nftObjectId: string | null,
    address: SuiAddress | null
 ) {
-    const data = useGetObject(nftObjectId!);
+    const data = useGetObject(nftObjectId || '', { showDisplay: true });


I feel like this is easy enough to get wrong that I'd prefer to just get all the data in the hook always.

pchrysochoidis · 2023-03-22T01:00:58Z

@Jordan-Mysten I think I addressed your comments. Please have another look!

mystie711 · 2023-03-22T01:02:36Z

On hover to expand description looks pretty good @pchrysochoidis.
I see that the ellipsis at the end is getting cut short by a dot. Bug?

Also, following up with you on slack about Object ID, Media Type addition.

pchrysochoidis · 2023-03-22T01:58:55Z

@mystie711 I did the changes and updated the video in the description

mystie711

looking good.

Jordan-Mysten

Took a brief look, some nits, nothing super concerning, might have someone else take a deeper look

Jordan-Mysten · 2023-03-22T21:01:23Z

apps/wallet/package.json

@@ -57,6 +57,7 @@
        "@storybook/react": "7.0.0-rc.3",
        "@storybook/react-webpack5": "7.0.0-rc.3",
        "@storybook/theming": "7.0.0-rc.3",
+        "@tailwindcss/line-clamp": "^0.4.2",


nit: Should we add this to core?

Jordan-Mysten · 2023-03-22T21:02:35Z

apps/wallet/src/ui/app/hooks/useOwnedNFT.ts

@@ -15,7 +15,7 @@ export function useOwnedNFT(
    nftObjectId: string | null,
    address: SuiAddress | null
 ) {
-    const data = useGetObject(nftObjectId!);
+    const data = useGetObject(nftObjectId || '');


This hook doesn't look like it handles nullable object IDs, instead of this defaulting we should update it to handle this properly

Jordan-Mysten · 2023-03-22T21:04:05Z

apps/wallet/src/ui/app/pages/home/nft-details/LabelValueItem.tsx

+
+export type LabelValueItemProps = {
+    label: string;
+    value: ReactNode | LinkData;


Never a big fan of this type of variability.

* nfts page show only objects that define the display property * removes any other support for nft metadata

All comments are addressed - dismissing to unblock.

pchrysochoidis requested review from williamrobertson13, damirka, Jibz1, mystie711 and Jordan-Mysten March 21, 2023 23:10

Jordan-Mysten previously requested changes Mar 22, 2023

View reviewed changes

vercel bot temporarily deployed to Preview – explorer-storybook March 22, 2023 00:54 Inactive

vercel bot deployed to Preview – explorer March 22, 2023 00:54 View deployment

pchrysochoidis force-pushed the pc-wallet-ext-apps-633-nfts-page branch from 24566a3 to db8f944 Compare March 22, 2023 00:57

vercel bot deployed to Preview – explorer March 22, 2023 00:58 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 22, 2023 00:59 Inactive

mystie711 approved these changes Mar 22, 2023

View reviewed changes

Jordan-Mysten reviewed Mar 22, 2023

View reviewed changes

pchrysochoidis force-pushed the pc-wallet-ext-apps-633-nfts-page branch from 9f037f4 to 5f07fcc Compare March 23, 2023 22:55

vercel bot deployed to Preview – explorer March 23, 2023 22:56 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 23, 2023 22:56 Inactive

pchrysochoidis force-pushed the pc-wallet-ext-apps-633-nfts-page branch from 5f07fcc to 5dcf800 Compare March 24, 2023 17:59

vercel bot deployed to Preview – explorer March 24, 2023 18:01 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 24, 2023 18:01 Inactive

pchrysochoidis added 4 commits March 24, 2023 18:45

wallet-ext: support nft standard

cc71ba0

* nfts page show only objects that define the display property * removes any other support for nft metadata

wallet-ext: address pr comments

d20acdf

wallet-ext: address ui comments

9054d19

address pr comments

83d7fb0

pchrysochoidis force-pushed the pc-wallet-ext-apps-633-nfts-page branch from 5dcf800 to 83d7fb0 Compare March 24, 2023 19:27

vercel bot deployed to Preview – explorer March 24, 2023 19:28 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 24, 2023 19:28 Inactive

Jibz-Mysten approved these changes Mar 24, 2023

View reviewed changes

pchrysochoidis merged commit 2db925b into main Mar 27, 2023

pchrysochoidis deleted the pc-wallet-ext-apps-633-nfts-page branch March 27, 2023 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet-ext: support nft standard #9673

wallet-ext: support nft standard #9673

Uh oh!

pchrysochoidis commented Mar 21, 2023 •

edited

Loading

Uh oh!

vercel bot commented Mar 21, 2023 •

edited

Loading

Uh oh!

Jordan-Mysten left a comment

Uh oh!

Jordan-Mysten Mar 21, 2023

Uh oh!

Jordan-Mysten Mar 22, 2023

Uh oh!

Jordan-Mysten Mar 22, 2023

Uh oh!

Jordan-Mysten Mar 22, 2023

Uh oh!

Jordan-Mysten Mar 22, 2023

Uh oh!

pchrysochoidis commented Mar 22, 2023

Uh oh!

mystie711 commented Mar 22, 2023

Uh oh!

pchrysochoidis commented Mar 22, 2023

Uh oh!

mystie711 left a comment

Uh oh!

Jordan-Mysten left a comment

Uh oh!

Jordan-Mysten Mar 22, 2023

Uh oh!

Jordan-Mysten Mar 22, 2023

Uh oh!

Jordan-Mysten Mar 22, 2023

Uh oh!

Uh oh!

wallet-ext: support nft standard #9673

wallet-ext: support nft standard #9673

Uh oh!

Conversation

pchrysochoidis commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Plan

Type of Change (Check all that apply)

Release notes

Uh oh!

vercel bot commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jordan-Mysten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pchrysochoidis commented Mar 22, 2023

Uh oh!

mystie711 commented Mar 22, 2023

Uh oh!

pchrysochoidis commented Mar 22, 2023

Uh oh!

mystie711 left a comment

Choose a reason for hiding this comment

Uh oh!

Jordan-Mysten left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pchrysochoidis commented Mar 21, 2023 •

edited

Loading

vercel bot commented Mar 21, 2023 •

edited

Loading