Skip to content

Fix/ens name#422

Open
heronlancellot wants to merge 45 commits intoblockful:developfrom
heronlancellot:fix/ens-name
Open

Fix/ens name#422
heronlancellot wants to merge 45 commits intoblockful:developfrom
heronlancellot:fix/ens-name

Conversation

@heronlancellot
Copy link
Copy Markdown
Contributor

Closes #320

@vercel
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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 { useContext, useEffect } from "react";
import { ENS } from "web3-eth-ens";
import Web3 from "web3";
// import { ENS } from "web3-eth-ens";
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

className="rounded-[10px] flex items-center justify-center w-full h-full"
>
<ENSAvatar avatarENSAddress={authenticatedUserAddress} />
{/* <ENSAvatar avatarENSAddress={authenticatedUserAddress} /> */}
Copy link
Copy Markdown
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

<div className="flex gap-2">
<div>
{address && (
{/* {address && (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And here

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
Copy Markdown
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
Copy Markdown
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!

export * from "./ConnectWallet";
export * from "./DisconnectWallet";
export * from "./ENSAvatar";
// export * from "./ENSAvatar";
Copy link
Copy Markdown
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

@@ -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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

heronlancellot and others added 8 commits September 20, 2024 09:11
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
heronlancellot and others added 4 commits September 20, 2024 10:58
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
heronlancellot and others added 3 commits September 20, 2024 11:03
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
@RafaDSan RafaDSan assigned RafaDSan and heronlancellot and unassigned RafaDSan Sep 23, 2024
<div className="flex flex-col">
<div className="flex items-center justify-start gap-2">
{primaryName && (
{/* {primaryName && (
Copy link
Copy Markdown
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 🧑🏼‍💻👊🏼🧑🏼‍💻

heronlancellot and others added 2 commits September 24, 2024 10:47
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
heronlancellot and others added 22 commits September 24, 2024 10:47
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Comment on lines 29 to 31
const { primaryName } = useEnsData({
ensAddress: authenticatedUserAddress,
});
Copy link
Copy Markdown
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

heronlancellot and others added 2 commits September 24, 2024 16:08
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
Co-authored-by: caco.eth <49823133+FrancoAguzzi@users.noreply.github.com>
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