-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: ⚡️ Ukraine relief feed #45
Conversation
Your Render PR Server URL is https://teia-ui-pr-45.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c8ul090ivq06pjq9rmh0. |
@xat It's not super clean because I could not tailor the request only in graphQL, to get only the tokens array based on the factors I used (supply >1, verified, Shares > 50% for the relief contract). The js part might surely be better but I think it's a good starting point |
be56fe6
to
0950b05
Compare
f8aab20
to
751eea2
Compare
src/data/hicdex.js
Outdated
) | ||
|
||
if (errors) { | ||
console.error(errors) |
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.
maybe throw or return here, because the code that follows expects a data
object and will fail if it's undefined.
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.
Yes tbh I copied the other methods but I usually try/catch those. We could also use the graph-ql package, it's very nice, I'm actually working on a little library wrapper that use most of the request we make here, I will propose it once I'm done :)
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.
Hmmm how are we handling them then? I never seem to be able to hit an error (providing a wrong address for instance).
I think this is because how it's handled in fetchGraphQL
I think it's out of scope for this issue
src/data/hicdex.js
Outdated
} | ||
return undefined | ||
}), | ||
(token) => token !== undefined |
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.
isn't this tokens
? (plural?)
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.
You mean the last one? It's to filter undefined token (singular) in the tokens array
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.
yes, the last one. hmm, in the map function above you are returning contract.tokens
(plural).
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.
at least to me, it looks like the const tokens
from line 261 contains an array of arrays.
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.
True there might be better, but I'm deconstructing it line 274
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.
Or are you just referring to renaming the variable token to token_list for instance line 271? 🤣
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.
yes, it's technically correct. just the variable name token
could be changed to tokens
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.
Ahahaha! Will do those edits
src/data/hicdex.js
Outdated
|
||
objkts = objkts.concat(...tokens) | ||
} | ||
objkts = _.orderBy(objkts, ['timestamp'], ['desc']) |
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.
not that it matters, but you could directly return 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.
nicely done!
Closes #43