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: Add character page #8

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

feat: Add character page #8

wants to merge 6 commits into from

Conversation

VishruthR
Copy link
Collaborator

@VishruthR VishruthR commented Feb 7, 2023

Status:

🚧 In development

Description

Fixes hack4impact-uiuc/starter-projects#27

Todos

Screenshots

image

@VishruthR VishruthR marked this pull request as ready for review February 10, 2023 03:35
Copy link
Member

@gracewzhang gracewzhang left a comment

Choose a reason for hiding this comment

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

This looks really good! I just left a few really quick comments (the most important one being that the name of the series isn't Lord of the Ring), and make sure to update the PR description~

@@ -0,0 +1,3575 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Member

Choose a reason for hiding this comment

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

It'd be great if we could avoid including this!

Copy link
Collaborator

@reteps reteps Feb 15, 2023

Choose a reason for hiding this comment

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

I believe it is actually good practice to include the yarn.lock file: https://stackoverflow.com/a/40206145, for versioning issues . In the future, you can run:

yarn install --frozen-lockfile

to get the repo packages exactly the same as other stuff.

Copy link
Member

@gracewzhang gracewzhang Feb 15, 2023

Choose a reason for hiding this comment

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

oh hm we include it even when using npm?

src/pages/character/[id].tsx Show resolved Hide resolved
src/pages/character/[id].tsx Show resolved Hide resolved
<Center pb={4}>
<Image src="/LordOfRingLogo.png" alt="Logo" width="300px" />
</Center>
<Center>
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the Figma, we want the card to be centered on the page---maybe try putting the image & the card in the same Center component & giving the image absolute positioning

src/pages/character/[id].tsx Show resolved Hide resolved
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