Skip to content
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

5. Get the check-in count from the BatchRegistry contract #6

Closed
edakturk14 opened this issue Jul 27, 2024 · 8 comments · Fixed by #25
Closed

5. Get the check-in count from the BatchRegistry contract #6

edakturk14 opened this issue Jul 27, 2024 · 8 comments · Fixed by #25
Labels
frontend frontend work good first issue Good for newcomers

Comments

@edakturk14
Copy link
Contributor

edakturk14 commented Jul 27, 2024

Goal

There is a "To Be implemented" string in packages/nextjs/app/page.tsx that should show the number of builders that have checked in to the contract.

We want to show the real count (reading from the BatchRegistry contract)

For more info about the contract: #10

@edakturk14 edakturk14 added the frontend frontend work label Jul 27, 2024
@edakturk14 edakturk14 changed the title 4. Tweak the main page Get the check-in count from the BatchRegistry contract Aug 5, 2024
@edakturk14 edakturk14 changed the title Get the check-in count from the BatchRegistry contract 5. Get the check-in count from the BatchRegistry contract Aug 5, 2024
@edakturk14 edakturk14 added the good first issue Good for newcomers label Aug 5, 2024
@RafaelCaso
Copy link
Contributor

working on this

@RafaelCaso
Copy link
Contributor

RafaelCaso commented Aug 12, 2024

My proposed solution for this issue. Would love to get other batch members' input!
Screen Shot 2024-08-12 at 5 06 35 PM

@RafaelCaso
Copy link
Contributor

This should take advantage of the isLoading state returned by useScaffoldReadContract. That would eliminate the need for the isNan() check

@indralukmana
Copy link
Contributor

@RafaelCaso maybe we can do it like this?

const result = isLoading ? "..." : Number(checkedIn);

I'm still not sure if checkedIn would always result in number or can be NaN if the loading is done (true). One pitfall I'm thinking if we are using ... for conflating the result of loading and NaN checking of checkedIn value we would miss a possible error when the loading is done and the checkedIn is NaN. Maybe it is fine to just show NaN or maybe add some checking and change the result (making it a let) when the checkedIn is actually a NaN.

with the above we can simplify the codes for showing the span in the screenshot from line 13 to 17 into

<span className="font-bold">Checked in builders count: {result}</span>

@RafaelCaso
Copy link
Contributor

@indralukmana

yeah, replacing Number.isNaN(result) with isLoading makes more sense to me.

if checkedIn fails it would be because of an error from the block chain, no? So maybe we would add a catch block and return a simple "Something wen't wrong, please try again" kind of message?

@indralukmana
Copy link
Contributor

@RafaelCaso
I'm honestly not sure what kind of error would result there, haven't dig in into the inner details of ScaffoldETH hooks yet maybe from blockchain or provider/rpc or something else entirely.

Maybe you can write and do PR then ask your questions in the PR and people who are more knowledgeable can give more insights. It is kind of hard not having real codes to check and do stuff 😁

@derrekcoleman
Copy link
Member

@RafaelCaso and @indralukmana, I definitely appreciate the conversation about error handling! We want users to have a good experience, even when things go wrong.

if checkedIn fails it would be because of an error from the block chain, no?

useScaffoldReadContract is a wrapper around wagmi's useReadContract, which has return fields related to possible errors and failureReasons. These are definitely worth learning as a FE developer reading blockchain data!

So maybe we would add a catch block and return a simple "Something wen't wrong, please try again" kind of message?

Back to our implementation, giving the user information like this is a great idea. Displaying more info about the error on the page or in a toast is a common pattern for good reason.

Start simple by handling all errors in the same way, then add complexity for (displaying) different error reasons one at a time.

@RafaelCaso
Copy link
Contributor

PR is in

@derrekcoleman derrekcoleman linked a pull request Aug 21, 2024 that will close this issue
2 tasks
derrekcoleman pushed a commit that referenced this issue Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend frontend work good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants