feature/event-page#42
Conversation
JotinderBhamra
changed the title
oorjagandhi
left a comment
oorjagandhi
left a comment
There was a problem hiding this comment.
Overall good start on the events page! It's clear you've put a lot of effort into it. Remember to run prettier to get the CI checks passing.
I’ve left some specific comments inline but here are some broader thoughts:
- In React, generally you want to avoid putting all the code in a single file for a page (component based architecture, read more: https://www.geeksforgeeks.org/reactjs/react-component-based-architecture/). Could you extract code into smaller components. i.e. having a UpcomingEventCard, RSVP button, SearchBar, EventCategories, etc. This will make the code more readable and maintainable.
- In regards to the images, can you please move some images into different folders for more accuracy, i.e. clock/location icons are not specific to only events so should be put in an ‘icon’ folder or similar
- A lot of the TailwindCSS seems to have quite oddly specific values with no justification, and the overall mobile responsiveness could be improved. In general, if you write code that may not be obvious to the code reviewer, you should write some justification either in the comments (if it’s short) or in the PR description.
- Here are some good videos to watch on responsive design with TailwindCSS: https://www.youtube.com/watch?v=PuovsjZN11Y and https://www.youtube.com/watch?v=l04dDYW-QaI
Looking forward to see you implement the changes!!!
| <button | ||
| type="button" | ||
| className="inline-flex min-h-10 w-full items-center justify-between rounded-full border-2 border-ssa-red bg-ssa-pink px-4 py-2 text-left font-averia text-xs font-bold uppercase tracking-[0.04em] text-ssa-red transition-colors duration-200 hover:bg-ssa-red hover:text-ssa-pink md:min-h-14 md:px-8 md:text-2xl" | ||
| > | ||
| <span>RSVP Now</span> | ||
| <span | ||
| aria-hidden="true" | ||
| className="text-base leading-none md:text-3xl" | ||
| > | ||
| → | ||
| </span> | ||
| </button> |
There was a problem hiding this comment.
bug: As you can see the RSVP button follows the same style as the "All" button to show that its been selected (light pink), but I assume the button should be showing the darker red colour to show that the user can interact with the button right?
| key={category} | ||
| type="button" | ||
| onClick={() => setSelectedPastEventCategory(category)} | ||
| className={`rounded-full border-2 px-3 py-1 font-averia text-[10px] font-bold transition-colors duration-200 md:min-h-[62px] md:px-8 md:text-[25px] ${ |
There was a problem hiding this comment.
nitpick: Right now the Inactive button hover (hover:bg-ssa-pink hover:text-ssa-red) renders identically to the active state (bg-ssa-pink text-ssa-red). So if a user mouses over an inactive category, it looks selected so they can't tell which one actually is. Might be worth bringing this up with Sydney to have a more distinct way of differentiating them (e.g. lighter pink, or just darken the red bg) so the active state stays visually unique or maybe not apply it to all by default at all.
you can see from my screenshot I'm hovering over the game option without having clicked it yet, could be a little confusing.
joengy
left a comment
joengy
left a comment
There was a problem hiding this comment.
Hey Jotinder, great improvement and it's really clear you took the feedback from the meeting, as you broke the page into smaller components (EventCategoryButton, EventCategoryFilters, EventSearchInput, HighlightCard).
I've left some inline comments below, the main things to address are swapping the CTA <a> to a Next.js <Link>, moving the clock/location icons to a shared folder (Oorja's carry-over from last review). There are a few minor token and sizing inconsistencies too, but nothing too complex. Once those are sorted this is looking close to mergeable, really happy with your stuff, good work!
| <a | ||
| href={ctaHref} | ||
| className="mt-8 inline-flex min-h-14 w-full max-w-[553px] items-center justify-between gap-4 rounded-full bg-ssa-card-cta px-6 font-averia text-xl font-bold text-ssa-muted-gold transition-colors hover:bg-ssa-card-cta-hover focus:outline-none focus:ring-2 focus:ring-ssa-muted-gold focus:ring-offset-2 focus:ring-offset-ssa-card lg:mt-auto lg:h-[68px] lg:text-[25px]" |
There was a problem hiding this comment.
The CTA uses <a href={ctaHref}> but it's called with ctaHref="/events", which is an internal route. In Next.js, internal links should use <Link> to avoid a full page reload and get prefetching.
| <a | |
| href={ctaHref} | |
| className="mt-8 inline-flex min-h-14 w-full max-w-[553px] items-center justify-between gap-4 rounded-full bg-ssa-card-cta px-6 font-averia text-xl font-bold text-ssa-muted-gold transition-colors hover:bg-ssa-card-cta-hover focus:outline-none focus:ring-2 focus:ring-ssa-muted-gold focus:ring-offset-2 focus:ring-offset-ssa-card lg:mt-auto lg:h-[68px] lg:text-[25px]" | |
| <Link | |
| href={ctaHref} | |
| className="mt-8 inline-flex min-h-14 w-full max-w-[553px] items-center justify-between gap-4 rounded-full bg-ssa-card-cta px-6 font-averia text-xl font-bold text-ssa-muted-gold transition-colors hover:bg-ssa-card-cta-hover focus:outline-none focus:ring-2 focus:ring-ssa-muted-gold focus:ring-offset-2 focus:ring-offset-ssa-card lg:mt-auto lg:h-[68px] lg:text-[25px]" | |
| > | |
| <span>{ctaLabel}</span> | |
| <span aria-hidden="true">→</span> | |
| </Link> |
Remember to add import Link from 'next/link' at the top.
| imageAlt, | ||
| }: HighlightCardProps) { | ||
| return ( | ||
| <article className="mx-auto grid w-full max-w-[1214px] overflow-hidden rounded-[32px] bg-ssa-card shadow-[0px_3px_4px_1px_#00000040,1px_-5px_4.3px_0px_#D5D5D54D] lg:min-h-[590px] lg:grid-cols-2"> |
There was a problem hiding this comment.
The box shadow value 4.3px and the hardcoded hex colours (#00000040, #D5D5D54D) look like they came straight from Figma. If they did just leave a comment next time saying this was from the Figma, or if the design wasn't finished/you were uncertain just let me know its a temperary thing.
| } | ||
|
|
||
| const categoryButtonBaseClassName = [ | ||
| 'rounded-full border-2 px-3 py-1 font-averia text-[10px] font-bold', |
There was a problem hiding this comment.
text-[10px] on mobile is very small for a button label, and md:text-[25px] / md:min-h-[62px] on line 12 are non-standard Tailwind values. I understand the mobile design is not finished yet so might be worth skipping for now and then coming back later to implement the mobile design for our app (probably something worth bringing up to with Oorja and or Sydney).
| 'px-4 py-1.5 font-averia text-base text-ssa-grey', | ||
| 'placeholder:text-ssa-pink-light', | ||
| 'focus:border-ssa-red focus:outline-none', | ||
| 'md:h-14 md:max-w-[1215px] md:px-5 md:text-xl', |
There was a problem hiding this comment.
md:max-w-[1215px] here doesn't match the max-w-[1214px] used on the wrapping div in page.tsx:28 and inside HighlightCard.tsx:34.
| text: '2nd April - 6PM', | ||
| }, | ||
| { | ||
| iconSrc: '/events/locationicon.svg', |
There was a problem hiding this comment.
I think Oorja flagged this in the last review, clockicon.svg and locationicon.svg (line 38) are generic icons, not event-specific, so they should live in a shared folder like web/public/icons/ rather than web/public/events/. Could you move them and update both paths? It just means that if another dev for whatever reason might want to use those icons, they can just import it from your code and it will function the same there as it did as you implemented rather than having to recode another one.
3 participants
What does this PR do?
Implemented the initial Events listing page including the Ice Kachang hero section, search functionality, and category filters. Added necessary SVG and PNG assets
Type of change
Checklist
feature/,fix/,chore/,hotfix/)