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

Kenny/7 home navbar #15

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Kenny/7 home navbar #15

wants to merge 18 commits into from

Conversation

KennyKeni
Copy link

Create the home page + nav bar

Issue Number(s): #7

What does this PR change and why?

Checklist

  • All components of the Figma designs should be implemented at the home page
  • The page should be located at the root route /
  • The navbar should be implemented and should contain interactive buttons that link between pages. Let these routes, for now, be /store, /bag, /log, /settings, /help.

Critical Changes

  • Create all components for home page
  • Add the feed button to navbar
  • Add Navbar includes interactive buttons that link between pages
  • Still need to fix font issues

Related PRs

Testing

View page on root route /

@KennyKeni KennyKeni marked this pull request as draft January 30, 2025 04:17
Copy link
Contributor

@michaelhyi michaelhyi left a comment

Choose a reason for hiding this comment

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

Overall, great work! I appreciate you writing comments, made it easy to answer questions + give feedback. I left comments, requesting some slight changes.

Just FYI, some of the UI seems to scale a bit differently based on resolution. All icons, buttons, text bubble appears to be bigger on my end, and it bleeds off the screen a bit. I left a SS of how it looks on my end.

Also, can we capitalize component names?

Screenshot 2025-01-30 at 11 13 32 AM

@michaelhyi michaelhyi marked this pull request as ready for review February 2, 2025 01:37
Copy link
Contributor

@michaelhyi michaelhyi left a comment

Choose a reason for hiding this comment

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

Great work Kenny! Everything looks great. Can we try to use SVG's for all assets?

Other than that, I left some small comments. Great work!

src/pages/_document.tsx Outdated Show resolved Hide resolved
}

const ProfileName: React.FC<ProfileNameProps> = ({ name: initialName }) => {
// Basic placeholder logic
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for turning the name into an input! On my end, even if I click the pencil icon, I still can't edit the input though.

Copy link
Author

Choose a reason for hiding this comment

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

I changed it a bit so it seems a more obvious that you can edit the text. Should the edit icon stay on the screen while you're editing it text though or should I replace it with a checkmark etc.

@cynthiazhang755
Copy link

Hey Kenny,
Thanks so much for your great work on this ticket!
For some reason, the text for the name, pencil icon, level number, and number of coins appears black on my end, is there a way of fixing that?
Screenshot 2025-02-01 at 9 42 01 PM

@mngo39
Copy link

mngo39 commented Feb 2, 2025

Home screen looks great, thank you so much!

The only comments I have for this sprint are completely my fault: could we make the pencil icon be white? I realized in the original design I had, i made it black so that is completely my fault! I have updated the design on the Figma to have a white icon with a drop shadow if you could use that instead?

@KennyKeni
Copy link
Author

Hey Kenny, Thanks so much for your great work on this ticket! For some reason, the text for the name, pencil icon, level number, and number of coins appears black on my end, is there a way of fixing that? Screenshot 2025-02-01 at 9 42 01 PM

I forgot to test with light mode, just fixed it.

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.

4 participants