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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Nomspace integration #621

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add Nomspace integration #621

wants to merge 4 commits into from

Conversation

Zhethan
Copy link

@Zhethan Zhethan commented Feb 16, 2022

No description provided.

@vercel
Copy link

vercel bot commented Feb 16, 2022

@Zhethan is attempting to deploy a commit to the Sushi Team on Vercel.

A member of the Team first needs to authorize it.

package.json Outdated
@@ -211,5 +211,9 @@
"not ie 11",
"not dead",
"not op_mini all"
]
],
"dependencies": {

Choose a reason for hiding this comment

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

Does it work to move these deps to devDependencies? Let's avoid adding stuff to dependencies considering their team hasn't added anything there

@@ -82,7 +84,13 @@ const AccountDetails: FC<AccountDetailsProps> = ({
/>
</div>
<Typography weight={700} variant="lg" className="text-white">
{ENSName ? ENSName : account && shortenAddress(account)}
{nom && ENSName

Choose a reason for hiding this comment

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

Let's do it in this priority:

ENSName ? ENSName : nom ? nom : account && shortenAddress(account)

@@ -127,7 +127,7 @@ function Web3StatusInner() {
</div>
) : (
<div className="flex items-center gap-2">
<div>{ENSName || shortenAddress(account)}</div>
<div>{ENSName ? ENSName : nom ? shortenAddress(account) + ' (' + nom + ')' : shortenAddress(account)}</div>

Choose a reason for hiding this comment

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

I think it's probably ok to just do nom instead of shortenAddress(account) + ' (' + nom + ')'

@@ -42,7 +42,16 @@ const HeaderDropdown: FC<HeaderDropdownProps> = ({ account, hideAccount = false
className="text-high-emphesis flex gap-1 cursor-pointer hover:text-blue-100"
weight={700}
>
{ENSName ? ENSName : account ? shortenAddress(account) : ''} <LinkIcon width={20} />
{nom && ENSName

Choose a reason for hiding this comment

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

ditto with last similar comment

@@ -1,21 +1,29 @@
// @ts-ignore

Choose a reason for hiding this comment

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

Don't use @ts-ignore. Add @ensdomains/ensjs declaration to a .d.ts file

Copy link

@BriungRi BriungRi left a comment

Choose a reason for hiding this comment

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

Please address comments

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

Successfully merging this pull request may close these issues.

None yet

2 participants