Feature/footer#53
Conversation
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Really good work Robin!!! 😄 Some minor pieces of feedback:
- global.css has a list of the SSA branding colours (red, yellow, light red/yellow, etc) that are used throughout the website. we define any colours we use more than once in global.css so its more readable in code and for consistency. wherever you use a hex code as a colour, could you update it to use the corresponding name for it in global.css?
- the SVG logos are created as files but are never used, and are redefined with the raw svg in the code. could you update the code to use the files?
- another thing on the SVG logos, could you make a subfolder called /web/public/social-media-logos and then place the logos there, so it's easier to find
There are some other comments I left on specific parts of the code. But overall really nice work for a first PR! Flick me a message if you have any questions or confusion on my comments, and request a re-review once you’ve added the changes :)
| <nav className="flex flex-col gap-2"> | ||
| <a | ||
| href="/team" | ||
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" |
There was a problem hiding this comment.
is text-m meant to be text-sm?
There was a problem hiding this comment.
also hover:text-[#FFE6B6] can be changed to hover:text-ssa-yellow
There was a problem hiding this comment.
Adding to Oorja's comment, /54 is an arbitrary opacity (default Tailwind scale only goes up to common steps like /50, /60). Where does the 54% come from? Was it pulled from Figma, or rounded by hand? If it's from Figma, thats fine fine if not, snapping to /50 or /60 will keep things on the standard scale or using the Figma's value would be more ideal.
There was a problem hiding this comment.
is text-m meant to be text-sm?
I thought its medium but I just found out it's not classified as a proper tailwind css class so thats a mistake.
There was a problem hiding this comment.
Adding to Oorja's comment, /54 is an arbitrary opacity (default Tailwind scale only goes up to common steps like /50, /60). Where does the 54% come from? Was it pulled from Figma, or rounded by hand? If it's from Figma, thats fine fine if not, snapping to /50 or /60 will keep things on the standard scale or using the Figma's value would be more ideal.
I just eyeballed the opacity before but now i will change it according to figma values
| About Us | ||
| </p> | ||
| <nav className="flex flex-col gap-2"> | ||
| <a |
There was a problem hiding this comment.
in next.js, we use <Link> instead of <a> for internal pages (/team, /sponsors, etc). This is bc <Link> does not fully reload the page, and it prefetches the page in the background so it loads faster when clicked (source)
<a> reloads the browser, which is slower for internal pages. But you used it correctly for the social media links since theyre external pages!
could you update all internal links from <a> to <Link>?
| <path | ||
| fillRule="evenodd" | ||
| clipRule="evenodd" | ||
| d="M21.5 3.87168C27.2445 3.87168 27.9248 3.89687 30.184 3.99766C32.2836 4.09004 33.4174 4.44277 34.1732 4.73672C35.1727 5.12305 35.8949 5.59336 36.6424 6.34082C37.3982 7.09668 37.8602 7.81055 38.2465 8.80996C38.5404 9.56582 38.8932 10.708 38.9855 12.7992C39.0863 15.0668 39.1115 15.7471 39.1115 21.4832C39.1115 27.2277 39.0863 27.908 38.9855 30.1672C38.8932 32.2668 38.5404 33.4006 38.2465 34.1564C37.8602 35.1559 37.3898 35.8781 36.6424 36.6256C35.8865 37.3814 35.1727 37.8434 34.1732 38.2297C33.4174 38.5236 32.2752 38.8764 30.184 38.9688C27.9164 39.0695 27.2361 39.0947 21.5 39.0947C15.7555 39.0947 15.0752 39.0695 12.816 38.9688C10.7164 38.8764 9.58262 38.5236 8.82676 38.2297C7.82734 37.8434 7.10508 37.373 6.35762 36.6256C5.60176 35.8697 5.13984 35.1559 4.75352 34.1564C4.45957 33.4006 4.10684 32.2584 4.01445 30.1672C3.91367 27.8996 3.88848 27.2193 3.88848 21.4832C3.88848 15.7387 3.91367 15.0584 4.01445 12.7992C4.10684 10.6996 4.45957 9.56582 4.75352 8.80996C5.13984 7.81055 5.61016 7.08828 6.35762 6.34082C7.11348 5.58496 7.82734 5.12305 8.82676 4.73672C9.58262 4.44277 10.7248 4.09004 12.816 3.99766C15.0752 3.89687 15.7555 3.87168 21.5 3.87168ZM21.5 0C15.6631 0 14.9324 0.0251953 12.6396 0.125977C10.3553 0.226758 8.78477 0.596289 7.42422 1.12539C6.00488 1.67969 4.80391 2.41035 3.61133 3.61133C2.41035 4.80391 1.67969 6.00488 1.12539 7.41582C0.596289 8.78477 0.226758 10.3469 0.125977 12.6312C0.0251953 14.9324 0 15.6631 0 21.5C0 27.3369 0.0251953 28.0676 0.125977 30.3604C0.226758 32.6447 0.596289 34.2152 1.12539 35.5758C1.67969 36.9951 2.41035 38.1961 3.61133 39.3887C4.80391 40.5812 6.00488 41.3203 7.41582 41.8662C8.78477 42.3953 10.3469 42.7648 12.6313 42.8656C14.924 42.9664 15.6547 42.9916 21.4916 42.9916C27.3285 42.9916 28.0592 42.9664 30.352 42.8656C32.6363 42.7648 34.2068 42.3953 35.5674 41.8662C36.9783 41.3203 38.1793 40.5812 39.3719 39.3887C40.5645 38.1961 41.3035 36.9951 41.8494 35.5842C42.3785 34.2152 42.748 32.6531 42.8488 30.3688C42.9496 28.076 42.9748 27.3453 42.9748 21.5084C42.9748 15.6715 42.9496 14.9408 42.8488 12.648C42.748 10.3637 42.3785 8.79316 41.8494 7.43262C41.3203 6.00488 40.5896 4.80391 39.3887 3.61133C38.1961 2.41875 36.9951 1.67969 35.5842 1.13379C34.2152 0.604687 32.6531 0.235156 30.3688 0.134375C28.0676 0.0251953 27.3369 0 21.5 0Z" |
There was a problem hiding this comment.
youve already uploaded the svg files in web/public - you could just use the SVGs by referencing those files.
could you update for al three social media icons?
There was a problem hiding this comment.
youve already uploaded the svg files in web/public - you could just use the SVGs by referencing those files.
could you update for al three social media icons?
I initially inlined the SVG paths so I could use CSS fill properties for the hover effects. If I switch to referencing the files in /public via Image tags, we’ll lose that colour control on hover. Do you have a preferred alternative for keeping the hover interaction?
There was a problem hiding this comment.
Covered in meeting, but please use Material UI for icons. Lmk if you need any help with importing and using it
| className=" | ||
| w-11 h-11 rounded-xl flex items-center justify-center | ||
| text-white bg-transparent | ||
| hover:bg-[#FFE6B6] hover:text-[#f2627e] |
There was a problem hiding this comment.
in global.css we've already defined the SSA branding colours with names, so:
hover:bg-[#FFE6B6] can be updated to hover:bg-ssa-yellow
also where is #f2627e from 🤔 it looks similar to ssa-red so maybe could update to that instead?
| </a> | ||
| <a | ||
| href="/sponsors" | ||
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" |
There was a problem hiding this comment.
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" | |
| className="text-ssa-black/54 font-averia hover:text-ssa-yellow text-m font-bold transition-colors duration-500" |
| <nav className="flex flex-col gap-2"> | ||
| <a | ||
| href="/events" | ||
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" |
There was a problem hiding this comment.
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" | |
| className="text-ssa-black/54 font-averia hover:text-ssa-yellow text-m font-bold transition-colors duration-500" |
| </a> | ||
| <a | ||
| href="/defunct" | ||
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" |
There was a problem hiding this comment.
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" | |
| className="text-ssa-black/54 font-averia hover:text-ssa-yellow text-m font-bold transition-colors duration-500" |
| <nav className="flex flex-col gap-2"> | ||
| <a | ||
| href="/team" | ||
| className="text-ssa-black/54 font-averia hover:text-[#FFE6B6] text-m font-bold transition-colors duration-500" |
There was a problem hiding this comment.
Adding to Oorja's comment, /54 is an arbitrary opacity (default Tailwind scale only goes up to common steps like /50, /60). Where does the 54% come from? Was it pulled from Figma, or rounded by hand? If it's from Figma, thats fine fine if not, snapping to /50 or /60 will keep things on the standard scale or using the Figma's value would be more ideal.
| <footer className="w-full"> | ||
| <div className="rounded-t-3xl px-30 py-12 w-full bg-ssa-red"> | ||
| {/* Main content */} | ||
| <div className="flex justify-between"> |
There was a problem hiding this comment.
bug / mobile: This flex justify-between row holds all 4 footer sections side-by-side at every breakpoint. On iPhone 14 Pro Max (430px) the Explore + Contact Us columns get clipped off-screen entirely and the visible columns squeeze so tightly that headings break mid-word ("About Us" → "About" / "Us"). Needs to stack on mobile like this:
<div className="flex flex-col gap-10 md:flex-row md:justify-between">
You can refer to my inital attached picture in the comment below to see what it looks like on a mobile screen.
|
|
||
| return ( | ||
| <footer className="w-full"> | ||
| <div className="rounded-t-3xl px-30 py-12 w-full bg-ssa-red"> |
There was a problem hiding this comment.
mobile: px-30 is 120px of horizontal padding. On a 320–375px phone that leaves only ~80–135px of usable width for all footer content. Could you make this responsive e.g. px-6 md:px-12 lg:px-30 so mobile gets a sensible padding and the wide layout only shows in at desktop sizes?
| </div> | ||
|
|
||
| {/* Nav links */} | ||
| <div className="flex gap-30"> |
There was a problem hiding this comment.
bug / mobile: Same issue, scoped to the three nav columns. gap-30 is 120px between each column won't fit. Could you make this stack and shrink the gap responsively as shown here:
<div className="flex flex-col gap-6 sm:flex-row sm:gap-10 lg:gap-30">
| </p> | ||
| <nav className="flex flex-col gap-2"> | ||
| <a | ||
| href="/team" |
There was a problem hiding this comment.
| href="/team" | |
| href="/about" |
bug: The navbar uses /about for the about-us page (see PR #35), but this links to /team. You should use /about.
…xt-m to text-base, correct opacity value, redefined all css colouring values with ssa branding colour values
3 participants
What does this PR do?
Implemented the site footer including social media links (instagram, linkedIn, facebook and email) and navigation links to About, Events, and Sponsors pages.
Type of change
Checklist
feature/,fix/,chore/,hotfix/)