Skip to content
This repository was archived by the owner on Jun 11, 2024. It is now read-only.

Use Tanstack's query to fetch data + NFTs in the explorer#210

Merged
mvaivre merged 132 commits into
masterfrom
not-redux
Aug 4, 2023
Merged

Use Tanstack's query to fetch data + NFTs in the explorer#210
mvaivre merged 132 commits into
masterfrom
not-redux

Conversation

@mvaivre

@mvaivre mvaivre commented Jun 24, 2023

Copy link
Copy Markdown
Member

Fixes #161 #213

@mvaivre
mvaivre temporarily deployed to github-pages July 31, 2023 08:33 — with GitHub Actions Inactive
@mvaivre
mvaivre temporarily deployed to github-pages August 1, 2023 12:43 — with GitHub Actions Inactive
@mvaivre
mvaivre temporarily deployed to github-pages August 1, 2023 12:51 — with GitHub Actions Inactive

@nop33 nop33 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm very much happier with the changes, good job! 👏 A few more small comments.

Comment on lines +35 to +38
const replaced2 = replaced1.replace(
/__VITE_NODE_URL__/g,
`"${process.env.VITE_NODE_URL ?? 'http://localhost:22973'}"`
)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: I guess all these replace could simply be chained, but I don't mind much.

Comment thread package.json
"@lukemorales/query-key-factory": "^1.3.2",
"@tanstack/react-query": "^4.29.15",
"apexcharts": "^3.35.0",
"axios": "^1.4.0",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It still seems to be here :)

one: (assetId: string) => ({
queryKey: ['assetType', assetId],
queryFn: (): Promise<AssetBase> => client.node.guessStdTokenType(assetId).then((r) => ({ id: assetId, type: r })),
staleTime: ONE_DAY_MS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not infinity anymore?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because it's now persisted in the local storage. I prefer to be on the safe side and refresh everyday.

})

if (isAlphIn) {
verifiedTokensMetadata.unshift({ ...ALPH, type: 'fungible', verified: true })

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: How about creating a small util object? It can be used in both places in this file.

const alphMetadata = { ...ALPH, type: 'fungible', verified: true } as VerifiedFungibleTokenMetadata

Comment thread src/components/Amount.tsx
if (value !== undefined) {
amount = getAmount({ value, isFiat, decimals, nbOfDecimalsToShow, fullPrecision })

if (fadeDecimals && ['K', 'M', 'B', 'T'].some((char) => amount.endsWith(char))) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess we need to update the list with the one from the SDK now that we added more chars!

along with the library. If not, see <http://www.gnu.org/licenses/>.
*/

/* eslint-disable @typescript-eslint/no-explicit-any */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see any anys 😝

Comment on lines +76 to +82
unsorted.unshift({
...ALPH,
type: 'fungible',
balance: BigInt(addressBalance.balance),
lockedBalance: BigInt(addressBalance.lockedBalance),
verified: true
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the alphMetadata I proposed above could also be used here!

const [isHovered, setIsHovered] = useState(false)

const desc = nft.file?.description
const cutDesc = desc && desc?.length > 200 ? nft.file?.description?.substring(0, 200) + '...' : desc

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did you have a look into it?

const mempoolTxs = await client.addresses.getAddressesAddressMempoolTransactions(id)
const { data: addressBalance } = useQuery({
...addressQueries.balance.details(addressHash),
enabled: !!addressHash

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we add a TODO so that this comment doesn't get forgotten?

const mempoolTxs = await client.addresses.getAddressesAddressMempoolTransactions(id)
const { data: addressBalance } = useQuery({
...addressQueries.balance.details(addressHash),
enabled: !!addressHash

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't it be useful to add the address validity check in the enabled flags of each query so that we avoid spamming the server with unnecessary requests when the address is invalid? Or is it too much for now?

@mvaivre

mvaivre commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

Thanks @nop33 !

If none of your comment is a blocker in your opinion, I'd be in favour of merging this huge PR now and start a new one for further improvements. I've compiled the proposed improvements here.

@mvaivre
mvaivre merged commit 0b1797a into master Aug 4, 2023
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.

Show addresses' assets (tokens & NFTs)

3 participants