Skip to content
This repository has been archived by the owner on Sep 16, 2022. It is now read-only.

Migrating address component #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stevenpslade
Copy link
Member

Part of Migrate Components and TS-ify

This PR is migrating the Address component from eth-components and Typescript-ifying it.

const blockExplorerLink = (address: string, blockExplorer?: string): string =>
`${blockExplorer || 'https://etherscan.io/'}address/${address}`;

export const useAddress = (props: AddressProps): AddressResult => {
Copy link
Member

Choose a reason for hiding this comment

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

I would add tsdocs comments similar to eth-hooks. It helps create automated documentation later. https://scaffold-eth.github.io/eth-hooks/docs/api/modules/Hooks. @stevenpslade

import { useResolveEnsName } from 'eth-hooks/dapps';
import { TEthersProvider } from 'eth-hooks/models';

export interface AddressProps {
Copy link
Member

Choose a reason for hiding this comment

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

Can we prefix interfaces with I and types with T. it was the standard in eth hooks

const [ensName] = useResolveEnsName(props.ensProvider, address);
const explorerLink = blockExplorerLink(address, props.blockExplorer);

const shortAddress = address ? `${address.substring(0, 5)}...${address.substring(address.length - 4)}` : '';
Copy link
Member

Choose a reason for hiding this comment

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

i think we should check validity of address using ethers.utils

Copy link
Member

Choose a reason for hiding this comment

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

if invalid, return that in shortAddress

const shortAddress = address ? `${address.substring(0, 5)}...${address.substring(address.length - 4)}` : '';

return {
shortAddress,
Copy link
Member

Choose a reason for hiding this comment

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

shortAddress can be called addressForDisplay

const blockExplorerLink = (address: string, blockExplorer?: string): string =>
`${blockExplorer || 'https://etherscan.io/'}address/${address}`;

export const useAddress = (props: AddressProps): AddressResult => {
Copy link
Member

Choose a reason for hiding this comment

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

maybe the hook should be called useAddressForDisplay?

Copy link
Member

@ShravanSunder ShravanSunder left a comment

Choose a reason for hiding this comment

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

The test should mock all the eth-hooks hooks and check if it works. maybe one case for invalid address, undefined address and valid address.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants