Skip to content
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.

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

Fix/ens name #422

Open
wants to merge 45 commits into
base: develop
Choose a base branch
from

Conversation

heronlancellot
Copy link
Member

Closes #320

Copy link

vercel bot commented Sep 19, 2024

@heronlancellot is attempting to deploy a commit to the Blockful Team on Vercel.

A member of the Team first needs to authorize it.

import { EthereumAddress } from "@/lib/shared/types";
import cc from "classcat";
import { useEffect, useState } from "react";
// /* eslint-disable react-hooks/exhaustive-deps */
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @heronlancellot

Whenever commenting out features from the project, can you please add a documentation at the top of the comments explaining this feature is out for a while?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • I'd rather using this comment methodology /** */

import { SwapContext } from "@/lib/client/contexts";
import { useContext, useEffect } from "react";
import { ENS } from "web3-eth-ens";
import Web3 from "web3";
// import { ENS } from "web3-eth-ens";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing around comment methodology in here

const { primaryName } = useEnsData({
ensAddress: authenticatedUserAddress,
});
// const { primaryName } = useEnsData({
Copy link
Contributor

Choose a reason for hiding this comment

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

And here!

});
// const { primaryName } = useEnsData({
// ensAddress: authenticatedUserAddress,
// });
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add docs above the comments (one per file is enough) saying why this feature is out for now

Copy link
Contributor

Choose a reason for hiding this comment

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

For each file, please

searchedENSName
? `${searchedENSName} offers`
: validatedAddressToSwap
// searchedENSName
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the other comment syntax

Copy link
Contributor

Choose a reason for hiding this comment

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

>
<ENSAvatar avatarENSAddress={authenticatedUserAddress} />
{/* <ENSAvatar avatarENSAddress={authenticatedUserAddress} /> */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here as well, adding a comment on why this is out

@@ -64,15 +64,16 @@ export const UserOfferInfo = ({
<div>
<div className="flex gap-2">
<div>
{address && (
{/* {address && (
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

@@ -7,7 +7,7 @@ import {

export const SwapSection = () => {
return (
<div className="max-w-[1280px] xl:max-h-[720px] w-full flex xl:flex-row flex-col lg:justify-center">
<div className="max-w-[1280px] xl:max-h-[720px] w-full flex xl:flex-row flex-col lg:justify-center xl:h-full">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already in another PR, shall update with merge

SUCCESS,
ERROR,
}
// export enum ENSAvatarQueryStatus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please refer to the above comments and apply the mentioned rules here!

components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
@@ -3,7 +3,7 @@ export * from "./ApproveTokenCards";
export * from "./BlockExplorerExternalLinkButton";
export * from "./ConnectWallet";
export * from "./DisconnectWallet";
export * from "./ENSAvatar";
// export * from "./ENSAvatar";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment in here as well

components/02-molecules/UserOfferInfo.tsx Outdated Show resolved Hide resolved
components/02-molecules/UserOfferInfo.tsx Outdated Show resolved Hide resolved
@@ -1,3 +1,23 @@
/**
* @deprecated This hook is deprecated because the searched ENS address
* sometimes returns incorrect results, which negatively impacts the user flow
Copy link
Contributor

Choose a reason for hiding this comment

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

Please replicate my suggestions above to the exact same text

* `ensAddress` changes. It updates the state variables `primaryName` and `avatarQueryStatus`
* based on the fetched data or any errors encountered during the fetch process.
*
* @param {Props} props - The properties object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not type the whole props object but each single prop

* based on the fetched data or any errors encountered during the fetch process.
*
* @param {Props} props - The properties object.
* @param {EthereumAddress | null} props.ensAddress - The ENS address to fetch the data for.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not mention props.

components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
components/01-atoms/index.ts Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
@RafaDSan RafaDSan assigned RafaDSan and heronlancellot and unassigned RafaDSan Sep 23, 2024
components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/ENSAvatar.tsx Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
components/02-molecules/EnsNameAndAddressWallet.tsx Outdated Show resolved Hide resolved
<div className="flex flex-col">
<div className="flex items-center justify-start gap-2">
{primaryName && (
{/* {primaryName && (
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment section can reproduce the method of commenting deprecated feature that the other sections received! Please go ahead and do so 🧑🏼‍💻👊🏼🧑🏼‍💻

components/02-molecules/OfferSummary.tsx Outdated Show resolved Hide resolved
heronlancellot and others added 22 commits September 24, 2024 10:47
components/01-atoms/SearchBar.tsx Outdated Show resolved Hide resolved
components/02-molecules/EnsNameAndAddressWallet.tsx Outdated Show resolved Hide resolved
Comment on lines 29 to 31
const { primaryName } = useEnsData({
ensAddress: authenticatedUserAddress,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is missing documentation on why it is commented out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🛠️ In Progress
Development

Successfully merging this pull request may close these issues.

3 participants