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

Add Wilfred profile #34

Merged
merged 10 commits into from
Jan 30, 2025
Merged

Conversation

Wilfred007
Copy link
Contributor

@Wilfred007 Wilfred007 commented Jan 15, 2025

Description

I've made all the changes and updates

  • Added links to my socials
  • Spaced the socials icons
  • Replaced the screenshot
  • Changed the image name to my EVM address
  • Moved static data outside React component
  • I fixed nextjs image error
  • I moved the ADDRESS below the import statements
  • I've fixed the missing props error

Screenshot_16-1-2025_65136_super-duper-giggle-pjjjg79rjpwc7jjp-3000 app github dev

Additional Information

Your ENS/address: 0x2aA2f7090f0ADD72B2d50386CdDE97CE27898287

Copy link

vercel bot commented Jan 15, 2025

@Wilfred007 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 Jan 15, 2025

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

Name Status Preview Comments Updated (UTC)
batch12.buidlguidl.com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 30, 2025 0:39am

Copy link
Member

@derrekcoleman derrekcoleman 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 for your PR, Wildfred007! And welcome to the Batch12 repo :)

Thank you for including a screenshot of your PR 📷

  • Please also add a few summary sentences to your PR message describing what this PR adds (your page, an overview of sections, keep it high-level).
  • Also read the Contributing docs, then check both boxes in the PR message to acknowledge them.

I look forward to buidling with you!

@Wilfred007
Copy link
Contributor Author

Wilfred007 commented Jan 16, 2025

I've made all the changes and updates

  • Added links to my socials
  • Spaced the socials icons
  • Replaced the screenshot
  • Changed the image name to my EVM address

@derrekcoleman

Screenshot_16-1-2025_65136_super-duper-giggle-pjjjg79rjpwc7jjp-3000 app github dev

Copy link
Member

@derrekcoleman derrekcoleman left a comment

Choose a reason for hiding this comment

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

Thanks for implementing most of my suggestions, @Wilfred007! There are a couple last things I'd like you to practice in the spirit of building those good git habits 🏋️

  1. It's common to respond to the comment with a suggested change either with a comment of your own such as "Sounds good, will do", a clarifying question, or at least an emoji reaction to acknowledge you saw it.
  2. However you respond, please also click Resolve conversation for any comments that you addressed in your recent commits. That's another way to help the reviewer know you incorporated their feedback.
  3. Like I said before, please also add a few summary sentences to your original PR message describing what this PR adds (your page, an overview of sections, keep it high-level).
  4. Whenever you update the PR, it is recommended to update the first message's screenshot as well so anyone coming to the discussion has an up-to-date picture of the changes you made.
  5. Please address this previous comment. If you aren't sure what I mean, you can always ask :)

Copy link
Member

@derrekcoleman derrekcoleman left a comment

Choose a reason for hiding this comment

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

One last change, following up on this request from last week. lmk if you aren't sure what I mean

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.

Sorry for the wait.

Below you'll find two tiny comments and then we finally merge.

Next time you implement your changes, you can request a review from us directly on GitHub. Start by leaving a quick comment to let us know you've completed the changes. Then, in the top-right corner of the page, locate the reviewer box and click on the 🔄 icon to request a review.

Thank you!

Comment on lines 46 to 51
<Image
src="/0x2aA2f7090f0ADD72B2d50386CdDE97CE27898287.jpg"
alt="0x2aA2f7090f0ADD72B2d50386CdDE97CE27898287"
layout="fill"
className="rounded-full ring-4 ring-blue-500"
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a good practice it is recommended to check to console for any warnings/errors.
It shows me the following:
Screenshot 2025-01-28 at 09 43 17

Pls make sure to fix them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still outstanding

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot 2025-01-29 at 20 14 05

Copy link
Member

@derrekcoleman derrekcoleman left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the Image prop issue, @Wilfred007! Please take a look at the other comment, related to factoring all of your constants outside of the NextPage component - you still have an ADDRESS in the middle that would be more organized if it was just below your import statements.

@Wilfred007
Copy link
Contributor Author

Wilfred007 commented Jan 29, 2025

Thanks for addressing the Image prop issue, @Wilfred007! Please take a look at the other comment, related to factoring all of your constants outside of the NextPage component - you still have an ADDRESS in the middle that would be more organized if it was just below your import statements.

Thank you for your patience, I've made the changes

@phipsae
Copy link
Collaborator

phipsae commented Jan 29, 2025

@Wilfred007, please see my comments above. I’m still seeing the warning in the console. In general, when requesting a review, please ensure that all previous points have been addressed and check the console for any warnings/erros. Otherwise, it becomes repetitive to highlight the same issues multiple times.

@phipsae
Copy link
Collaborator

phipsae commented Jan 30, 2025

@Wilfred007 why do you request a review, when you haven't made any chances? I don't see any new commits...

This is the error it get still shown to me. If you scroll up, I already mentioned it in the unresolved conversation.
image

Pls fix it, thank!

@Wilfred007
Copy link
Contributor Author

@Wilfred007 why do you request a review, when you haven't made any chances? I don't see any new commits...

This is the error it get still shown to me. If you scroll up, I already mentioned it in the unresolved conversation. image

Pls fix it, thank!

Thank you for your patience, Ive fixed the missing props error, but I'm seeing a lit is in dev mode warning is that something I need to fix too because I've not seen this warning before. Can you help? Thank you

@derrekcoleman
Copy link
Member

Thank you for your patience, Ive fixed the missing props error, but I'm seeing a lit is in dev mode warning is that something I need to fix too because I've not seen this warning before. Can you help? Thank you

That error does not seem to happen in production, which is consistent with it being a "dev mode" (i.e. local testing) warning. Have you searched the error on the internet or asked an AI what it might mean and whether any changes might be needed? Those are steps I sometimes take when I'm facing a new, unfamiliar error.

@Wilfred007
Copy link
Contributor Author

Thank you for your patience, Ive fixed the missing props error, but I'm seeing a lit is in dev mode warning is that something I need to fix too because I've not seen this warning before. Can you help? Thank you

That error does not seem to happen in production, which is consistent with it being a "dev mode" (i.e. local testing) warning. Have you searched the error on the internet or asked an AI what it might mean and whether any changes might be needed? Those are steps I sometimes take when I'm facing a new, unfamiliar error.

I did but can't seem to find a direct solution, it's saying it might not be something serious in production. Is it possible for us to move on with this?

Copy link
Member

@derrekcoleman derrekcoleman left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to implement our suggestions, @Wilfred007! Your git-fu skills are stronger for it 🥷

@derrekcoleman derrekcoleman merged commit da6fa7c into BuidlGuidl:main Jan 30, 2025
3 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.

3 participants