Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR introduces a restructured layout with a new Footer component, adds new legal and privacy policy pages, removes date badges from event cards, extends the member journey data model to support multiple logos, and updates the testimonials component to render logo arrays with optional links. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
![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
🧹 Nitpick comments (6)
components/UpcomingEventsGrid.tsx (2)
98-116:formattedDateis computed but never used.The date parsing logic remains in the code, but since the date badge display was removed,
formattedDateis now dead code. Consider removing this block to reduce code complexity.🧹 Proposed cleanup
- // Parse the date and handle different formats - let formattedDate = 'Date unavailable' - - try { - const dateStr = event.start_at - - if (dateStr) { - const eventDate = new Date(dateStr) - - if (!isNaN(eventDate.getTime())) { - formattedDate = eventDate.toLocaleDateString('en-US', { - month: 'short', - day: 'numeric', - year: 'numeric' - }) - } - } - } catch (err) { - console.error('Error parsing date:', err, event) - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/UpcomingEventsGrid.tsx` around lines 98 - 116, Remove the unused date-parsing block that defines formattedDate and reads event.start_at inside the UpcomingEventsGrid component: delete the declaration "let formattedDate = 'Date unavailable'", the try/catch that constructs eventDate and sets formattedDate, and the console.error catch — or alternatively replace it by using formattedDate where intended; ensure no references to formattedDate remain in the component so there are no unused variables.
38-40: Remove debug console.log statements before merging.These debug logging statements should be removed for production code as they expose internal API response structure and add unnecessary console noise.
🧹 Proposed fix
- // Log the full API response for debugging - console.log('Upcoming Events API Response:', data) - console.log('Number of upcoming events:', data.entries?.length || 0) -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/UpcomingEventsGrid.tsx` around lines 38 - 40, Remove the two debug console.log statements in the UpcomingEventsGrid component that output 'Upcoming Events API Response:' and 'Number of upcoming events:'; locate these calls inside components/UpcomingEventsGrid.tsx (within the UpcomingEventsGrid component or its data-fetching handler) and delete them, or replace them with a guarded debug/logging mechanism (e.g., a logger.debug or process.env.DEBUG check) if persistent, non-production logging is required.app/legal-notice/page.tsx (1)
1-17: Consider adding a max-width constraint for better readability on wide screens.The content container lacks a
max-w-*constraint, which could cause text to span excessively wide on large screens, reducing readability. The Navigation component usespx-10 lg:px-20with content centered, but this page might benefit from a max-width container similar to other pages.✨ Proposed enhancement
export default function LegalNoticePage() { return ( <div className="min-h-screen bg-brand-dark-blue"> - <div className="mx-auto px-10 lg:px-20 py-20"> + <div className="max-w-7xl mx-auto px-10 lg:px-20 py-20"> <h1 className="text-4xl md:text-5xl font-black text-white mb-12"> LEGAL NOTICE </h1>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/legal-notice/page.tsx` around lines 1 - 17, The page lacks a max-width on the content container which makes text too wide on large screens; update the inner container used in LegalNoticePage (the div with classes "mx-auto px-10 lg:px-20 py-20") to include a sensible max-w class (e.g., max-w-2xl or max-w-prose) or wrap the prose block in a container with a max-w-* class so the "prose prose-invert max-w-none" content gets constrained for improved readability on wide screens.app/privacy-policy/page.tsx (1)
1-17: Consider extracting a shared layout component for legal pages.This page has identical structure to
app/legal-notice/page.tsx. Consider creating a reusableLegalPageLayoutcomponent to reduce duplication and ensure consistent styling across legal pages.Also, the same
max-w-7xlconstraint suggestion from the legal notice page applies here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/privacy-policy/page.tsx` around lines 1 - 17, This page duplicates the structure in app/legal-notice/page.tsx; extract a reusable component (e.g., LegalPageLayout) and update both PrivacyPolicyPage and the LegalNotice page to render inside that component to remove duplication and ensure consistent styling; move the outer wrapper and common classes (min-h-screen bg-brand-dark-blue, mx-auto px-10 lg:px-20 py-20, H1 styling, and the prose container) into LegalPageLayout, accept props for title and children, and adjust the inner container to use the suggested max-w-7xl constraint (replace current max-w-none with max-w-7xl) so both PrivacyPolicyPage and LegalNotice import and use LegalPageLayout.components/Footer.tsx (1)
6-6: Minor: extra space in className attribute and consider responsive padding.There's an extra space before
mx-autoin the className. Also,px-20may cause horizontal overflow issues on small mobile screens.✨ Proposed fix
- <div className= "mx-auto px-20 py-10"> + <div className="mx-auto px-6 md:px-10 lg:px-20 py-10">As per coding guidelines: "Implement responsive design" - the fixed
px-20padding could cause layout issues on smaller viewports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/Footer.tsx` at line 6, In the Footer component update the div with className to remove the leading space before "mx-auto" and replace the fixed "px-20" with responsive padding classes (e.g., use small-screen default like "px-4" and then increase at breakpoints such as "sm:px-6" or "lg:px-20") so it no longer overflows on mobile; locate the div in Footer.tsx (the JSX element containing className "mx-auto px-20 py-10") and apply the corrected className string.components/TestimonialsSection.tsx (1)
70-70: Remove duplicate Tailwind utilityLine 70 includes
w-autotwice. This is harmless but should be cleaned up for readability.Proposed fix
- className="h-5 w-auto w-auto object-contain" + className="h-5 w-auto object-contain"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/TestimonialsSection.tsx` at line 70, In TestimonialsSection.tsx remove the duplicated Tailwind utility by editing the className string on the element that currently has "h-5 w-auto w-auto object-contain" (inside the TestimonialsSection component) so it becomes a single "w-auto" (e.g., "h-5 w-auto object-contain"); update the className value to remove the redundant token for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/TestimonialsSection.tsx`:
- Line 12: The logos prop and rendering in TestimonialsSection currently reuse
item.company for all logo accessible names, causing ambiguous labels; update the
logos type to include a per-logo accessible label (e.g., logos?: { src: string;
url?: string; label?: string }[]) and change the render logic in
TestimonialsSection where logos are mapped to use logo.label (fallback to
item.company only when absent) for both the anchor aria-label and the img alt
attribute so each logo has its own descriptive accessible name.
---
Nitpick comments:
In `@app/legal-notice/page.tsx`:
- Around line 1-17: The page lacks a max-width on the content container which
makes text too wide on large screens; update the inner container used in
LegalNoticePage (the div with classes "mx-auto px-10 lg:px-20 py-20") to include
a sensible max-w class (e.g., max-w-2xl or max-w-prose) or wrap the prose block
in a container with a max-w-* class so the "prose prose-invert max-w-none"
content gets constrained for improved readability on wide screens.
In `@app/privacy-policy/page.tsx`:
- Around line 1-17: This page duplicates the structure in
app/legal-notice/page.tsx; extract a reusable component (e.g., LegalPageLayout)
and update both PrivacyPolicyPage and the LegalNotice page to render inside that
component to remove duplication and ensure consistent styling; move the outer
wrapper and common classes (min-h-screen bg-brand-dark-blue, mx-auto px-10
lg:px-20 py-20, H1 styling, and the prose container) into LegalPageLayout,
accept props for title and children, and adjust the inner container to use the
suggested max-w-7xl constraint (replace current max-w-none with max-w-7xl) so
both PrivacyPolicyPage and LegalNotice import and use LegalPageLayout.
In `@components/Footer.tsx`:
- Line 6: In the Footer component update the div with className to remove the
leading space before "mx-auto" and replace the fixed "px-20" with responsive
padding classes (e.g., use small-screen default like "px-4" and then increase at
breakpoints such as "sm:px-6" or "lg:px-20") so it no longer overflows on
mobile; locate the div in Footer.tsx (the JSX element containing className
"mx-auto px-20 py-10") and apply the corrected className string.
In `@components/TestimonialsSection.tsx`:
- Line 70: In TestimonialsSection.tsx remove the duplicated Tailwind utility by
editing the className string on the element that currently has "h-5 w-auto
w-auto object-contain" (inside the TestimonialsSection component) so it becomes
a single "w-auto" (e.g., "h-5 w-auto object-contain"); update the className
value to remove the redundant token for clarity.
In `@components/UpcomingEventsGrid.tsx`:
- Around line 98-116: Remove the unused date-parsing block that defines
formattedDate and reads event.start_at inside the UpcomingEventsGrid component:
delete the declaration "let formattedDate = 'Date unavailable'", the try/catch
that constructs eventDate and sets formattedDate, and the console.error catch —
or alternatively replace it by using formattedDate where intended; ensure no
references to formattedDate remain in the component so there are no unused
variables.
- Around line 38-40: Remove the two debug console.log statements in the
UpcomingEventsGrid component that output 'Upcoming Events API Response:' and
'Number of upcoming events:'; locate these calls inside
components/UpcomingEventsGrid.tsx (within the UpcomingEventsGrid component or
its data-fetching handler) and delete them, or replace them with a guarded
debug/logging mechanism (e.g., a logger.debug or process.env.DEBUG check) if
persistent, non-production logging is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc4077e8-d10f-4a16-a727-6b63f330e0ec
⛔ Files ignored due to path filters (3)
public/memberJourney/alumni/FelixHaas.pngis excluded by!**/*.pngpublic/memberJourney/alumni/JoshuaCornelius.pngis excluded by!**/*.pngpublic/ourMembers/hero.pngis excluded by!**/*.png
📒 Files selected for processing (10)
app/events/page.tsxapp/layout.tsxapp/legal-notice/page.tsxapp/member-journey/page.tsxapp/members/page.tsxapp/privacy-policy/page.tsxcomponents/Footer.tsxcomponents/PastEventsGrid.tsxcomponents/TestimonialsSection.tsxcomponents/UpcomingEventsGrid.tsx
💤 Files with no reviewable changes (1)
- components/PastEventsGrid.tsx
| quote: string | ||
| logo?: string // Optional additional logo (like partner logo) | ||
| logoAlt?: string // Optional alt text for the logo | ||
| logos?: { src: string; url?: string }[] // Optional logos with optional links |
There was a problem hiding this comment.
Add per-logo accessible labels instead of reusing company name
Line 12 and Line 69 force all logo images/links to share the same label (item.company). With multiple logos, this gives ambiguous/incorrect accessible names.
Proposed fix
export interface TestimonialItem {
id: string
name: string
role: string
company: string // This can be company name or partner name
image: string
story: string
quote: string
- logos?: { src: string; url?: string }[] // Optional logos with optional links
+ logos?: { src: string; alt: string; url?: string }[] // Optional logos with optional links
}
@@
const logoImg = (
<img
src={logo.src}
- alt={item.company}
+ alt={logo.alt}
className="h-5 w-auto w-auto object-contain"
onError={(e) => {
const target = e.target as HTMLImageElement
target.style.display = 'none'
}}
/>
)
return logo.url ? (
- <a key={index} href={logo.url} target="_blank" rel="noopener noreferrer" className="rounded p-1 shadow-lg hover:opacity-80 transition-opacity">
+ <a key={index} href={logo.url} target="_blank" rel="noopener noreferrer" aria-label={logo.alt} className="rounded p-1 shadow-lg hover:opacity-80 transition-opacity">
{logoImg}
</a>
) : (Also applies to: 67-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/TestimonialsSection.tsx` at line 12, The logos prop and rendering
in TestimonialsSection currently reuse item.company for all logo accessible
names, causing ambiguous labels; update the logos type to include a per-logo
accessible label (e.g., logos?: { src: string; url?: string; label?: string }[])
and change the render logic in TestimonialsSection where logos are mapped to use
logo.label (fallback to item.company only when absent) for both the anchor
aria-label and the img alt attribute so each logo has its own descriptive
accessible name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Style