Refactor app styling and standardize UI components#479
Refactor app styling and standardize UI components#479Kabe-Innovates wants to merge 13 commits intoFOSSUChennai:mainfrom
Conversation
- Introduced new "Add Event" link button in CallToAction component, routing to /add-event page - Updated button styles to use consistent 'btn' classes with improved hover and focus effects - Enhanced rocket image with drop-shadow and adjusted container padding for responsiveness - Polished gradient background, text contrast, and overall layout for improved visual impact
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive design system refactor to standardize UI components and improve visual consistency across the application. The changes centralize styling through utility classes defined in globals.css and apply them consistently throughout the codebase.
Key Changes
- Design System Foundation: Added centralized utility classes (
.btn,.badge,.surface,.section-title, etc.) inglobals.csswith consistent colors, spacing, and responsive patterns - Component Standardization: Replaced ad-hoc inline styles with reusable design system classes across all major components (Header, Footer, Events, Archive, Communities)
- Layout Improvements: Implemented a consistent
.container-pagewrapper for responsive padding and max-width constraints across all pages
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/app/globals.css |
Added comprehensive design system with utility classes for buttons, badges, surfaces, typography, and layout containers |
src/components/shared/header.tsx |
Refactored navigation to use design system classes, improved spacing with gap utilities, added priority loading to logo images |
src/components/shared/footer.tsx |
Updated to use container-page wrapper, improved link styling with consistent decoration patterns, added rel="noreferrer" for security |
src/components/pages/home/hero.tsx |
Changed from div to semantic section element, added section-kicker for subtitle, updated color utilities to use Tailwind's semantic color scale |
src/components/pages/home/events.tsx |
Applied surface utility class, replaced inline badge styles with badge utility classes, improved heading semantics |
src/components/pages/home/calltoaction.tsx |
Converted to semantic section, applied btn utilities, improved gradient and responsive layout |
src/components/pages/Communities/Community.tsx |
Updated to use container-page and surface classes, changed h2 to h1 for proper heading hierarchy, improved search input styling |
src/components/pages/Archive/archive.tsx |
Applied container-page layout, updated filter controls with improved styling, changed h2 to h1, streamlined button text |
src/components/github-button.tsx |
Simplified to use btn-secondary utility class |
src/app/page.tsx |
Removed redundant div wrappers, applied container-page directly to main element |
src/app/not-found.tsx |
Updated gradient colors to use semantic Tailwind utilities |
src/app/layout.tsx |
Removed fixed max-width from body, allowing container-page to control layout width |
src/app/Communities/page.tsx |
Code formatting cleanup, removed unnecessary fragment wrappers |
package-lock.json |
Metadata updates with peer dependency flags (no functional changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </select> | ||
| </div> | ||
| <div className='surface p-6 sm:p-8'> | ||
| <h1 className='section-title'>Archive</h1> |
There was a problem hiding this comment.
The Archive section heading changed from h2 to h1, which is appropriate for a page-level heading. However, it still contains a redundant span wrapper. The h1 should use the section-title class directly rather than wrapping it in a span element.
| <h2 className='mb-3 text-lg font-normal'> | ||
| <span className='text-[30px] font-semibold text-black'>this month</span> | ||
| <span className='section-title'>This month</span> | ||
| </h2> |
There was a problem hiding this comment.
The h2 element contains a span with the section title, creating redundant nesting. The h2 should directly contain the title text or use the section-title class directly on the h2 element instead of wrapping it in a span. This also applies to line 282-284.
| <button | ||
| onClick={() => setSortOrder('asc')} | ||
| className={`px-3 py-2 text-sm font-medium transition ${ | ||
| sortOrder === 'asc' | ||
| ? 'bg-gray-900 text-white' | ||
| : 'bg-white text-gray-700 hover:bg-gray-50' | ||
| }`} | ||
| > | ||
| Asc | ||
| </button> | ||
| <button | ||
| onClick={() => setSortOrder('desc')} | ||
| className={`border-l border-gray-200 px-3 py-2 text-sm font-medium transition ${ | ||
| sortOrder === 'desc' | ||
| ? 'bg-gray-900 text-white' | ||
| : 'bg-white text-gray-700 hover:bg-gray-50' | ||
| }`} | ||
| > | ||
| Desc | ||
| </button> |
There was a problem hiding this comment.
The button text is abbreviated from "Ascending"/"Descending" to "Asc"/"Desc". While this saves space, consider adding aria-label attributes to the buttons to maintain accessibility, as screen reader users may not understand the abbreviated text as clearly. For example: aria-label="Sort ascending" and aria-label="Sort descending".
| <section className='surface relative w-full p-5 sm:p-8'> | ||
| <div className='mb-8'> | ||
| <h2 className='mb-4 text-3xl font-bold text-gray-900'>Tech Communities in Tamil Nadu</h2> | ||
| <h1 className='section-title'>Tech Communities in Tamil Nadu</h1> |
There was a problem hiding this comment.
The heading changed from h2 to h1, which is appropriate for a top-level page heading. However, it still contains a redundant span wrapper. The h1 should use the section-title class directly rather than wrapping it in a span element.
|
@JustinBenito @nammahari @Sarveshmrao your views please. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
can you remove the top header line in the Hero text ? the Discover conferences things, its feels so overpacking |
Removed the header tagline in the Hero component
|
Okay, Done! |
|
Hey lovely work @Kabe-Innovates Cheers :D |
📝 WalkthroughWalkthroughAdds a design-system layer (global CSS variables, components, button/badge variants) and a green color palette; refactors multiple pages/components to use the new classes and simpler layout semantics; adds an /add-event route that returns 404. No runtime logic changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Yeah, Done :) Thanks |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/pages/Communities/Community.tsx (1)
156-166: Avoid usingeval()— security risk and code smell.Using
eval(key)to check if a social link variable exists is dangerous and unnecessary. This pattern can lead to code injection vulnerabilities and makes the code harder to understand.🔒 Proposed fix: Use object lookup instead of eval
<div className='mt-4 flex flex-shrink-0 gap-3 opacity-100 transition-opacity'> - {Object.entries(socialLinks).map(([key, { Icon, color, title, link }]) => - eval(key) ? ( + {Object.entries(socialLinks).map(([key, { Icon, color, title, link }]) => + link ? ( <HoverIcon key={key} Icon={Icon} link={link ?? ''} // Provide a default value for link title={title} hoverColor={color} /> ) : null )} </div>The
linkproperty already exists in thesocialLinksobject and will beundefinedif not provided, so you can simply checklinkdirectly.
🤖 Fix all issues with AI agents
In `@src/app/globals.css`:
- Around line 41-57: Add a missing .btn-glass variant to the design system so
the Communities button in calltoaction.tsx (which uses "btn btn-glass") has
defined styles; create a .btn-glass class alongside
.btn-primary/.btn-secondary/.btn-ghost that composes .btn and applies a
translucent/“glass” background (e.g., semi-transparent white or backdrop-blur,
subtle border and text color) plus hover and disabled states consistent with the
existing utility-based patterns used in globals.css.
♻️ Duplicate comments (5)
src/components/shared/header.tsx (1)
22-22: Typo:h:8should beh-8.The className contains
h:8which is invalid Tailwind syntax. This should beh-8with a hyphen for the height utility to work correctly.🐛 Proposed fix
- className='h:8 w-auto max-[375px]:h-6 sm:h-10 lg:h-12' + className='h-8 w-auto max-[375px]:h-6 sm:h-10 lg:h-12'src/components/pages/home/calltoaction.tsx (1)
43-43:btn-glassclass is not defined in the design system.This button uses
btn btn-glass, butbtn-glassis missing fromglobals.css. The button will only receive basebtnstyles without the glass effect.See the proposed fix in
globals.cssreview to add the missingbtn-glassvariant.src/components/pages/home/events.tsx (2)
254-258: Container updated to use design system classes.The
surface p-6class provides consistent styling. However, the<span className='section-title'>inside<h2>creates redundant nesting — thesection-titleclass could be applied directly to theh2.
281-284: Redundant span wrapper in section heading.Same pattern as line 257 — the span wrapper inside h2 is unnecessary.
src/components/pages/Archive/archive.tsx (1)
294-314: Add aria-labels to sort buttons for accessibility.The abbreviated "Asc"/"Desc" button text may not be clear to screen reader users. Adding
aria-labelattributes improves accessibility.♿ Proposed fix for accessibility
<button onClick={() => setSortOrder('asc')} + aria-label='Sort ascending' className={`px-3 py-2 text-sm font-medium transition ${ sortOrder === 'asc' ? 'bg-gray-900 text-white' : 'bg-white text-gray-700 hover:bg-gray-50' }`} > Asc </button> <button onClick={() => setSortOrder('desc')} + aria-label='Sort descending' className={`border-l border-gray-200 px-3 py-2 text-sm font-medium transition ${ sortOrder === 'desc' ? 'bg-gray-900 text-white' : 'bg-white text-gray-700 hover:bg-gray-50' }`} > Desc </button>
🧹 Nitpick comments (6)
src/components/shared/footer.tsx (2)
7-12: External link missingtarget="_blank"andrel="noreferrer".The FOSS United Chennai link points to an external URL but doesn't open in a new tab, unlike the wiki.tamilnadu.tech link below. For consistency and expected UX with external links, consider adding the same attributes.
Suggested fix
<a href='https://fossunited.org/c/chennai' - className='ml-2 font-medium text-green-700 underline decoration-green-300 underline-offset-4 hover:text-green-800' + className='ml-2 font-medium text-green-700 underline decoration-green-300 underline-offset-4 hover:text-green-800' + target='_blank' + rel='noreferrer' >
16-21: Consider using Next.jsLinkfor internal navigation.The
/archiveroute is an internal link. Using the Next.jsLinkcomponent enables client-side navigation, prefetching, and avoids full page reloads, which improves performance and user experience.Suggested fix
+import Link from 'next/link'; + export default function Footer() {- <a + <Link href='/archive' className='font-medium text-gray-700 underline decoration-gray-300 underline-offset-4 hover:text-gray-900' > Events Archive - </a> + </Link>src/components/github-button.tsx (1)
10-12: Consider inlining or stabilizing the fetch function.
fetchStarsis recreated on each render but referenced inuseEffectwith an empty dependency array. While this works, ESLint'sreact-hooks/exhaustive-depsrule would flag this. Consider inlining the fetch logic or wrapping inuseCallback.♻️ Suggested refactor: inline fetch in useEffect
useEffect(() => { - fetchStars(); - }, []); - - const fetchStars = async () => { - try { - const response = await fetch(`https://api.github.com/repos/fossuchennai/communities`); - if (!response.ok) { - throw new Error('Failed to fetch repository data'); - } - const data = await response.json(); - setStarCount(data.stargazers_count); - } catch (error) { - console.error('Error fetching star count:', error); - } - }; + const fetchStars = async () => { + try { + const response = await fetch(`https://api.github.com/repos/fossuchennai/communities`); + if (!response.ok) { + throw new Error('Failed to fetch repository data'); + } + const data = await response.json(); + setStarCount(data.stargazers_count); + } catch (error) { + console.error('Error fetching star count:', error); + } + }; + fetchStars(); + }, []);src/components/pages/home/calltoaction.tsx (1)
36-38: Consider usingrel="noopener noreferrer"for external link.When using
window.openfor external URLs, adding security attributes prevents the opened page from accessingwindow.opener.♻️ Proposed fix
onClick={() => - window.open('https://github.com/fossuchennai/communities/blob/main/CONTRIBUTING.md') + window.open('https://github.com/fossuchennai/communities/blob/main/CONTRIBUTING.md', '_blank', 'noopener,noreferrer') }src/components/pages/home/events.tsx (1)
317-332: Tooltip component implementation is functional.The inline Tooltip component works correctly. Consider extracting it to a shared component if it's used across multiple files (also present in archive.tsx).
src/components/pages/Archive/archive.tsx (1)
113-260: Consider extracting shared EventCard and Tooltip components.The
EventCardcomponent (lines 113-260) andTooltipcomponent (lines 356-372) are nearly identical to those inevents.tsx. Extracting them to shared components would reduce duplication and improve maintainability.Potential shared component locations:
src/components/shared/EventCard.tsxsrc/components/shared/Tooltip.tsxAlso applies to: 356-372
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
src/app/Communities/page.tsxsrc/app/globals.csssrc/app/layout.tsxsrc/app/not-found.tsxsrc/app/page.tsxsrc/components/github-button.tsxsrc/components/pages/Archive/archive.tsxsrc/components/pages/Communities/Community.tsxsrc/components/pages/home/calltoaction.tsxsrc/components/pages/home/events.tsxsrc/components/pages/home/hero.tsxsrc/components/shared/footer.tsxsrc/components/shared/header.tsxtailwind.config.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/app/Communities/page.tsx (1)
src/app/archive/page.tsx (1)
Communities(3-9)
src/components/pages/Archive/archive.tsx (1)
src/components/no-events-card.tsx (1)
EmptyEventCard(3-15)
🔇 Additional comments (24)
tailwind.config.ts (1)
12-25: LGTM! Well-structured green color palette.The green color scale follows Tailwind conventions with DEFAULT matching 500, and the Material Design-based values provide good contrast ratios across the spectrum. This supports the design system tokens used throughout the PR.
src/app/not-found.tsx (1)
14-16: LGTM! Good migration to design system tokens.Replacing hardcoded hex values with Tailwind's green color tokens (
green-600,green-400) improves maintainability and ensures consistency with the new design system.src/app/page.tsx (1)
9-13: LGTM! Good use of semantic HTML and design system.Using the
<main>element for primary content improves accessibility and document structure. Thecontainer-pageclass centralizes layout constraints, aligning with the design system approach.src/app/layout.tsx (1)
81-83: LGTM! Clean architectural shift for layout constraints.Moving the max-width constraint from the body to the
container-pageutility class in child components is a good pattern—it allows more flexibility for full-bleed sections while maintaining consistent content widths. Themin-h-screenandantialiasedadditions improve layout stability and text rendering.The background color previously on the body (
#fafafa) is properly defined as a CSS variable inglobals.cssand applied via@apply bg-backgroundon the body element, maintaining the intended design while following best practices.src/app/Communities/page.tsx (1)
1-5: LGTM!Clean simplification — removing the unnecessary React fragment wrapper and returning the component directly is the idiomatic approach for single-child renders.
src/app/globals.css (2)
20-65: Well-structured design system.Good use of Tailwind's
@layer componentsfor reusable utility classes. The button variants include proper focus-visible states for accessibility, and the consistent use of design tokens (colors, spacing, border-radius) will improve maintainability.
6-13: The Tailwind configuration intailwind.config.tsalready correctly definesbackgroundandforegroundcolors intheme.extend.colors, with both properly referencing their corresponding CSS variables usingvar(). The@apply bg-background text-foregrounddirectives in globals.css will work as intended. No changes needed.Likely an incorrect or invalid review comment.
src/components/github-button.tsx (1)
28-36: Good responsive text handling.The dual-span approach for responsive text content is a clean pattern — showing abbreviated text on mobile while preserving the full CTA on larger screens.
src/components/shared/header.tsx (2)
42-51: Good accessibility on RSS link.The
aria-labelprovides clear context for screen reader users, and thetitleattribute offers a tooltip for sighted users. Well done.
12-54: Header structure aligns well with the design system.Clean adoption of
container-pagefor consistent page width andbtn btn-ghostfor the RSS link. The responsive logo/icon switching pattern is appropriate for the use case.src/components/pages/home/calltoaction.tsx (1)
9-51: Clean section structure with good visual hierarchy.The gradient card layout, responsive padding, and positioned rocket image create an engaging CTA. Good use of semantic
<section>element and proper text contrast withtext-gray-950against the green gradient.src/components/pages/home/hero.tsx (4)
10-11: Good semantic and layout improvements.Using
<section>instead of<div>improves document semantics. The flex layout with responsive breakpoints (md:flex-row,md:text-left) provides a clean mobile-first approach.
18-46: Typography and color updates align well with the design system.The heading now uses the Tailwind color token
text-gray-950and the accent usestext-green-600, moving away from hardcoded hex values. The responsive font sizes (md:text-[64px],lg:text-[72px]) andtracking-tightprovide good visual hierarchy.
60-60: Responsive image sizing is appropriate.Adding
sm:h-[365px]provides better scaling on small screens while maintaining the base height ofh-[320px].
65-71: Decorative background element updated consistently.The lighter green tint (
bg-green-300/60) and adjusted positioning work well with the overall design system changes.src/components/pages/Communities/Community.tsx (3)
102-102: Card border styling simplified appropriately.The change from a heavy two-tone border to
border border-gray-200aligns with the design system's simpler visual approach.
184-188: Layout and heading structure updated for design system consistency.The use of
container-page,surface, andsection-titleclasses aligns with the new design system. The h1 is appropriate for a page-level heading.
194-194: Search input styling enhanced.The updated styling with
rounded-xl,shadow-sm, and focus ring (focus:ring-4 focus:ring-green-500/10) improves visual feedback and consistency with the design system.src/components/pages/home/events.tsx (3)
174-174: Card border styling simplified consistently.The change to
border border-gray-200matches the styling pattern used in other components (Community.tsx, archive.tsx).
218-218: Title styling updated appropriately.Adding
tracking-tightand usingtext-gray-900improves typography consistency with the design system.
230-232: Badge classes are properly defined in the design system.The
badgeandbadge-greenclasses are correctly defined insrc/app/globals.css(lines 59, 63), and the implementation aligns with the design system approach.src/components/pages/Archive/archive.tsx (3)
183-183: Card border styling simplified consistently.The change to
border border-gray-200matches the pattern across all card components.
263-267: Page layout and heading structure updated appropriately.Using
container-page,surface, andsection-titleclasses aligns with the design system. The h1 is correctly applied without redundant wrapper.
239-241: Badge classes applied consistently.Using
badge badge-greenandbadgematches the pattern inevents.tsx.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Hi! Just checking if anything else is needed from my side to get this merged. Thanks! |
resolve conflicts bro |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/pages/Archive/archive.tsx (2)
51-69:⚠️ Potential issue | 🟠 MajorPre-existing bug:
setEvents(null)overwrites the fallback on fetch failure.While not part of this PR's changes, this is worth flagging since you're actively refactoring this file. When
response.okisfalse, the first.thensets the fallback (setEvents(pastEvents)) and returnsnull. The chained.then((json) => setEvents(json))then overwrites events withnull, which will crash downstream.filter()/.sort()calls.Suggested fix
.then((response) => { if (!response.ok) { setEvents(pastEvents); return null; } return response.json(); }) - .then((json) => setEvents(json)); + .then((json) => { + if (json) setEvents(json); + });
240-251:⚠️ Potential issue | 🟡 MinorBadge row may overflow on narrow viewports.
The location, date, time badges and the calendar button are laid out in a single non-wrapping flex row (
flex … items-center space-x-2). Long location names or date strings could cause horizontal overflow on small screens. Consider addingflex-wrapand switching fromspace-x-2togap-2.Suggested fix
- <div className='flex items-center space-x-2'> + <div className='flex flex-wrap items-center gap-2'>
🧹 Nitpick comments (5)
src/components/pages/Communities/Community.tsx (1)
206-206: Consider extracting the shared input class into the design system.This input styling (
rounded-xl border border-gray-200 bg-white … focus:border-green-500 focus:ring-4 focus:ring-green-500/10) is nearly identical to the<select>styling inarchive.tsx(Line 283). Since the PR is establishing a design system with reusable classes (btn,badge,surface, etc.), it would be consistent to add aninputorform-controlutility class inglobals.cssand reuse it here and in archive.src/components/pages/Archive/archive.tsx (4)
9-9: Unused import:divfromframer-motion/clientis never referenced.This import appears to be dead code —
divisn't used anywhere in this file. It should be removed to avoid unnecessary bundle impact and confusion.-import { div } from 'framer-motion/client';
294-317: Sort segmented control lacks accessible state indicators.Screen readers have no way to determine which sort button is active. Add
role="group"witharia-labelon the wrapper, andaria-pressedon each button.Suggested fix
- <div className='inline-flex overflow-hidden rounded-xl border border-gray-200 bg-white shadow-sm'> + <div + role='group' + aria-label='Sort order' + className='inline-flex overflow-hidden rounded-xl border border-gray-200 bg-white shadow-sm' + > <button onClick={() => setSortOrder('asc')} + aria-pressed={sortOrder === 'asc'} className={`px-3 py-2 text-sm font-medium transition ${ sortOrder === 'asc' ? 'bg-gray-900 text-white' : 'bg-white text-gray-700 hover:bg-gray-50' }`} > Asc </button> <button onClick={() => setSortOrder('desc')} + aria-pressed={sortOrder === 'desc'} className={`border-l border-gray-200 px-3 py-2 text-sm font-medium transition ${ sortOrder === 'desc' ? 'bg-gray-900 text-white' : 'bg-white text-gray-700 hover:bg-gray-50' }`} > Desc </button> </div>
326-345: Use a stable composite key instead of array index.Since this list is sortable and filterable, using
indexas the key causes React to incorrectly reuse DOM nodes across sort/filter changes—this can produce stale hover-state artifacts inEventCard. Compose a key from unique event properties:- <EventCard - key={index} + <EventCard + key={`${event.communityName}-${event.eventName}-${event.eventDate}`}
36-36: Remove dead code:upcomingEventsandupcomingCardHeightare computed but never used.In the archive page,
upcomingEvents(lines 92–98) is filtered and included in the useEffect dependency, but never rendered in the JSX. Similarly,upcomingCardHeight(line 36) is computed in the useEffect but never applied sinceisMonthlyis hardcoded totrue(line 339), making the conditional at line 232 always selectmonthlyCardHeight. Consider removing these to eliminate unnecessary computation on each render.
…ing, fetch fallback
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/components/pages/Archive/archive.tsx (4)
38-43:⚠️ Potential issue | 🟡 Minor
endOfTodayis unused dead code;LoadingAnimationimport is also unreferenced.
endOfToday(lines 42-43) is never read. TheLoadingAnimationimport (line 8) also appears unused after replacing the loading state withEmptyEventCard. Both are leftovers from the refactor.🧹 Cleanup
Remove the unused variable:
- // Create a date object for end of today - const endOfToday = new Date(); - endOfToday.setHours(23, 59, 59, 999);Remove the unused import at line 8:
-import LoadingAnimation from '@/components/LoadingAnimation';
46-66:⚠️ Potential issue | 🟠 MajorMissing
.catch()— network failures leave the archive empty instead of falling back to local data.If the
fetchitself rejects (DNS failure, network offline, CORS error), there's no.catch()handler. Theeventsstate stays as[]and the user sees an empty archive. The localpastEventsfallback should apply here too.🛠️ Proposed fix
.then((json) => { if (json) setEvents(json); - }); + }) + .catch(() => { + setEvents(pastEvents); + });
89-231: 🛠️ Refactor suggestion | 🟠 Major
EventCarddefined insideArchiveis re-created every render — causes unnecessary unmount/remount cycles.Defining a component inside another component's body means React gets a new component reference on each render, treating it as a different type. This unmounts and remounts all
EventCardinstances wheneverArchivere-renders (e.g., on filter/sort changes), destroying internal state like the mouse-tracking glow effect and triggering avoidable DOM churn.
EventCarddoesn't use any closure variables fromArchive— all data flows through props. Extract it to module scope (alongsideTooltip).♻️ Move to module scope
+const EventCard: React.FC<EventCardProps> = ({ + communityName, + title, + ... +}) => { + // ... existing implementation unchanged +}; + const Archive = () => { // ... remove the inner EventCard definition };
332-348:⚠️ Potential issue | 🟡 Minor
Tooltipis mouse-only — keyboard users can't see truncated community names.The tooltip triggers via
onMouseEnter/onMouseLeavebut has noonFocus/onBlurhandlers. Keyboard-navigating users (and screen reader users) can't access the full community name.♿ Minimal keyboard support
- <div className='relative inline-block'> - <div onMouseEnter={() => setShowTooltip(true)} onMouseLeave={() => setShowTooltip(false)}> + <div className='relative inline-block'> + <div + tabIndex={0} + onMouseEnter={() => setShowTooltip(true)} + onMouseLeave={() => setShowTooltip(false)} + onFocus={() => setShowTooltip(true)} + onBlur={() => setShowTooltip(false)} + >Also consider adding
role="tooltip"andaria-describedbyfor screen readers.
🧹 Nitpick comments (2)
src/app/add-event/page.tsx (1)
1-12: Clean pattern for retiring a route.
notFound()throws internally and never returns, soreturn nullon line 11 is unreachable dead code. It's harmless, but you can remove it for clarity.✂️ Optional cleanup
export default function AddEventPage() { notFound(); - return null; }src/app/globals.css (1)
47-62:.form-inputbreaks the logical grouping of button variants.
.form-input(Lines 51-54) is wedged between.btn-primaryand.btn-secondary, splitting the button family. Moving it after all.btn-*variants (or before the button block) would improve scanability of the design system.♻️ Suggested reordering
.btn-primary { `@apply` bg-gray-900 text-white hover:bg-gray-800; } - .form-input { - `@apply` rounded-xl border border-gray-200 bg-white px-3 py-2 text-sm text-gray-900 shadow-sm outline-none transition; - `@apply` focus:border-green-500 focus:ring-4 focus:ring-green-500/10; - } - .btn-secondary { `@apply` bg-gray-100 text-gray-900 hover:bg-gray-200; } .btn-ghost { `@apply` bg-transparent text-gray-700 hover:bg-gray-100; } .btn-glass { `@apply` border border-white/30 bg-white/15 text-white shadow-sm backdrop-blur-md; `@apply` hover:border-white/40 hover:bg-white/25 hover:text-white; `@apply` disabled:border-white/20 disabled:bg-white/10 disabled:text-white/70; } + + .form-input { + `@apply` rounded-xl border border-gray-200 bg-white px-3 py-2 text-sm text-gray-900 shadow-sm outline-none transition; + `@apply` focus:border-green-500 focus:ring-4 focus:ring-green-500/10; + }
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/pages/Archive/archive.tsx`:
- Around line 331-349: The EmptyEventCard currently occupies a single grid
column; wrap or render it with a container that spans all grid columns so it
fills the layout when monthlyEvents is empty. Locate the conditional that
renders EmptyEventCard (near the monthlyEvents.map block) and change the
empty-state render to use a wrapper with a full-span Tailwind class (e.g.,
col-span-full or responsive spans like col-span-1 sm:col-span-2 xl:col-span-3)
or pass a className prop to EmptyEventCard if it supports styling, ensuring it
spans the entire grid width across breakpoints.
- Around line 181-205: The Tooltip component incorrectly assigns role="tooltip"
to the trigger; update Tooltip to generate a stable unique id (useId or useRef)
and remove role="tooltip" from the trigger wrapper, instead add
aria-describedby={tooltipId} to the trigger, and move role="tooltip" and
id={tooltipId} onto the actual tooltip popup element (the absolute div rendered
when showTooltip is true); keep the existing keyboard/mouse handlers and
tabIndex on the trigger so focus/hover still toggle showTooltip.
🧹 Nitpick comments (5)
src/app/add-event/page.tsx (1)
4-7: Metadata may be overshadowed by the not-found boundary.When
notFound()is called, Next.js renders the nearestnot-found.tsxboundary, whose ownmetadata(title:'404') will typically take precedence. Thismetadataexport is unlikely to be served to the client. Not harmful, but worth being aware of — if the intent is to provide a custom title for this specific route's 404, a dedicatednot-found.tsxinsidesrc/app/add-event/would be needed instead.src/components/pages/Archive/archive.tsx (4)
31-174: EventCard and Tooltip are duplicated fromevents.tsx.Per the relevant code snippets,
events.tsxcontains a near-identicalEventCard(with the same hover overlay, overflow detection, venue formatting, andTooltipcomponent). This is a significant DRY violation — bug fixes or design changes will need to be applied in both places.Consider extracting
EventCardandTooltipinto shared modules (e.g.,src/components/EventCard.tsxandsrc/components/Tooltip.tsx) and importing them in botharchive.tsxandevents.tsx.Also applies to: 176-205
42-43: InconsistentReact.useStatevsuseState.Line 42 uses
React.useStatewhile Line 46 usesuseState(the named import). Pick one style for consistency.Proposed fix
- const [mousePosition, setMousePosition] = React.useState<{ + const [mousePosition, setMousePosition] = useState<{ x: number; y: number; } | null>(null);
71-78:validateAndFormatVenueis recreated on every render.This pure function doesn't depend on component state or props. Move it to module scope (alongside EventCard) to avoid unnecessary recreation.
126-128: Redundant fallbacklogo || ''inside a truthy guard.Line 126 already checks
logo &&, sologois guaranteed to be truthy on Line 128. The|| ''is dead code.Proposed fix
- src={logo || ''} + src={logo}
4 participants
Description: This PR introduces a cohesive design system to improve visual consistency and codebase maintainability.
Key Changes
Demonstration.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores