feat: Add utility code for NFT Features #4256

cbachmeier · 2022-08-03T01:41:16Z

Adds in the utility code for NFT Features including:

dependencies
styles
hooks
queries
types
util functions

vercel · 2022-08-03T01:41:19Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
interface	✅ Ready (Inspect)	Visit Preview	Aug 10, 2022 at 11:25PM (UTC)

…face-repository

vm · 2022-08-03T15:38:13Z

@zzmp will own reviewing this PR

zzmp

The pngs and svgs should be added with the appropriate PR, not as part of utility code, unless they are directly referenced by utility code.
The ttfs are redundant - we already include Inter in public/fonts/Inter-roman.var.woff2.
privacy.pdf should probably not be included. I don't see anything linking to it.

(these are just initial comments - I'll give this a more thorough review tomorrow AM)

craco.config.cjs

zzmp

A lot of the packages that you've added are not yet used, or are already used implicitly and do not need to be explicitly added. They should not be added until they are actually used, even if it makes the subsequent PRs a little more cumbersome. Adding them all at once risks adding no-longer used packages, and makes it harder to review and ensure that our dependencies are being pinned correctly (and that no malicious dependencies or version skew gets through).

Please remove these packages from the package.json, and then re-checkout the lockfile and re-install the files to regenerate it: git checkout main yarn.lock.

@babel/runtime
@testing-library/react-hooks
@types/react-table
tslib
@react-spring/web
@use-gesture/react
color.js
deepmerge
html2canvas
overlayscrollbars
overlayscrollbars-react
query-string
react-country-flag
react-error-overlay
react-infinite-scroll-component
react-merge-refs
react-meta-tags
react-player
react-select
react-tilty

package.json

src/index.tsx

package.json

…ments

zzmp

I've still got to go through src/nft/utils/ and the yarn.lock.

src/lib/hooks/useCurrencyBalance.ts

zzmp · 2022-08-04T13:09:38Z

src/lib/hooks/useCurrencyBalance.ts

@@ -141,3 +141,6 @@ export default function useCurrencyBalance(
    useMemo(() => [currency], [currency])
  )[0]
 }
+
+export const useCurrencyBalanceString = (account: string): string =>


Following other naming conventions in this file, this should be useNativeCurrencyBalance(account: string): CurrencyAmount<Currency> | undefined.

You should defer converting it to a string until it is used, so that it is passed around as a more usable CurrencyAmount.

This utility function was named and recommended by Connor in this PR https://github.com/Uniswap/interface-nft/pull/4#discussion_r907469235

craco.config.cjs

src/connection/index.ts

zzmp · 2022-08-05T07:55:07Z

src/nft/css/atoms.ts

+  reset?: keyof JSX.IntrinsicElements
+}
+
+export const atoms = ({ reset, ...rest }: Atoms) => {


Document what reset does.

It's used by the Box component when declaring the component as something other than a div. It's should only be used for that specific purpose https://github.com/Uniswap/interface-nft/blob/main/src/nft/components/Box.ts

src/nft/hooks/useToggle.ts

src/nft/hooks/useWalletBalance.ts

src/nft/hooks/useWindowDimensions.ts

src/nft/hooks/useWrongNetwork.ts

src/nft/types/index.ts

zzmp

A lot of the utils use hard-coded english strings. What is the plan for i18n/l10n?
Will nft features use lingui? Have they been using something else?

src/nft/utils/address.ts

src/nft/utils/calcPoolPrice.ts

src/nft/utils/chains.ts

zzmp · 2022-08-05T11:46:32Z

src/nft/utils/date.ts

+  return isNaN(d) ? false : true
+}
+
+export const getTimeDifference = (eventTimestamp: string) => {


This is not internationalized. Why not use a library for this?

We have no yet started the work for internationalization for NFT components. For procedurally generated text such as getting time difference in either days, hours, or minutes is there a standard practice? When it is tackled which I believe is outside of the scope of this PR, should we return a Trans component in this case?

Either return a trans component, or use a library which deals with internationalizing date/time strings. I believe we already use such a library in places like governance.

zzmp · 2022-08-05T11:47:40Z

src/nft/utils/fetchPrice.ts

+
+export const fetchPrice = async (currency: Currency = Currency.ETH): Promise<number | undefined> => {
+  try {
+    const response = await fetch(`https://api.coinbase.com/v2/exchange-rates?currency=${currency}`)


I think this is fine, but it seems strange to go to coinbase for this price when the global app checks it from uniswap.

This is how we retrieved current price on Genie. For the sake of V0 and the scope of this ticket would it be fine if we keep this and add it as tech debt to replace and refactor where it is used?

yes - you should make a low-priority jira ticket to document the tech debt (same with i18n work).

src/nft/utils/gtag.tsx

src/nft/utils/listNfts.ts

zzmp · 2022-08-05T11:50:06Z

src/nft/utils/roundAndPluralize.ts

@@ -0,0 +1,5 @@
+export const roundAndPluralize = (i: number, word: string) => {


This doesn't work with internationalization.

We'll be tackling internationalization in a later sprint. When we do, will we not be able to have any kind of generalized behaviour helper function here or will we just need to use a conditional for the full word?

you will not be able to use a helper function, you'll need to use the i18n libraries (lingui) to deal with pluralization.

zzmp · 2022-08-05T11:50:31Z

src/nft/utils/timeSince.ts

@@ -0,0 +1,24 @@
+import { roundAndPluralize } from './roundAndPluralize'


This doesn't work with i10n.

We'll be tackling internationalization in a later sprint.

zzmp · 2022-08-05T12:05:06Z

The lockfile changes were too large to comment on, so I updated them locally and pushed the changes to this branch.

…face-repository

src/connection/index.ts

src/hooks/useImperativeDisableScroll.ts

src/nft/hooks/useIsMobile.ts

src/nft/hooks/useSellAsset.ts

src/nft/queries/genie/CollectionStatsFetcher.ts

zzmp · 2022-08-10T17:24:40Z

src/nft/utils/buildSellObject.ts

@@ -0,0 +1,16 @@
+export const buildSellObject = (amount: string) => {
+  return {
+    address: '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee',


What is this address?

It's effectively the same as the null address and it's what the genieswap contract uses for buying with ETH. @azflin might be able to discuss this further if you have more questions on it.

src/nft/utils/chains.ts

zzmp

Can you fix the issues the linter brought up before merging?

src/nft/queries/genie/RouteFetcher.ts#L15 (!== instead of !=)
src/nft/queries/openSea/OSCollectionsFetcher.ts#L7 (=== instead of ==)

Charles Bachmeier added 2 commits August 2, 2022 18:32

import nft utilities

ee9f03d

re-add featureflag provider

d8e057c

cbachmeier requested review from vm, zzmp, lynnshaoyu and cmcewen August 3, 2022 01:42

vercel bot deployed to Preview August 3, 2022 01:43 View deployment

Merge branch 'main' into WEB-687-merge-utility-code-in-the-main-inter…

75c65e2

…face-repository

vercel bot deployed to Preview August 3, 2022 02:07 View deployment

vm removed their request for review August 3, 2022 15:38

add back in useCurrencyBalanceString fn

99b4d6f

vercel bot deployed to Preview August 3, 2022 17:57 View deployment

zzmp suggested changes Aug 3, 2022

View reviewed changes

craco.config.cjs Outdated Show resolved Hide resolved

remove static assets for separate pr

483a616

cbachmeier requested a review from zzmp August 3, 2022 21:45

vercel bot deployed to Preview August 3, 2022 21:47 View deployment

zzmp suggested changes Aug 4, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/index.tsx Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

vm mentioned this pull request Aug 4, 2022

chore: Revert "chore: add craco and vanilla extract libraries (#4100)" #4269

Merged

Charles Bachmeier added 2 commits August 4, 2022 11:04

remove resolutions, swap dev and regular dependencies, respond to com…

c614304

…ments

remove currently unused dependencies and resynth .lock

6db9608

cbachmeier requested a review from zzmp August 4, 2022 18:11

vercel bot deployed to Preview August 4, 2022 18:19 View deployment

cbachmeier mentioned this pull request Aug 4, 2022

feat: Phase 0 NavBar #4285

Closed

zzmp suggested changes Aug 5, 2022

View reviewed changes

zzmp added 2 commits August 5, 2022 13:54

build: update lockfile

bfbb146

build: update lockfile

a67d353

gtag removed

3d0a55d

cbachmeier requested a review from zzmp August 9, 2022 05:33

vercel bot deployed to Preview August 9, 2022 05:37 View deployment

vercel bot deployed to Preview August 10, 2022 00:10 View deployment

Merge branch 'main' into WEB-687-merge-utility-code-in-the-main-inter…

d5c9f21

…face-repository

cbachmeier force-pushed the WEB-687-merge-utility-code-in-the-main-interface-repository branch from 7db6fed to d5c9f21 Compare August 10, 2022 00:21

vercel bot deployed to Preview August 10, 2022 00:24 View deployment

cbachmeier closed this Aug 10, 2022

cbachmeier reopened this Aug 10, 2022

Merge branch 'main' into WEB-687-merge-utility-code-in-the-main-inter…

c85273a

…face-repository

vercel bot deployed to Preview August 10, 2022 00:33 View deployment

Charles Bachmeier added 2 commits August 9, 2022 17:36

revert

0a29726

revert

7780756

vercel bot deployed to Preview August 10, 2022 00:44 View deployment

build: update lockfile

63faf5b

vercel bot had a problem deploying to Preview August 10, 2022 17:05 Failure

zzmp suggested changes Aug 10, 2022

View reviewed changes

Charles Bachmeier added 5 commits August 10, 2022 13:49

remove walletinfo interface

57392e2

remove newline

d7e12ff

remove tslinst exception from isMobile

df4eef9

remove hiding linter warnings

d6abda2

remove unused util

83d8d6c

cbachmeier requested a review from zzmp August 10, 2022 21:45

vercel bot deployed to Preview August 10, 2022 21:48 View deployment

zzmp approved these changes Aug 10, 2022

View reviewed changes

fix linter warnings

9d32821

vercel bot deployed to Preview August 10, 2022 23:25 View deployment

cbachmeier merged commit 8dbc91e into main Aug 10, 2022

cbachmeier deleted the WEB-687-merge-utility-code-in-the-main-interface-repository branch August 10, 2022 23:38

felipebrahm mentioned this pull request Nov 17, 2022

chore: remove unused file #5269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add utility code for NFT Features #4256

feat: Add utility code for NFT Features #4256

cbachmeier commented Aug 3, 2022 •

edited

vercel bot commented Aug 3, 2022 •

edited

vm commented Aug 3, 2022

zzmp left a comment

zzmp left a comment

zzmp left a comment

zzmp Aug 4, 2022

cbachmeier Aug 5, 2022

zzmp Aug 5, 2022

cbachmeier Aug 5, 2022

zzmp left a comment

zzmp Aug 5, 2022

cbachmeier Aug 9, 2022

zzmp Aug 10, 2022

zzmp Aug 5, 2022

cbachmeier Aug 9, 2022 •

edited

zzmp Aug 10, 2022

zzmp Aug 5, 2022

cbachmeier Aug 9, 2022

zzmp Aug 10, 2022

zzmp Aug 5, 2022

cbachmeier Aug 9, 2022

zzmp commented Aug 5, 2022

zzmp Aug 10, 2022

cbachmeier Aug 10, 2022

zzmp left a comment

		@@ -0,0 +1,5 @@
		export const roundAndPluralize = (i: number, word: string) => {

		@@ -0,0 +1,24 @@
		import { roundAndPluralize } from './roundAndPluralize'

feat: Add utility code for NFT Features #4256

feat: Add utility code for NFT Features #4256

Conversation

cbachmeier commented Aug 3, 2022 • edited

vercel bot commented Aug 3, 2022 • edited

vm commented Aug 3, 2022

zzmp left a comment

Choose a reason for hiding this comment

zzmp left a comment

Choose a reason for hiding this comment

zzmp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzmp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbachmeier Aug 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzmp commented Aug 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzmp left a comment

Choose a reason for hiding this comment

cbachmeier commented Aug 3, 2022 •

edited

vercel bot commented Aug 3, 2022 •

edited

cbachmeier Aug 9, 2022 •

edited