-
Notifications
You must be signed in to change notification settings - Fork 635
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
Claimables: query + wallet screen ui #6071
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nitpicks, but lgtm
src/components/asset-list/RecyclerAssetList2/ClaimablesListHeader.tsx
Outdated
Show resolved
Hide resolved
src/components/asset-list/RecyclerAssetList2/ClaimablesListHeader.tsx
Outdated
Show resolved
Hide resolved
src/components/asset-list/RecyclerAssetList2/ClaimablesListHeader.tsx
Outdated
Show resolved
Hide resolved
…@benisgold/claimables-wallet-screen
…@benisgold/claimables-wallet-screen
} | ||
); | ||
|
||
const [claimable] = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we planning to have multiple claimables here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
if (data.metadata.status !== 'ok') { | ||
logger.error(new RainbowError('[userAssetsQueryFunction]: Failed to fetch user assets (API error)'), { | ||
message: data.metadata.errors, | ||
}); | ||
} | ||
|
||
return parseClaimables(data.payload.claimables, currency); | ||
} catch (e) { | ||
logger.error(new RainbowError('[userAssetsQueryFunction]: Failed to fetch user assets (client error)'), { | ||
message: (e as Error)?.message, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when data.metadata.status !== 'ok'
will we have a data.payload.claimables
? just to be sure it wont log like (API error)
and then right after (client error)
because it couldn't access payload.claimables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question, i believe data.payload.claimables
should always be at least an empty array, according to the addys backend types
usePositions({ address: accountAddress, currency: nativeCurrency }); | ||
useClaimables({ address: accountAddress, currency: nativeCurrency }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add { notifyOnChangeProps: [] }
to these queries to avoid rerendering this whole screen when it refetches or is it supposed to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think it triggers a rerender bc the return value of useClaimables
is not stored
…@benisgold/claimables-wallet-screen
Fixes APP-####
What changed (plus any additional context for devs)
built out react query infra for addys claimables endpoint + wallet screen ui. locked behind feature flag
also created
resources/addys
directory, would like to clean up positions/assets queries in the future and consolidate them here since they have overlapping types bc they all rely on addysScreen recordings / screenshots
What to test
you can check figma here: https://www.figma.com/design/BqH9Y7pC6zOjO3UrwTBTbL/claimables?node-id=0-1&m=dev