-
Notifications
You must be signed in to change notification settings - Fork 43
refactor: collateral zustand to tanstack #1423
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
chartExpanded={chartExpanded} | ||
page="create" | ||
/> | ||
{market && ( |
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.
Why is this added? It results in the page jumping during load
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.
For some reason the old code type asserted that llamma couldn't be undefined
. But I'm not sure if that assertion is valid. How can it be asserted to not be null when stuff is being loaded? Either the assertion is somehow true and this won't jump, or stuff was being shown prematurely in the old verison. I'd rather keep this check to make sure there's no errors. If people never complained about the page jumping it's probably a safe assumption?
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.
People dont complain because it works, it just looks unprofessional
The base branch was changed.
fc7f25d
to
975d5ed
Compare
975d5ed
to
ebf681d
Compare
ebf681d
to
2804725
Compare
return useMemo(() => { | ||
if (!api) return undefined | ||
|
||
// If markets aren't found they throw an error, but we want to return undefined instead |
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 please, ignoring errors is asking for tricky bugs.
If you want to catch invalid market IDs please check the exact exception and return a proper error state, not undefined
Depends on #1422
Focus is on removing stores in favor of tanstack queries & hooks, not whether legacy component make sense. In a few cases I've renamed
llama
orllamma
tomarket
for simplicity.