Skip to content

feat(namekit-react): Identity #439

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.

Sign up for GitHub

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

Open
notrab wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat(namekit-react): Identity #439

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
db46f78
feat(namekit-react): Identity
notrab Oct 15, 2024
450998f
wrap the client component
notrab Oct 15, 2024
1d92fb1
feat: handle avatar and nameguard report state
notrab Oct 22, 2024
105a487
feat: add ens profile link
notrab Oct 22, 2024
ef9dbc5
feat: follower count
notrab Oct 23, 2024
3ddd190
rename ethfollow func
notrab Oct 28, 2024
ec2cb9c
feat: apply feedback from lightwalker
notrab Nov 3, 2024
752ac3a
updates to profile link
notrab Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 59 additions & 59 deletions apps/storybook.namekit.io/stories/Namekit/Identity.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import React from "react";
import React, { useRef } from "react";
import type { Meta, StoryObj } from "@storybook/react";
import { Identity, NameKitProvider } from "@namehash/namekit-react/client";
import {
Identity,
NameKitProvider,
ProfileLinkGenerator,
} from "@namehash/namekit-react/client";

const meta: Meta<typeof Identity.Root> = {
title: "Namekit/Identity",
Expand All @@ -21,6 +25,15 @@ export default meta;

type Story = StoryObj<typeof Identity.Root>;

const TwitterProfileLink = new ProfileLinkGenerator(
"Twitter",
"https://twitter.com/",
);
const GitHubProfileLink = new ProfileLinkGenerator(
"GitHub",
"https://github.com/",
);

const DefaultIdentityCard: React.FC<{
address: string;
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 4, 2024

Choose a reason for hiding this comment

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

Please check all places in the code where you have an address param. These should all use the Address type defined in viem.

network?: "mainnet" | "sepolia";
Expand All @@ -45,17 +58,10 @@ const DefaultIdentityCard: React.FC<{
</Identity.Root>
);

const customAppConfig = {
profileLinks: {
getProfileURL: (address: string) => `/profiles/${address}`,
getProfileLink: (address: string, children: React.ReactNode) => (
<a href={`/profiles/${address}`}>{children}</a>
),
},
};

const CustomAppIdentityCard: React.FC<{ address: string }> = ({ address }) => (
<NameKitProvider config={customAppConfig}>
<NameKitProvider
config={{ profileLinks: [TwitterProfileLink, GitHubProfileLink] }}
>
<Identity.Root address={address}>
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 4, 2024

Choose a reason for hiding this comment

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

Suggest we make a few updates here to put our "Identity" components on a stronger foundation. Specifically: an "identity reference" is more than just an address. It is a combination of a chain and an address.

  1. Suggest defining an interface for ImplicitIdentityReference that combines both an address and a chain, but where the chain param is optional. The purpose of this interface is to optimize DX for the average case where all addresses within an app are on the same chain. It should be noted how this isn't always the case though, and increasingly won't be in the future the way things are headed.
  2. Suggest defining an interface for IdentityReference that extends ImplicitIdentityReference but makes the chain param required.
  3. Suggest defining a little utility function that can be used within the context of a NameKitProvider that automatically converts any ImplicitIdentityReference to an explicit IdentityReference using the configured chain as the default.
  4. There's a bunch of functions here that currently only take an "address" param. These should change so that they instead take a param of type IdentityReference or ImplicitIdentityReference. Inside the implementation of these functions, we can call the utility function described above that takes either an IdentityReference or ImplicitIdentityReference as a param and always returns an explicit IdentityReference for use in all our internal logic.
  5. Functions such as getProfileURL and getProfileLink should take an IdentityReference as a param. The default implementation of these functions should then consider the chain parameter and not only the address. For example. If the chain param is mainnet, then our existing strategy is good. If the chain param is anything else (such as sepolia, etc..), then there isn't an existing profile service we can easily link to for this. So instead suggest making our default implementation of all the profile link generator functions trigger some alert box or something like that for the case that the chain is anything but mainnet. Once we build the backend APIs into NameKit for generating profile data, we can come back to this and implement some improved default behaviour for showing profiles on chains other than mainnet.

Here's an acceptance criteria for everything described above: If I use our Identity components to show the identity for a chain other than mainnet (such as sepolia) I shouldn't get a profile link to view the profile of that address on mainnet as that wouldn't actually be the correct profile / identity.

Please ask me if any questions 👍

<Identity.Avatar />
<Identity.Name />
Expand All @@ -66,36 +72,35 @@ const CustomAppIdentityCard: React.FC<{ address: string }> = ({ address }) => (
</NameKitProvider>
);

const modalConfig = {
profileLinks: {
getProfileURL: (address: string) => `#${address}`,
getProfileLink: (address: string, children: React.ReactNode) => (
<button
onClick={(e) => {
e.preventDefault();
alert(`Would open modal for ${address}`);
}}
>
{children}
</button>
),
},
};
const ModalIdentityCard: React.FC<{ address: string }> = ({ address }) => {
const dialogRef = useRef<HTMLDialogElement>(null);

const ModalIdentityCard: React.FC<{ address: string }> = ({ address }) => (
<NameKitProvider config={modalConfig}>
<Identity.Root address={address}>
<Identity.Avatar />
<Identity.Name />
<Identity.ProfileLink>
<>
<span>Open Profile Modal</span>
<span>→</span>
</>
</Identity.ProfileLink>
</Identity.Root>
</NameKitProvider>
);
const handleClick = (e: React.MouseEvent) => {
e.preventDefault();
dialogRef.current?.showModal();
};

return (
<NameKitProvider
config={{ profileLinks: [new ProfileLinkGenerator("Modal", "#")] }}
>
<Identity.Root address={address}>
<Identity.Avatar />
<Identity.Name />
<Identity.ProfileLink onClick={handleClick}>
<>
<span>Open Profile Modal</span>
<span>→</span>
</>
</Identity.ProfileLink>
</Identity.Root>
<dialog ref={dialogRef}>
Hello {address}
<button onClick={() => dialogRef.current?.close()}>Close</button>
</dialog>
</NameKitProvider>
);
};

export const Default: Story = {
args: {
Expand All @@ -110,8 +115,8 @@ export const MultipleCards: Story = {
render: () => (
<>
<DefaultIdentityCard address="0x838aD0EAE54F99F1926dA7C3b6bFbF617389B4D9" />
<CustomAppIdentityCard address="0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045" />
<ModalIdentityCard address="0xf81bc66316a3f2a60adc258f97f61dfcbdd23bb1" />
<DefaultIdentityCard address="0xd8dA6BF26964aF9D7eEd9e03E53415D37aA96045" />
<DefaultIdentityCard address="0xf81bc66316a3f2a60adc258f97f61dfcbdd23bb1" />
</>
),
};
Expand All @@ -121,10 +126,12 @@ export const ProfileLinkVariants: Story = {
const address = "0x838aD0EAE54F99F1926dA7C3b6bFbF617389B4D9";

return (
<div>
<div className="nk-space-y-8">
<div>
<h3>Default ENS App Link</h3>
<Identity.Root address={address}>
<Identity.Avatar />
<Identity.Name />
<Identity.ProfileLink>
<div className="nk-flex nk-items-center nk-gap-2">
<ENSLogo />
Expand All @@ -136,27 +143,20 @@ export const ProfileLinkVariants: Story = {

<div>
<h3>Custom App Link</h3>
<NameKitProvider config={customAppConfig}>
<NameKitProvider
config={{ profileLinks: [TwitterProfileLink, GitHubProfileLink] }}
>
<Identity.Root address={address}>
<Identity.ProfileLink>
<button>View Profile</button>
</Identity.ProfileLink>
<Identity.Avatar />
<Identity.Name />
<Identity.ProfileLinks />
</Identity.Root>
</NameKitProvider>
</div>

<div>
<h3>Modal Trigger</h3>
<NameKitProvider config={modalConfig}>
<Identity.Root address={address}>
<Identity.ProfileLink>
<>
<span>Open Profile Modal</span>
<span>→</span>
</>
</Identity.ProfileLink>
</Identity.Root>
</NameKitProvider>
<h3>Modal Link</h3>
<ModalIdentityCard address={address} />
</div>
</div>
);
Expand Down
6 changes: 5 additions & 1 deletion packages/namekit-react/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,8 @@ export {
} from "./components/CurrencySymbol/CurrencySymbol";
export { TruncatedText } from "./components/TruncatedText";

export { Identity, NameKitProvider } from "./components/Identity";
export {
Identity,
NameKitProvider,
ProfileLinkGenerator,
} from "./components/Identity";
119 changes: 87 additions & 32 deletions packages/namekit-react/src/components/Identity.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,21 +12,46 @@ import {
} from "@namehash/nameguard";
import cc from "classcat";

export interface ProfileLinkConfig {
getProfileURL: (address: string) => string;
getProfileLink: (address: string, children: ReactNode) => JSX.Element;
export class ProfileLinkGenerator {
private baseURL: string;
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 5, 2024

Choose a reason for hiding this comment

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

I don't think this should have a "baseURL". This assumes too much about how the URLs will be constructed, especially when considering how they should consider both chain and address.

private name: string;

constructor(name: string, baseURL: string) {
this.name = name;
this.baseURL = baseURL;
}

getName(): string {
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 5, 2024

Choose a reason for hiding this comment

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

Can you help me understand the goal for getName? It will help a lot to document ideas in comments.

return this.name;
}

getProfileURL(address: string): string {
return `${this.baseURL}${address}`;
}
}
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 4, 2024

Choose a reason for hiding this comment

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

I shared some other comments in this PR with the idea of having a default fallback implementation of the profile link anchor. Ex: An ENS Logo, etc..

It seems to me that one nice way to do this would be to define another function in this interface, for example: getProfileLinkAnchor that takes no params and returns a JSX.Element. We could have a default implementation that provides an anchor that is an ENS Logo, for example.

Appreciate your advice / suggestions 👍


export const ENSProfileLink = new ProfileLinkGenerator(
"ENS",
"https://app.ens.domains/",
);

const DEFAULT_PROFILE_LINKS = [ENSProfileLink];
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 5, 2024

Choose a reason for hiding this comment

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

Interesting. Can you help me understand the goal of having an array here?


interface NameKitConfig {
profileLinks?: ProfileLinkConfig;
profileLinks?: ProfileLinkGenerator[];
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 5, 2024

Choose a reason for hiding this comment

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

Similar to the feedback above. Can you help me understand the goal for representing this as an array? In my thinking there should be exactly one, not 0 or many.

Suggested change
profileLinks?: ProfileLinkGenerator[];
profileLinkGenerator: ProfileLinkGenerator;

}

const NameKitConfigContext = createContext<NameKitConfig>({});

export const NameKitProvider: React.FC<{
interface NameKitProviderProps {
children: React.ReactNode;
config: NameKitConfig;
}> = ({ children, config }) => {
}

export const NameKitProvider: React.FC<NameKitProviderProps> = ({
children,
config,
}) => {
return (
<NameKitConfigContext.Provider value={config}>
{children}
Expand Down Expand Up @@ -268,40 +293,69 @@ const Followers = ({ className, ...props }: SubComponentProps) => {
);
};

interface ProfileLinkProps extends SubComponentProps {
config?: ProfileLinkConfig;
interface ProfileLinkProps {
config?: ProfileLinkGenerator;
className?: string;
children?: React.ReactNode;
onClick?: (e: React.MouseEvent) => void;
}

const ProfileLink = ({
className,
const ProfileLink: React.FC<ProfileLinkProps> = ({
config,
children,
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 4, 2024

Choose a reason for hiding this comment

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

Would it make sense to make children an optional param, and if not provided, to have some default children that would for example just be the ENS Logo?

config: instanceConfig,
...props
}: ProfileLinkProps) => {
const { loadingState, address } = useIdentity();
const globalConfig = useNameKitConfig();
onClick,
}) => {
const identity = useIdentity();
const nameKitConfig = useNameKitConfig();

if (loadingState !== "success") {
const linkConfig =
config || nameKitConfig.profileLinks?.[0] || DEFAULT_PROFILE_LINKS[0];

if (!identity) {
console.warn("ProfileLink used outside of Identity context");
return null;
}

const config = instanceConfig ||
globalConfig.profileLinks || {
getProfileURL: (address) => `https://app.ens.domains/${address}`,
getProfileLink: (address, children) => (
<a
href={`https://app.ens.domains/${address}`}
target="_blank"
rel="noopener noreferrer"
className={cc(["namekit-profile-link", className])}
{...props}
>
{children}
</a>
),
};
const url = linkConfig.getProfileURL(identity.address);

return config.getProfileLink(address, children);
return (
<a
Copy link
Member

@lightwalker-eth lightwalker-eth Oct 23, 2024

Choose a reason for hiding this comment

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

Appreciate your advice. Should we use our <Link for this instead of <a?

Copy link
Member Author

@notrab notrab Oct 28, 2024

Choose a reason for hiding this comment

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

In theory, yes. But I want to make more progress on the core implementation before moving to UI.

href={url}
target="_blank"
rel="noopener noreferrer"
className="namekit-profile-link"
onClick={onClick}
>
{children || linkConfig.getName()}
</a>
);
};

interface ProfileLinksProps {
Copy link
Member

@lightwalker-eth lightwalker-eth Nov 5, 2024

Choose a reason for hiding this comment

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

Let's have a chat to help align on goals here. I think we're drifting away from solving the problems that people are actually looking to solve.

configs?: ProfileLinkGenerator[];
className?: string;
}

const ProfileLinks: React.FC<ProfileLinksProps> = ({ configs, className }) => {
const identity = useIdentity();
const { profileLinks: globalConfigs } = useNameKitConfig();

const linksToRender = configs || globalConfigs || DEFAULT_PROFILE_LINKS;

if (!identity) {
console.warn("ProfileLinks used outside of Identity context");
return null;
}

return (
<div className={cc(["namekit-profile-links", className])}>
{linksToRender.map((config) => (
<ProfileLink key={config.getName()} config={config}>
{config.getName()}
</ProfileLink>
))}
</div>
);
};

export const Identity = {
Expand All @@ -311,5 +365,6 @@ export const Identity = {
Address,
NameGuardShield,
ProfileLink,
ProfileLinks,
Followers,
};