-
Notifications
You must be signed in to change notification settings - Fork 1
Update Dashboard UI #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
Conversation
Deploying wildcat-dashboard with
|
Latest commit: |
099660a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://da87dd1c.wildcat-dashboard.pages.dev |
Branch Preview URL: | https://stefan-update2.wildcat-dashboard.pages.dev |
Codecov ReportAttention: Patch coverage is 📢 Thoughts on this report? Let us know! |
src/components/nav/NavUser.tsx
Outdated
<AvatarImage src={user.avatar} alt={user.name} /> | ||
<AvatarFallback className="rounded-lg">{user.name}</AvatarFallback> | ||
<div | ||
className="w-full h-full flex items-center justify-center text-white font-semibold text-sm" |
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 may use the custom background color directly with the bg-
class utiltity to avoid creating extra variables and passing it down through properties, for example: bg-[#f59e0b]
className="w-full h-full flex items-center justify-center text-white font-semibold text-sm" | |
className="w-full h-full flex items-center justify-center text-white font-semibold text-sm bg-[#f59e0b]" |
src/pages/balances/BalancesPage.tsx
Outdated
unit: string | ||
} | ||
|
||
export function BalanceText({ amount, unit, children }: PropsWithChildren<{ amount: string; unit: string }>) { |
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.
Use the BalanceDisplay
here to keep consistency
export function BalanceText({ amount, unit, children }: PropsWithChildren<{ amount: string; unit: string }>) { | |
export function BalanceText({ amount, unit, children }: PropsWithChildren<BalanceDisplay>) { |
src/pages/balances/BalancesPage.tsx
Outdated
function PageBody() { | ||
const { data } = useSuspenseQuery({ | ||
queryKey: ["balances"], | ||
queryFn: fetchBalances, | ||
function isErrorResponse<T>(res: { data?: T; error?: unknown }): res is { data: undefined; error: unknown } { | ||
return res.error !== 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.
Is there a special reason to stop using tanstack query here? If it's suspense mode, I understand that it may be tricky to handle multiple queries at once with it, but you could still use the useQueries
hook to call multiple functions at once, this way you wouldn't have to handle all the individual state updates and errors, and could still access the status imperatively with isLoading
, isError
, isSuccess && data
, etc.
For reference:
https://tanstack.com/query/latest/docs/framework/react/reference/useQueries
src/pages/balances/BalancesPage.tsx
Outdated
if (creditResponse.status === "fulfilled" && !isErrorResponse(creditResponse.value)) { | ||
const creditData = creditResponse.value.data | ||
if (creditData && "amount" in creditData && "unit" in creditData) { | ||
newBalances.credit = { | ||
amount: String(creditData.amount), | ||
unit: String(creditData.unit), | ||
} | ||
} | ||
} else { | ||
console.warn( | ||
"Failed to fetch credit balance:", | ||
creditResponse.status === "rejected" ? creditResponse.reason : creditResponse.value.error, | ||
) | ||
} | ||
|
||
// Handle debit balance response | ||
if (debitResponse.status === "fulfilled" && !isErrorResponse(debitResponse.value)) { | ||
const debitData = debitResponse.value.data | ||
if (debitData && "amount" in debitData && "unit" in debitData) { | ||
newBalances.debit = { | ||
amount: String(debitData.amount), | ||
unit: String(debitData.unit), | ||
} | ||
} | ||
} else { | ||
console.warn( | ||
"Failed to fetch debit balance:", | ||
debitResponse.status === "rejected" ? debitResponse.reason : debitResponse.value.error, | ||
) | ||
} | ||
|
||
if (onchainResponse.status === "fulfilled" && !isErrorResponse(onchainResponse.value)) { | ||
const onchainData = onchainResponse.value.data | ||
if (onchainData && typeof onchainData === "object" && "confirmed" in onchainData) { | ||
console.log("Onchain balance:", onchainData.confirmed) | ||
newBalances.bitcoin = { | ||
amount: String(onchainData.confirmed), | ||
unit: "BTC", | ||
} | ||
} | ||
} else { | ||
console.warn( | ||
"Failed to fetch onchain balance:", | ||
onchainResponse.status === "rejected" ? onchainResponse.reason : onchainResponse.value.error, | ||
) | ||
} | ||
|
||
setBalances(newBalances) | ||
} catch (error) { | ||
const errorMessage = error instanceof Error ? error.message : "Unknown error occurred" | ||
setError(errorMessage) | ||
console.error("Failed to fetch balances:", error) | ||
} finally { | ||
setIsLoading(false) | ||
} | ||
}, []) |
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 tend to think this is too complicated, because it has several statuses blocks to handle. Let me know if you found @tanstack/react-query
not handy in here
src/pages/balances/BalancesPage.tsx
Outdated
useEffect(() => { | ||
void fetchAllBalances() | ||
|
||
const interval = setInterval(() => void fetchAllBalances(), 30000) |
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.
All good here, just a suggestion to separate the decimals here for better readability xD
const interval = setInterval(() => void fetchAllBalances(), 30000) | |
const interval = setInterval(() => void fetchAllBalances(), 30_000) |
src/pages/quotes/QuotePage.tsx
Outdated
function getInitials(name?: string): string { | ||
if (!name) return "?" | ||
const words = name.trim().split(/\s+/) | ||
if (words.length === 1) { | ||
return words[0].substring(0, 2).toUpperCase() | ||
} | ||
return (words[0][0] + words[words.length - 1][0]).toUpperCase() | ||
} |
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 move this to it's own utils file? Not sure if it's an early optimization tho, please tell me. Also, we have this very same utility with a cleaner functional approach in our frontend repo:
https://github.com/BitcreditProtocol/E-Bill-frontend/blob/main/src/utils/strings.ts#L11
We have other util functions there too, so please feel free to use as many of those as you want 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.
Very clean, I ended up stealing it.
Only a few things and quick improvements suggestions. The major thing is the use of tanstack query that has been reduced in here. It works with anything wrapped in a Promise, so you wouldn't have to handle all the individual statuses (fulfilled, pending, rejection, etc) manually. I think it's worth the time to try and learn the basics of it to keep redundancy minimal: Excellent job! |
@cryptosalomao Thank you for the review effort! I will take a look at tanstack, indeed my unfamiliarity with it (and react) might be overcomplicating things! |
You're welcome! I'm sure you'll find it easy after giving it a try. Let me know if you need extra help and want to jump on a call to address any questions you may have |
📝 Description
✅ Checklist
Please ensure the following tasks are completed before requesting a review:
npm run lint
or the equivalent linting command.