Issue 14 create first two sections of landing page #31
Conversation
laurenpudz
left a comment
laurenpudz
left a comment
There was a problem hiding this comment.
A really great start, just some things to consider :)
There was a problem hiding this comment.
Please delete one of these and change the name to godot.png! We only need one :)
There was a problem hiding this comment.
Please use the different coloured version of this from the figma. You can export the image from the figma and swap it out for this one.
| const pageTitle = "Game Development UWA"; | ||
| const description = | ||
| "Little eye catching intro about what the club does here. Maybe " + | ||
| "something about the purpose of the club, maybe something about the " + | ||
| "type of events that the club runs."; |
There was a problem hiding this comment.
As these only get used once, they don't need to be defined as constants. Constants should only really be used for things that get reused. Instead, you can just write the title and description wherever you would use the constants.
| "something about the purpose of the club, maybe something about the " + | ||
| "type of events that the club runs."; | ||
| const btnList = [ | ||
| { name: "More about us", link: "" }, |
There was a problem hiding this comment.
Please link this to the committee/about page!
There was a problem hiding this comment.
Please move this into index.tsx. We can move the existing content in index.tsx into another page, maybe /healthcheck?
| <button | ||
| key={i} | ||
| className="rounded-lg border border-[#9CA4FD] px-6 py-3 font-jersey10 font-medium text-white transition hover:bg-[#9CA4FD]" | ||
| > | ||
| {item.name} > | ||
| </button> |
There was a problem hiding this comment.
Please use the existing button component here, so you wont need to include all the styling. The first button should have the filled variant (which should already be the default) and the second button should have the outline variant. Don't worry if the buttons don't look like the figma yet, they should be updated in an upcoming PR
| </div> | ||
| </div> | ||
|
|
||
| <div className="flex h-[280px] w-[400px] items-center justify-center rounded-xl bg-[#cfc2ff]"> |
There was a problem hiding this comment.
Please use tailwind utility classes here, instead of arbitrary px classes. This will be better for scaling and is generally better practice. Please also change over all other uses of arbitrary classes in this file that you can :)
| </div> | ||
| </div> | ||
|
|
||
| <div className="flex h-[280px] w-[400px] items-center justify-center rounded-xl bg-[#cfc2ff]"> |
There was a problem hiding this comment.
Please use the custom project colours wherever you can. There should be a matching colour for this somewhere. If one doesn't exist (and you are reusing this colour in other places), you can create one.
| } | ||
|
|
||
| return ( | ||
| <div className="rounded-md border border-purple-400 px-4 py-2 font-jersey10 font-semibold text-purple-300"> |
There was a problem hiding this comment.
Text in the title of the event type cards should be white, as in the figma. The background of the card title section should also be a dark purple. Please also change this over for the card with the special border. It may also look better if you increase the font size of the title a bit.
Please try increasing the border width a little so it more closely matches the figma and the width of the border of the specially shaped box.
| const mainPic = { | ||
| url: "/placeholder.png", | ||
| alt: "placeholder", | ||
| }; | ||
|
|
There was a problem hiding this comment.
This probably also doesn't need to be defined as a const unless we resuse this information somewhere else
| <div className="flex w-full max-w-[1440px] flex-col items-center justify-between gap-12 md:flex-row"> | ||
| <div className="flex max-w-lg flex-col gap-6 text-white"> | ||
| <h1 className="font-jersey10 text-4xl font-bold">{pageTitle}</h1> | ||
| <p className="font-sans text-base leading-relaxed text-white/80"> |
There was a problem hiding this comment.
I think that font-sans should already be the default, so you don't need to apply the font-sans class here
| </div> | ||
| </div> | ||
|
|
||
| <div className="flex h-[280px] w-[400px] items-center justify-center rounded-xl bg-[#cfc2ff]"> |
There was a problem hiding this comment.
Also - the main image should take up the entire space of the div here, inside of being centered inside of the div. You should be able to remove the div that is wrapping the image and just use an Image component.
You can export the placeholder image from the figma and add it to the repo to use here.
| width={card.image.width} | ||
| height={card.image.height} | ||
| alt={card.image.alt} | ||
| className="opacity-60" |
There was a problem hiding this comment.
I don't think changing the opacity is necessary here. We should be able to get the desired colour by using the image imported from the figma
| className="opacity-60" |
| type: string; | ||
| image: cardImage | null; | ||
| row: number; | ||
| gridColumn: string; |
There was a problem hiding this comment.
I don't think the gridColumn value ever actually gets used when rendering the cards. Please either remove it or use it when rendering the cards.
Change Summary
Implemented the first two sections of the Landing Page.
Added gage title, description and event cards.
Basic layout and styling in Tailwind completed.
Change Form
Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.
Other Information
Related issue