Feature/home carousel by GladwynChua · Pull Request #83 · UoaWDCC/ssa

GladwynChua · 2026-05-19T09:43:52Z

What does this PR do?

Created a carousel for mock images tentatively will replace once images are confirmed. Event titles at the top are just mock names. Responsive with mobile version by swiping. HREF for linking between albums needs to be confirmed as well.

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

Linked Issues

Closes #71

GladwynChua · 2026-05-19T09:50:41Z

Currently HREF not confirmed for the albums. Unsure which images should be displayed on the carousel, so mock images are used.

oorjagandhi

Nice work! I noticed there are some differences between this and the Figma file, more specifically

the 'View Album' button styling
the styling of the text on the top right
the left / right buttons
the nav buttons on the carousel should be centred.

You should also create a 'gallery' component and just use that in the app page for improved modularity. Overall good work tho! :)

joengy

Nice work! Carousel behaves well and CI's green. Main ask is to pull it into its own Gallery component left a few small notes inline. Happy to re-review once that's done but overall good job.

joengy · 2026-05-20T04:59:10Z

+  },
+]
+
 export default function Home() {


Could you extract this into its own component, e.g. web/src/components/Gallery.tsx(or Carousel.tsx)? Right now page.tsx is mostly carousel logic, which breaks the per-component pattern used by Hero, HighlightCard, InstagramFeed etc., and forces the entire home page to be 'use client'.

joengy · 2026-05-20T05:12:03Z

+    },
+    [startTimer],
+  )
+
+  const goPrev = useCallback(() => {


goPrev and goNext both read current directly, so they have to list it in their dependency arrays. That means every time the slide advances (every 6s, plus on every user interaction), React throws away the old callbacks and creates fresh ones, which also recreates handleTouchEnd (since it depends on them). It's not a visible bug, just unnecessary churn.

You can avoid it by reading the previous value from setCurrent's updater function instead of from the closure, then the callbacks don't need current at all and stay stable across renders:

const goPrev = useCallback(() => { setCurrent((c) => (c - 1 + carouselImages.length) % carouselImages.length) startTimer() }, [startTimer])

const goNext = useCallback(() => { setCurrent((c) => (c + 1) % carouselImages.length) startTimer() }, [startTimer])

joengy · 2026-05-20T05:14:50Z

+      <section>
+        <div className="relative">
+          <div
+            className="relative w-full aspect-[16/7] sm:aspect-[16/7] overflow-hidden"


sm:aspect-[16/7] is the same value as the base so can drop the sm: variant.

joengy · 2026-05-20T05:15:30Z

        />
      </section>
+
+      {/* Image Carousel */}


The carousel pauses on hover but not on keyboard focus so a keyboard user tabbing to the prev/next buttons will have slides advancing under them. Could you add onFocus={stopTimer} / onBlur={startTimer} (or equivalent focus-within logic) so it pauses for keyboard users too?

joengy

The home carousel is looking good, however theres some small changes that need to be done so I left some comments for those. Othewise good job overall Gladwyn.

joengy · 2026-05-23T07:10:11Z

@@ -1,8 +1,10 @@
+'use client'


Drop this 'use client'. HomeCarousel already declares its own 'use client' boundary, so importing it from a server component works fine. Marking the homepage as a client component opts the whole route (Hero, HighlightCard, InstagramFeed, Footer) out of server rendering for no benefit.

joengy · 2026-05-23T07:16:32Z

+                  <a
+                    href={image.albumHref}
+                    className="inline-flex items-center gap-2 border-2 rounded-xl border-ssa-white bg-transparent text-ssa-white font-averia font-semibold text-sm md:text-base px-5 py-2 transition-colors cursor-pointer hover:bg-white/20"


Albums are internal routes per PR #87 (/events/[eventID]), so use next/link instead of a raw <a> to get client-side navigation and prefetching:

import Link from 'next/link' // ... <Link href={image.albumHref} // e.g. `/events/${eventId}` className="inline-flex items-center gap-2 border-2 rounded-xl border-ssa-white bg-transparent text-ssa-white font-averia font-semibold text-sm md:text-base px-5 py-2 transition-colors cursor-pointer hover:bg-white/20" > View Album → </Link>

joengy · 2026-05-23T07:19:06Z

This looks like the result of a stray npm install so could you please revert all changes to web/package-lock.json (e.g. git checkout origin/main -- web/package-lock.json) and run pnpm install instead. The added rows are just @next/swc-* optional platform packages that pnpm/Next resolve automatically, so pnpm-lock.yaml likely doesn't need to move either.

No need to delete the file itself as I'm putting up a separate chore PR to remove web/package-lock.json from tracking. Once that lands you can rebase and the lockfile will be gone entirely.

joengy · 2026-05-23T07:21:30Z

+          <div className="absolute bottom-4 left-0 right-0 flex justify-center gap-2">
+            {carouselImages.map((_, index) => (
+              <button
+                key={index}
+                onClick={() => goTo(index)}
+                aria-label={`Go to slide ${index + 1}`}
+                className="flex items-center justify-center w-6 h-6 cursor-pointer"
+              >
+                <span
+                  className={`block w-2.5 h-2.5 rounded-full transition-all duration-200 ${index === current ? 'bg-ssa-red scale-125' : 'bg-white/70'}`}
+                />
+              </button>
+            ))}
+          </div>
+        </div>


The dots row is at bottom-4 and so is the "View Album" button so they'll visually overlap on narrow viewports, and the dots may also intercept clicks intended for the button. I attached an image of what that would look like on the mobile view.

joengy · 2026-05-23T07:22:53Z

+            {carouselImages.map((image, index) => (
+              <div key={index} className="relative h-full w-full shrink-0">


key={index} is fine while the list is static, but once slides come from CMS data prefer a stable id (e.g. eventId or slug) so React doesn't reconcile the wrong slide if order changes.

joengy · 2026-05-23T07:27:02Z

+    intervalRef.current = setInterval(() => {
+      setCurrent((prev) => (prev + 1) % carouselImages.length)
+    }, 6000)


The auto-advance should respect prefers-reduced-motion and give users a way to pause. Minimum change is gating the interval on the OS setting:

useEffect(() => { if (window.matchMedia('(prefers-reduced-motion: reduce)').matches) return startTimer() return () => { if (intervalRef.current) clearInterval(intervalRef.current) } }, [startTimer])

oorjagandhi

Thanks for updating the width of the gallery! Just some changes:

the font for title top left should be thinner to match Figma
also the top left title should have more spacing from top left corner
arrows should be less opaque
view album button should have more spacing from the bottom right corner
view album button should be bigger
add a drop shadow on component

Overall good job! Most of the changes are to better match the Figma. In the future it'd be great if you did a side-by-side check with Figma before putting up a PR, so you can catch differences early and minimize the amount of changes you need to make during review :)

GladwynChua added 2 commits May 19, 2026 21:38

feat(home-carousel): implemented sliding carousel with events names

b43d17f

feat(home-carousel): implemented sliding carousel with events names

7acffb0

oorjagandhi requested changes May 20, 2026

View reviewed changes

joengy requested changes May 20, 2026

View reviewed changes

GladwynChua added 3 commits May 22, 2026 15:38

refactor: extract carousel into HomeCarousel component

63379bd

import home carousel to page.tsx

cca3fb7

chore: pass CI checks

ce7e8b4

joengy requested changes May 23, 2026

View reviewed changes

joengy mentioned this pull request May 23, 2026

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

Merged

2 tasks

oorjagandhi requested changes May 24, 2026

View reviewed changes

		{carouselImages.map((image, index) => (
		<div key={index} className="relative h-full w-full shrink-0">

Conversation

GladwynChua commented May 19, 2026

What does this PR do?

Type of change

Checklist

Linked Issues

Uh oh!

GladwynChua commented May 19, 2026

Uh oh!

oorjagandhi left a comment

Choose a reason for hiding this comment

Uh oh!

joengy 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joengy 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!

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 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oorjagandhi left a comment •

edited

Loading