feat: add view components #1483

alessey · 2024-10-23T14:17:05Z

What changed? Why?
Add nft view components

Notes to reviewers

How has it been tested?

vercel · 2024-10-23T14:17:16Z

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

Name	Status	Preview	Comments	Updated (UTC)
onchainkit-coverage	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 23, 2024 5:59pm
onchainkit-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 23, 2024 5:59pm
onchainkit-routes	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 23, 2024 5:59pm

0xAlec · 2024-10-23T14:32:11Z

src/nft/components/view/NFTImage.tsx

+  onLoading,
+  onLoaded,
+  onError,


should these just be onStatus?

I think I'd need to basically create a pseudo lifecycle status to change to onStatus. I could handle lifecycle status in the children, but I was trying to keep the mediaLoaded/success logic centralized in one place.

0xAlec · 2024-10-23T14:34:17Z

src/nft/components/view/NFTLastSoldPrice.tsx

+
+export function NFTLastSoldPrice({
+  className,
+  label = 'Mint price',


should we standardize on label? we use text in our other components

label seems more flexible if you ever want to change from string to ReactNode

0xAlec · 2024-10-23T14:36:09Z

src/nft/components/view/NFTMedia.tsx

+  if (mediaType.isVideo) {
+    return (
+      <NFTVideo
+        onLoading={handleLoading}
+        onLoaded={handleLoaded}
+        onError={handleError}
+      />
+    );
+  }
+
+  if (mediaType.isAudio) {
+    return (
+      <div className="relative w-full">
+        <NFTImage />
+        <div className="absolute bottom-[20px] mx-auto w-full">
+          <NFTAudio />
+        </div>
+      </div>
+    );
+  }
+
+  return (
+    <NFTImage
+      onLoading={handleLoading}
+      onLoaded={handleLoaded}
+      onError={handleError}
+    />
+  );


prefer a switch here

any particular reason? performance differences at this point are negligible

not for performance but for > 2 cases ifl it's cleaner to just use a switch compared to multiple if statements

definitely a nit though

updated, it made more sense when i switched to an enum anyways

0xAlec · 2024-10-23T14:36:57Z

src/nft/components/view/NFTMedia.tsx

+  const mediaType = useMemo(() => {
+    return {
+      isVideo: mimeType?.startsWith('video'),
+      isAudio:
+        mimeType?.startsWith('audio') || mimeType?.startsWith('application'),
+      isImage: mimeType?.startsWith('image'),
+    };
+  }, [mimeType]);


prefer returning an enum here rather than an object with always 2/3 null values

0xAlec · 2024-10-23T14:38:10Z

src/nft/components/view/NFTNetwork.tsx

+} as Record<string, ReactNode>;
+
+export function NFTNetwork({ className, label = 'Network' }: NFTNetworkProps) {
+  const { chain } = useOnchainKit();


I think we should deprecate this top-level chain prop, so I don't recommend using it here.

Is there any other way to tell which chain the NFT is on?

cpcramer · 2024-10-23T16:36:54Z

src/internal/svg/baseSvg.tsx

+    <circle cx="73" cy="73" r="73" fill="#0052FF" />
+    <path
+      d="M73.323 123.729C101.617 123.729 124.553 100.832 124.553 72.5875C124.553 44.343 101.617 21.4463 73.323 21.4463C46.4795 21.4463 24.4581 42.0558 22.271 68.2887H89.9859V76.8864H22.271C24.4581 103.119 46.4795 123.729 73.323 123.729Z"
+      fill="white"


Is the fill always meant to be white here? Is the intention for it to NOT update based off of the current theme?

no, I didn't think the base logo should change

cpcramer · 2024-10-23T16:45:59Z

src/nft/components/view/NFTMintDate.tsx

+  return (
+    <div
+      className={cn(
+        'flex items-center justify-between py-1',
+        text.label2,
+        className,
+      )}
+    >
+      <div>{label}</div>
+      <div>{formattedDate}</div>
+    </div>
+  );


I think these divs are unnecessary and can be removed:

Suggested change

return (

<div

className={cn(

'flex items-center justify-between py-1',

text.label2,

className,

)}

>

<div>{label}</div>

<div>{formattedDate}</div>

</div>

);

return (

<div

className={cn(

'flex items-center justify-between py-1',

text.label2,

className,

)}

>

{label}

{formattedDate}

</div>

);

since i'm flex space-between these divs are needed so the content is aligned left/right, otherwise they'd be treated as a single text node

cpcramer · 2024-10-23T16:48:30Z

src/nft/components/view/NFTNetwork.tsx

+  return (
+    <div
+      className={cn(
+        'flex items-center justify-between py-1',
+        text.label2,
+        className,
+      )}
+    >
+      <div>{label}</div>
+      <div className="flex items-center gap-2">
+        <div className="h-4 w-4 object-cover">{networkMap[chain.name]}</div>
+        <div>{chain.name}</div>
+      </div>
+    </div>
+  );


Probably can remove these divs that don't have styling:

Suggested change

return (

<div

className={cn(

'flex items-center justify-between py-1',

text.label2,

className,

)}

>

<div>{label}</div>

<div className="flex items-center gap-2">

<div className="h-4 w-4 object-cover">{networkMap[chain.name]}</div>

<div>{chain.name}</div>

</div>

</div>

);

return (

<div

className={cn(

'flex items-center justify-between py-1',

text.label2,

className,

)}

>

{label}

<div className="flex items-center gap-2">

<div className="h-4 w-4 object-cover">{networkMap[chain.name]}</div>

{chain.name}

</div>

</div>

);

this is possible, but I think its easier to see whats going on using an element as a flex-child vs. a text node.

cpcramer · 2024-10-23T16:49:44Z

src/nft/components/view/NFTOwner.tsx

+        className,
+      )}
+    >
+      <div>{label}</div>


Suggested change

<div>{label}</div>

{label}

cpcramer · 2024-10-23T16:51:01Z

src/nft/components/view/NFTVideo.tsx

+      videoRef.current.onerror = (error: string | Event) => {
+        onError?.({
+          error: typeof error === 'string' ? error : error.type,
+          code: 'NmNVc01', // NFT module NFTVideo component 01 error


Should we add all of the possible error codes in a error / const file and reference from it?

I like this idea

let me revisit that at the end

abcrane123 · 2024-10-23T17:51:04Z

src/nft/components/view/NFTImage.tsx

+
+  const handleTransitionEnd = () => {
+    setTransitionEnded(true);
+  };


nit but should this be useCallback just to keep consistent

abcrane123 · 2024-10-23T17:51:35Z

src/nft/components/view/NFTImage.tsx

+  onLoading?: (mediaUrl: string) => void;
+  onLoaded?: () => void;
+  onError?: (error: NFTError) => void;
+};


nit move to types file

abcrane123 · 2024-10-23T17:53:38Z

src/nft/components/view/NFTLastSoldPrice.tsx

+type NFTLastSoldPriceProps = {
+  className?: string;
+  label?: ReactNode;
+};


nit move to types

abcrane123 · 2024-10-23T17:54:16Z

src/nft/components/view/NFTMintDate.tsx

+type NFTMintDateProps = {
+  className?: string;
+  label?: ReactNode;
+};


nit move to types

vercel bot deployed to Preview – onchainkit-routes October 23, 2024 14:17 View deployment

github-actions bot added root pkg: styles labels Oct 23, 2024

vercel bot deployed to Preview – onchainkit October 23, 2024 14:18 View deployment

vercel bot deployed to Preview – onchainkit-playground October 23, 2024 14:18 View deployment

vercel bot deployed to Preview – onchainkit-coverage October 23, 2024 14:18 View deployment

alessey force-pushed the alessey/nft-2 branch from b692955 to c38f148 Compare October 23, 2024 14:21

vercel bot deployed to Preview – onchainkit-routes October 23, 2024 14:21 View deployment

vercel bot deployed to Preview – onchainkit October 23, 2024 14:23 View deployment

feat: add view components

64ebe5e

vercel bot deployed to Preview – onchainkit-playground October 23, 2024 14:23 View deployment

alessey force-pushed the alessey/nft-2 branch from c38f148 to 64ebe5e Compare October 23, 2024 14:23

vercel bot deployed to Preview – onchainkit-routes October 23, 2024 14:23 View deployment

vercel bot deployed to Preview – onchainkit October 23, 2024 14:24 View deployment

vercel bot deployed to Preview – onchainkit-playground October 23, 2024 14:24 View deployment

vercel bot deployed to Preview – onchainkit-coverage October 23, 2024 14:25 View deployment

alessey marked this pull request as ready for review October 23, 2024 14:29

0xAlec reviewed Oct 23, 2024

View reviewed changes

alessey added 2 commits October 23, 2024 11:27

fix: update string to reactNode

78ca481

fix: pr comments

4610ff4

vercel bot deployed to Preview – onchainkit-routes October 23, 2024 15:58 View deployment

vercel bot deployed to Preview – onchainkit October 23, 2024 15:59 View deployment

vercel bot deployed to Preview – onchainkit-playground October 23, 2024 16:00 View deployment

vercel bot had a problem deploying to Preview – onchainkit-coverage October 23, 2024 16:00 Failure

fix: lint/tests

63520de

vercel bot deployed to Preview – onchainkit October 23, 2024 16:11 View deployment

vercel bot deployed to Preview – onchainkit-coverage October 23, 2024 16:12 View deployment

fix: style updates

dc41c27

vercel bot deployed to Preview – onchainkit-routes October 23, 2024 16:24 View deployment

vercel bot deployed to Preview – onchainkit October 23, 2024 16:26 View deployment

vercel bot deployed to Preview – onchainkit-playground October 23, 2024 16:26 View deployment

vercel bot deployed to Preview – onchainkit-coverage October 23, 2024 16:26 View deployment

alessey requested review from dschlabach, cpcramer, abcrane123 and 0xAlec October 23, 2024 16:34

cpcramer reviewed Oct 23, 2024

View reviewed changes

src/nft/components/view/NFTOwner.tsx

className,

)}

>

<div>{label}</div>

Copy link

Contributor

cpcramer Oct 23, 2024

Choose a reason for hiding this comment

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

Suggested change

<div>{label}</div>

{label}

cpcramer reviewed Oct 23, 2024

View reviewed changes

alessey requested a review from cpcramer October 23, 2024 17:06

cpcramer approved these changes Oct 23, 2024

View reviewed changes

abcrane123 reviewed Oct 23, 2024

View reviewed changes

fix: add usecallback

90081bf

vercel bot deployed to Preview – onchainkit-routes October 23, 2024 17:57 View deployment

vercel bot deployed to Preview – onchainkit-playground October 23, 2024 17:58 View deployment

vercel bot deployed to Preview – onchainkit October 23, 2024 17:58 View deployment

vercel bot deployed to Preview – onchainkit-coverage October 23, 2024 17:59 View deployment

abcrane123 approved these changes Oct 23, 2024

View reviewed changes

alessey merged commit 4447721 into main Oct 23, 2024
16 checks passed

alessey deleted the alessey/nft-2 branch October 23, 2024 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add view components #1483

feat: add view components #1483

alessey commented Oct 23, 2024 •

edited

Loading

vercel bot commented Oct 23, 2024 •

edited

Loading

0xAlec Oct 23, 2024

alessey Oct 23, 2024

0xAlec Oct 23, 2024

alessey Oct 23, 2024

0xAlec Oct 23, 2024

alessey Oct 23, 2024

0xAlec Oct 23, 2024 •

edited

Loading

0xAlec Oct 23, 2024

alessey Oct 23, 2024

0xAlec Oct 23, 2024

0xAlec Oct 23, 2024

cpcramer Oct 23, 2024

alessey Oct 23, 2024

cpcramer Oct 23, 2024

alessey Oct 23, 2024

cpcramer Oct 23, 2024 •

edited

Loading

alessey Oct 23, 2024

cpcramer Oct 23, 2024

cpcramer Oct 23, 2024

0xAlec Oct 23, 2024

alessey Oct 23, 2024

abcrane123 Oct 23, 2024

abcrane123 Oct 23, 2024

abcrane123 Oct 23, 2024

abcrane123 Oct 23, 2024

feat: add view components #1483

feat: add view components #1483

Conversation

alessey commented Oct 23, 2024 • edited Loading

vercel bot commented Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xAlec Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcramer Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessey commented Oct 23, 2024 •

edited

Loading

vercel bot commented Oct 23, 2024 •

edited

Loading

0xAlec Oct 23, 2024 •

edited

Loading

cpcramer Oct 23, 2024 •

edited

Loading