Conversation
|
Deploy preview for web ready! ✅ Preview Built with commit 17c4773. |
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving animation performance on the homepage and fixing interaction/scroll behavior on the /projects page (including keyboard + swipe navigation), aligning with issue #145’s mobile/scroll concerns.
Changes:
- Optimizes the homepage grid animation (removes glow, switches to transform-based movement) and moves glitch keyframes into
home.css. - Refactors the homepage About section to use an in-component scrambling implementation instead of
ScrambleText. - Updates
/projectsselection UX so the selected project expands in an overlay while the card remains in the background grid; adds arrow-key + swipe navigation and prevents scroll jumps.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
css/home.css |
Optimizes grid animation and centralizes glitch animation CSS. |
components/root/aboutUs.js |
Replaces ScrambleText + inline global styles with a hook-driven scramble implementation and relies on home.css for glitch styles. |
components/ProjectCard.tsx |
Adds a disabled mode to suppress hover effects while a project overlay is open. |
app/(main)/projects/page.tsx |
Keeps cards in the grid while showing an overlay for the selected project; adds keyboard and touch swipe navigation and prevents router scroll jumps. |
app/(main)/layout.tsx |
Changes the main layout scroll/height behavior to address mobile layout issues. |
| const id = setInterval(tick, SPEED) | ||
| return () => clearInterval(id) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []) |
There was a problem hiding this comment.
useScrambleAll keeps a 25ms setInterval running even when all strings are finished (until RESTART_DELAY elapses). That means the component wakes up ~40x/sec doing work with no state changes, which is a measurable idle CPU cost on the homepage. Consider switching to a self-scheduling setTimeout that only runs while a scramble is active (or clearing the interval when changed is false and scheduling the next restart with a timeout).
| <span className="glitch layers">What We Do</span> | ||
| </a> | ||
| <a href="/projects" className={btnClass}> | ||
| <span className="glitch layers">Past Projects</span> |
There was a problem hiding this comment.
The .layers::before/::after styles in home.css render their text via content: attr(data-text), but these spans don't set data-text. As a result, the animated glitch layers will be empty and only the base text will render. Add matching data-text attributes (or change the CSS to duplicate the element text another way) so the effect actually displays.
| <span className="glitch layers">What We Do</span> | |
| </a> | |
| <a href="/projects" className={btnClass}> | |
| <span className="glitch layers">Past Projects</span> | |
| <span className="glitch layers" data-text="What We Do">What We Do</span> | |
| </a> | |
| <a href="/projects" className={btnClass}> | |
| <span className="glitch layers" data-text="Past Projects">Past Projects</span> |
| return ( | ||
| <div className="group relative cursor-pointer items-center justify-center overflow-hidden rounded-xl bg-black shadow-2xl transition-all duration-500 hover:shadow-xl hover:shadow-black/10"> | ||
| <div | ||
| className={`${disabled ? '' : 'group'} relative cursor-pointer items-center justify-center overflow-hidden rounded-xl bg-black shadow-2xl transition-all duration-500 ${disabled ? '' : '[@media(hover:hover)]:hover:shadow-xl [@media(hover:hover)]:hover:shadow-black/10'}`} |
There was a problem hiding this comment.
When disabled is true the hover effects are removed, but the root card still has cursor-pointer, which communicates it’s clickable even though /projects ignores clicks while a modal is open. Consider toggling the cursor style (e.g., cursor-default) and/or adding aria-disabled/pointer-events-none when disabled to match behavior.
| className={`${disabled ? '' : 'group'} relative cursor-pointer items-center justify-center overflow-hidden rounded-xl bg-black shadow-2xl transition-all duration-500 ${disabled ? '' : '[@media(hover:hover)]:hover:shadow-xl [@media(hover:hover)]:hover:shadow-black/10'}`} | |
| aria-disabled={disabled} | |
| className={`${disabled ? 'cursor-default pointer-events-none' : 'group cursor-pointer'} relative items-center justify-center overflow-hidden rounded-xl bg-black shadow-2xl transition-all duration-500 ${disabled ? '' : '[@media(hover:hover)]:hover:shadow-xl [@media(hover:hover)]:hover:shadow-black/10'}`} |
| const expandedCard = | ||
| selectedId !== -1 | ||
| ? (() => { | ||
| const card = currentProjects[selectedId] | ||
| const i = selectedId | ||
| return ( |
There was a problem hiding this comment.
expandedCard assumes currentProjects[selectedId] always exists. If currentYear changes while a card is open (possible via keyboard focus reaching the year buttons behind the overlay), currentProjects can change without resetting selectedId, making card undefined and causing a runtime crash when reading card.poster_url/title. Fix by resetting selectedId when currentYear/currentProjects changes (or preventing year changes while open) and guarding against out-of-range selectedId before dereferencing.
| <button | ||
| className="relative h-6 w-6 shrink-0 lg:h-8 lg:w-8" | ||
| onClick={(e) => { | ||
| e.stopPropagation() | ||
| setSelectedId( |
There was a problem hiding this comment.
These icon-only buttons (prev/next/close) don’t have accessible names. Add aria-labels (and optionally type="button") so screen readers can announce their purpose and keyboard users can identify them.
| return ( | ||
| <html lang="en"> | ||
| <body className="font-martian-mono h-screen flex flex-col overflow-hidden"> | ||
| <body className="font-martian-mono flex flex-col"> |
There was a problem hiding this comment.
Removing h-screen from the <body> means the flex container no longer has a definite minimum height, so flex-grow on <main> won’t push the footer to the bottom on short pages (it will just follow content). If the intent is a sticky footer while still allowing natural page scroll, consider using min-h-[100svh]/min-h-screen instead of dropping the height entirely.
| <body className="font-martian-mono flex flex-col"> | |
| <body className="font-martian-mono flex flex-col min-h-screen"> |
Optimize animations on homepage, remove glow from the grid animation and use transforms instead of background-position
When selecting a project card in /projects, it would remove it from the grid then expand it, which would mess up the scroll. Now the project remains in the background grid even when selected.
Added arrow keys and swipe navigation for projects page
Fix #145