Feature/event gallery by Robin-deng617 · Pull Request #87 · UoaWDCC/ssa

Robin-deng617 · 2026-05-21T09:50:35Z

Added a event gallery page to view and download event photos.

Type of change

Feature
Bug fix
Chore / config
Hotfix

Checklist

My branch follows the naming convention (feature/, fix/, chore/, hotfix/)
My commit messages follow the conventional commits format
CI checks pass

…te component

…design

joengy

Great work initally on the components and gallery pages so far Robin, I've left some comments and suggestions so when you get the chance could you implement those. Otherwise happy with how the code is so far.

joengy · 2026-05-23T06:36:21Z

+export default async function GalleryPage({
+  params,
+}: {
+  params: { eventID: string }
+}) {
+  const { eventID } = await params


In Next.js 15+/16 (our repo is on next@16.1.6), params is a Promise that resolves to the params object, not the object itself. A Promise<T> is just "a value of type T that isn't ready yet" so you unwrap it with await. Your code already awaits it which is why it works, but the type annotation still claims it's a plain object. Fix the type like this:

export default async function GalleryPage({ params, }: { params: Promise<{ eventID: string }> }) { const { eventID } = await params ... }

joengy · 2026-05-23T06:39:49Z

+          {/* Close button */}
+          <button
+            onClick={onClose}
+            className="absolute top-4 right-4 w-8 h-8 rounded-full bg-ssa-light-grey/20 flex items-center justify-center text-ssa-white hover:bg-ssa-skin-yellow hover:text-ssa-light-brown transition-colors"


Modal container is missing dialog semantics. Add role="dialog", aria-modal="true", and aria-labelledby pointing at the <h2>. The close/prev/next buttons also need aria-labels.

joengy · 2026-05-23T06:40:00Z

+              key={img.id}
+              onClick={() => openModal(index)}
+              className="relative aspect-square rounded-lg md:rounded-xl overflow-hidden group cursor-pointer"
+            >


Clickable <div> thumbnail isn't keyboard-accessible (no tabIndex, no Enter/Space handler). Make it a <button type="button"> .

joengy · 2026-05-23T06:41:50Z

+import ZoomInIcon from '@mui/icons-material/ZoomIn'
+
+const INITIAL_COUNT = 12
+const LOAD_MORE_COUNT = 30


Change LOAD_MORE_COUNT = 30 to 12 so each "page" matches the initial render and the layout doesn't jump on the first click.

joengy · 2026-05-23T06:45:56Z

+
+  return (
+    <>
+      <div className="flex flex-col items-center gap-10 max-w-8xl mx-auto w-full px-4 md:px-30 py-10 md:py-30">


max-w-8xl, md:px-30, md:py-30 aren't default Tailwind tokens (max is max-w-7xl and spacing scale jumps from 28 to 32). Unless they're added to the Tailwind config they shouldn't be properly styling. Use max-w-7xl / md:px-28 / md:px-32 or extend the theme. See if changing those values does anything to the gallery grid.

it made it a little smaller but i think it should be fine

joengy · 2026-05-23T06:50:16Z

+              className="absolute left-3 top-1/2 -translate-y-1/2 w-10 h-10 rounded-full flex items-center justify-center text-gray-700 bg-[#FFF7E9]/60 hover:bg-[#FFF7E9]/90 transition-colors duration-200 border border-white"
+            >
+              <ArrowBackIcon fontSize="small" />
+            </button>
+
+            {/* Right arrow */}
+            <button
+              onClick={handleNext}
+              className="absolute right-3 top-1/2 -translate-y-1/2 w-10 h-10 rounded-full flex items-center justify-center text-gray-700 bg-[#FFF7E9]/60 hover:bg-ssa-yellow-light/90 transition-colors duration-200 border border-white"


Another set of hardcoded hex colors (#FFF7E9, #D4CECE, #C9A84C). These exist have very similar colours in the global CSS file. If they are completely new colours you can add an additonal colour in global.css, otherwise change them.

joengy · 2026-05-23T06:51:43Z

+  --color-ssa-light-grey: rgba(130, 130, 130);
+  --color-ssa-light-brown: rgb(150, 128, 85);


The way you set these two colours is completely different to how we're currently using hex values. Could you change these two to hex values rather than rgb and rgba to keep the rest of the file consistent.

joengy · 2026-05-23T06:53:58Z

+  eventTitle: string
+  eventDate: string
+}
+


current can be undefined if images is empty or currentIndex is out of range so the useEffect at line 51 will then throw on current.url. Add a check like if (!current) return null before the effects.

joengy · 2026-05-23T06:56:28Z

+  }, [currentIndex, total, onNavigate])
+
+  useEffect(() => {
+    const img = new window.Image()


new window.Image() causes a second network fetch on top of next/image's fetch, and the modal visibly resizes when the measurement lands. Consider using next/image's onLoad (which exposes naturalWidth/naturalHeight).

joengy · 2026-05-23T06:57:16Z

+          <div className="flex items-center justify-end mt-4">
+            <a
+              href={current.url}
+              download
+              className="flex items-center gap-2 px-6 py-2.5 rounded-full bg-ssa-red-light text-white font-averia font-bold hover:bg-ssa-red transition-opacity border-[3px] border-ssa-red"


<a href={url} download> only works for same-origin URLs. Once images come from the CMS (likely a different origin), browsers will ignore download and just navigate. Worth noting for the data-integration follow-up.

oorjagandhi

Nice work on this!!

For the new components added, could you please move the gallery specific components (GalleryGrid, PhotoAlbum) into the component folder within the events folder? This is because we want to group specific components into their respective folders so it's easier to track the files, rather than having one massive components folder at the root (the root folder should be reserved for components that will be used throughout the app, for example the ViewMoreButton may be reused somewhere else so you can leave it in the current folder).

Another note: on anything that is 'clickable' like the buttons, hovering over the image before viewing it, etc, the cursor should change to a pointer. This can be done by chucking "cursor-pointer" into the in-line Tailwind for the component you want to be made clickable. I left a code comment as an example, could you please update all clickable components to reflect this?

As discussed in the Discrd, there are some minor design refinements so lmk once you add those and I'll re-review

Awesome work! :)

oorjagandhi · 2026-05-25T02:53:58Z

+                fill
+                className="object-cover"
+              />
+              <div className="absolute inset-0 bg-black/40 opacity-0 group-hover:opacity-100 transition-opacity duration-300 flex items-center justify-center">


Suggested change

<div className="absolute inset-0 bg-black/40 opacity-0 group-hover:opacity-100 transition-opacity duration-300 flex items-center justify-center">

<div className="absolute inset-0 bg-black/40 opacity-0 group-hover:opacity-100 transition-opacity duration-300 flex items-center justify-center cursor-pointer">

I've implemented all the changes you suggested but regarding the minor design tweaks I'll wait a bit longer until the smaller details are tuned out

Robin-deng617 added 6 commits May 11, 2026 23:49

added event gallery image grid with dynamic event routing

5eca59a

added a viewMore component and also extracted galleryGrid into sepera…

5cc3a7a

…te component

added photo album lightbox with hover overlay and view more button re…

19ca097

…design

added mobile responsive styles and fix hover state for buttons

f2fe780

resolve merge conflicts with main

56005aa

update pnpm-lock.yaml with MUI dependencies

83c8e8e

joengy requested changes May 23, 2026

View reviewed changes

This was referenced May 23, 2026

Feature/home carousel #83

Open

chore: remove tracked web/package-lock.json #88

Merged

implemented changes requested by tl

63fc12c

oorjagandhi requested changes May 25, 2026

View reviewed changes

implemented cursor-pointer to all clickable components

ddadbaf

		--color-ssa-light-grey: rgba(130, 130, 130);
		--color-ssa-light-brown: rgb(150, 128, 85);

	<div className="absolute inset-0 bg-black/40 opacity-0 group-hover:opacity-100 transition-opacity duration-300 flex items-center justify-center">
	<div className="absolute inset-0 bg-black/40 opacity-0 group-hover:opacity-100 transition-opacity duration-300 flex items-center justify-center cursor-pointer">

Conversation

Robin-deng617 commented May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Checklist

Uh oh!

joengy left a comment

Choose a reason for hiding this comment

Uh oh!

joengy May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oorjagandhi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Robin-deng617 commented May 21, 2026 •

edited

Loading

joengy May 23, 2026 •

edited

Loading