-
Notifications
You must be signed in to change notification settings - Fork 1
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
ENG-4849 feat(portal): replace combobox search in create claim with graphql equivalent #945
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.
Great job! A few comments for our future selves -- one about being able to use the useGetConnectionsCountQuery
for the stats and another about our utils.
@@ -45,6 +45,7 @@ fragment AtomVaultDetails on atoms { | |||
vault { | |||
positionCount | |||
totalShares | |||
currentSharePrice |
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 addition!
@@ -7510,6 +7510,7 @@ export type AtomVaultDetailsFragment = { | |||
__typename?: 'vaults' | |||
positionCount: number | |||
totalShares: any | |||
currentSharePrice: any |
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.
We still have a good bit of any
in our code gen types. As we get closer to migration we'll have to see what we need to tighten up at the schema level.
@@ -427,6 +428,106 @@ export const getAtomId = (atom: IdentityPresenter) => { | |||
return atom.identity_id | |||
} | |||
|
|||
// atom GQL helpers | |||
export const getAtomImageGQL = ( |
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.
Amazing! I started adding some of these in the branch for Explore (my open PR). We can streamline once both are merged in and see if there are duplicates.
@@ -0,0 +1,12 @@ | |||
import { useEffect, useState } from 'react' |
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.
Great! If we decide we want to adopt using lodash
in the future for our utils we can update to use lodash's debounce but this is great until then.
avatarSrc={getAtomImageGQL(selectedIdentity)} | ||
name={getAtomLabelGQL(selectedIdentity)} | ||
id={getAtomIdGQL(selectedIdentity)} | ||
stats={undefined} // TODO: add stats when available |
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.
This is available on another query. I split the queries out for revalidation and optimization purposes. It can be obtained via useGetConnectionsCountQuery
and passed in.
It's an aggregate query so it's pretty quick. We can update that in a future PR though
|
||
const { data: atomsData, isLoading } = useGetAtomsQuery( | ||
{ | ||
where: { |
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.
Great! I'm glad this is working -- thanks for pulling the debounce into it's own hook. This is how I approached it in the playground tests so I'm glad it works and that you abstracted it into a hook!
Affected Packages
Apps
Packages
Tools
Overview
Screen Captures
https://www.loom.com/share/450e9ce606f44e9caaf785995035caa5?sid=40b26455-6a42-467c-97b8-842b10f8e739
Declaration