-
Notifications
You must be signed in to change notification settings - Fork 10
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: enhance homepage and add builders grid #20
Conversation
@gboigwe is attempting to deploy a commit to the BuidlGuidl Team on Vercel. A member of the Team first needs to authorize it. |
Although not entirely completed as I still need to work on some console errors I found, feel free to make suggestions and or contribute. |
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.
Nice job. Just a few ideas for improvements
packages/nextjs/app/builders/0x5cc8Be96B1C9A68F57a73b5bEa60cF5D890055A1/page.tsx
Outdated
Show resolved
Hide resolved
useEffect(() => { | ||
fetch("/api/builders") | ||
.then(res => res.json()) | ||
.then(addresses => { | ||
const buildersList = addresses.map((address: string, index: number) => ({ | ||
id: index, | ||
address: address, | ||
})); | ||
setBuilders(buildersList); | ||
setIsLoading(false); | ||
}) | ||
.catch(error => { | ||
console.error("Error fetching builders:", error); | ||
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.
This is a matter of preference, but react-query would fit well 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.
Also, beside getting the builders by their profile pages, we could get whoever made checkin, even the ones without a profile page. Check this discussion.
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.
Can we get the details from the builders that checked-in?
Okay, I will try it out and see, but not sure how to figure it, but I will definitely try all my best. Thanks so much
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 can grab the second parameter of this event:
event CheckedIn(bool first, address builder, address checkInContract);
There's an example on how to do this on the Challenge-4-Dex.
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.
@gboigwe, this one is still open. We want to display all checked-in builders, not just those with a personal profile.
@gboigwe thanks, you def tackle one of the more difficult tasks! Pls look into the comments @melanke made and work on them before you ask for a review. Let me know then I'll have a review! |
Okay, thank you. I actually did used the useScaffoldReadContract I will check to confirm why that was not seen. Thank you |
Oh and for the builders profile, remember that I used my builders page before for testing but @melanke suggested that I pull his as it was merged already, so I removed mine from the PR and used his |
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.
Thanks for the changes!
Here’s a general tip: focus solely on the issue you’ve chosen to work on and avoid making unrelated changes. Only modify the files necessary for that specific task.
I understand it can be challenging at first, but that’s exactly why we have this course to guide you! 😊
...s/nextjs/app/builders/0x2Bc096A12C5b37F035180aDeF70EB2B88351e5B8/_components/SocialLinks.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/app/builders/0x2Bc096A12C5b37F035180aDeF70EB2B88351e5B8/page.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/app/page.tsx
Outdated
const { | ||
data: checkedInCount, | ||
isLoading, | ||
error, | ||
} = useScaffoldReadContract({ | ||
contractName: "BatchRegistry", | ||
functionName: "checkedInCounter", | ||
}); | ||
|
||
const displayCount = isLoading ? ( | ||
<span className="loading loading-spinner loading-sm text-primary"></span> | ||
) : error ? ( | ||
"Error loading" | ||
) : typeof checkedInCount === "bigint" ? ( | ||
checkedInCount.toString() | ||
) : ( | ||
"0" | ||
); | ||
|
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 out of scope and relates to issue #6. Please remove it.
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.
Please remove this. It belongs to issue #6. If you want to include the "checkedInCounter", create a separate PR for it.
In a "real-world" scenario, when working on an issue, ensure you only add code that falls within its scope.
f06c50e
to
5b62d7e
Compare
Hi @gboigwe, thanks for the updates! There are just two more points left to address. Please always make sure to review the conversation for details.
Almost done :) |
Okay, thank you. Thanks for you patience with me too |
I will make sure to put that into effect. |
Of course, that's why I am here! So no worries at all. It's all about learning. |
@phipsae This code doesn't work because the const { data: events, isLoading: eventsLoading } = useScaffoldEventHistory({
contractName: "BatchRegistry",
eventName: "CheckedIn",
fromBlock: 127324219n,
}); Which is strange, because I got the Contract Creation block and subtracted 10, as we did on the challenges: But it works when we put zero on fromBlock. Do you know what's happening? |
No idea, right now. As soon as you push the code, I can have a look at it. |
I tested it locally, and it works in my environment. Do you have your NEXT_PUBLIC_ALCHEMY_API_KEY set? (Although, if block 0 works, it should be fine.) |
…dInCount, it's supposed to be implemented on a different task
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Looks great, thanks!
Only these little comments below and then we are finally ready to merge.
Thanks a lot for your effort! Finally lets merge 🎉 |
Enhanced Homepage and Builders Grid
Changes Made
Implementation Details
/builders
directory[Add screenshots of homepage and grid in light/dark modes]
Screenshot for the Checked Builders Count:
Screenshot for the number of registered builders page:
Additional Information
Still trying to fix some console issues
I have read the contributing docs (if this is your first contribution)
This is not a duplicate of any existing pull request
Related Issues
_Closes #4 _
Your ENS/address: 0x5cc8Be96B1C9A68F57a73b5bEa60cF5D890055A1