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

feat: members list #20

Merged
merged 21 commits into from
Nov 4, 2024
Merged

feat: members list #20

merged 21 commits into from
Nov 4, 2024

Conversation

pbkompasz
Copy link
Contributor

Description

Adds a simple list containing the checked in members' builder page.

Additional Information

Related Issues

Closes #4

Your ENS/address: 0x26BfbD8ED2B302ec2c2B6f063C4caF7abcB062e0

Copy link

vercel bot commented Oct 19, 2024

@pbkompasz is attempting to deploy a commit to the BuidlGuidl Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Oct 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
batch10-buidlguidl-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 21, 2024 7:42pm

@phipsae
Copy link
Collaborator

phipsae commented Oct 22, 2024

Thanks for the PR!

It would be great to include a picture of what you've implemented in the PR description (so I could see immediately what to expect). Also you might want to incorporate the issue number in your PR name.

Unfortunately, it doesn't seem to be working (see screenshot). Could you please fix it and push the changes again? I'll review it once that's done.

Screenshot 2024-10-22 at 17 44 54

@pbkompasz
Copy link
Contributor Author

@phipsae I added the builder profiles that are currently merged, the rest will have to be added afterwards.
Screenshot from 2024-10-23 19-52-13

Copy link
Collaborator

@phipsae phipsae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pbkompasz, thank you for taking on the issue!

There are still a few things that need attention, particularly implementing an automated approach for retrieving the builder profiles.

Please see my comments below.

If there are any questions, just ask me! Thank you!

packages/nextjs/app/builders/page.tsx Outdated Show resolved Hide resolved
packages/nextjs/app/builders/page.tsx Outdated Show resolved Hide resolved
packages/nextjs/app/page.tsx Outdated Show resolved Hide resolved
packages/nextjs/app/page.tsx Outdated Show resolved Hide resolved
packages/nextjs/app/builders/page.tsx Outdated Show resolved Hide resolved
packages/nextjs/app/builders/page.tsx Outdated Show resolved Hide resolved
@phipsae phipsae self-requested a review October 28, 2024 17:50
@pbkompasz pbkompasz marked this pull request as ready for review October 28, 2024 18:14
Copy link
Collaborator

@phipsae phipsae left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! It’s working well. However, I’d avoid using a constant for the names, as we’d have to manually update the codebase each time a new builder is added. Also, let’s ensure no errors are thrown in the console.

packages/nextjs/app/builders/constants.ts Outdated Show resolved Hide resolved
packages/nextjs/app/builders/page.tsx Outdated Show resolved Hide resolved
packages/nextjs/app/builders/page.tsx Show resolved Hide resolved
@phipsae phipsae self-requested a review October 29, 2024 17:29
@phipsae
Copy link
Collaborator

phipsae commented Nov 4, 2024

Thanks a lot for your work!

It works great, let's merge and we finally have a nice overview page for all the builders.

@phipsae phipsae merged commit 18c7775 into BuidlGuidl:main Nov 4, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7. List the members of the batch (read BatchRegistry contract)
4 participants