search

henkaku-center / henkaku-nengajo-frontend

https://nengajo.henkaku.org/
8 stars 14 forks source link

Add header and footer. #27

Closed imaichiyyy closed 1 year ago

imaichiyyy commented 1 year ago

Summary.

vercel[bot] commented 1 year ago

@imaichiyyy is attempting to deploy a commit to the Henkaku Community Dev team Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git โ†—๏ธŽ

Name Status Preview Updated
henkaku-nengajo-frontend โœ… Ready (Inspect) Visit Preview Nov 27, 2022 at 1:22PM (UTC)
alecrem commented 1 year ago

Deploy approved. I will take a better look later.

Beware that I haven't set NEXT_PUBLIC_CONTRACT_HENKAKUV2_ADDRESS on Vercel yet, so $HENKAKU balance isn't correctly retrieved.

Edit: I just added the environment variable and redeployed. $HENKAKU balance is correctly retrieved, so all functionality seems to still work. The new URL is: https://henkaku-nengajo-frontend-kd2skp9gu-henkaku-community-dev-team.vercel.app/

alecrem commented 1 year ago

Please add "Close #23" to your summary, to link this PR to the issue.

imaichiyyy commented 1 year ago

Thank you for confirming. Another thing, I've merged #24 pull request and replaced the paths. Please check this as well.

imaichiyyy commented 1 year ago

I also removed "[WIP]".

alecrem commented 1 year ago

This PR adds a lot of content and features, so I will have a lot of comments and maybe other people should share their opinion too.

This is from the issue;

Collection

(Which of the following?)

Does it show "Nengajo" that user have minted.
Or do you want to see the "Nengajo" user have registered.
Or both

This is a great question, and I think we will have both pages. But the navigation menu looks good to me as is.

I think the "Nengajo you have registered" page can be linked from the Register or Collection page. No real need for a nav menu link.

Either way, after this PR is merged it should be easy to add and remove links from the navigation menu.

alecrem commented 1 year ago

About the filename for the new components, two things are different from the other components we have. I have some questions in case someone know about naming conventions or has a confident opinion.

I know these naming schemes exist on Omise, but I don't know if it's just us being inconsistent.

imaichiyyy commented 1 year ago

componentname/index.tsx: I don't know why it has its own directory if it's only one file Why not just use Componentname.tsx?

Next.js supports "CSS Modules" by default, so the CSS files used for components are stored in the same directory for better visibility and clarity.

ex: 

/components/Header/index.tsx
/components/Header/Header.module.css

I think "Omise" is probably for the same reason.

So, I think it is better to use its own directory.

layouts/layout.tx: Why not start the filename with a big L? Layout.tsx.

Sorry, I referred to the "Omise". So I am not very clear on this point. I think this is about naming conventions, so the only thing left to do is make a choice. What shall we do?๐Ÿง

alecrem commented 1 year ago

So, I think it is better to use its own directory.

Thank you for educating me! ๐Ÿ™‡ I agree.

layouts/layout.tx: Why not start the filename with a big L? Layout.tsx. I think this is about naming conventions, so the only thing left to do is make a choice.

Starting the filename with a capital letter would be consistent with previous components on the Nengajo frontend, and with most of non-index.tsx Omise components.

This reason doesn't have a lot of weight, so I'd like to hear other arguments if anybody has them.

alecrem commented 1 year ago

Notes from today's meeting.

You have already done a lot more than the scope of the issue. Please let me know when you want to merge by requesting a new review.

The following changes are not required to close the issue, and you don't need to include them on the PR. But if you want to do them during the weekend, that is nice too.

Minting vocabulary

Create -> Get ไฝœใ‚‹ -> ใ‚‚ใ‚‰ใ†

("Get" doesn't actually need to be used on this PR)

Site title

Component path and filename capitalization:

It looks like both the directory within components and the component's filename should start with a capital letter:

Directory/Component.tsx

Links shared during the meeting:

imaichiyyy commented 1 year ago

Thanks for your comment. I have corrected the following items you wrote in your Notes.

Please check code.

(But, If this work is more than the scope of the PR, please reject it)