updated SponsorshipModel UI #181
Conversation
WalkthroughA new SponsorshipModal component is introduced with tabbed interface, modal dialog functionality, keyboard navigation, and focus management. The main entry point is refactored to render a test harness for the modal instead of the Redux-based application. Changes
Sequence DiagramsequenceDiagram
participant User
participant TestApp
participant SponsorshipModal
participant DOM
User->>TestApp: Click "Open Modal" button
TestApp->>TestApp: setOpen(true)
TestApp->>SponsorshipModal: Pass open={true}
SponsorshipModal->>SponsorshipModal: useEffect detects open change
SponsorshipModal->>DOM: Focus modal element (trap focus)
SponsorshipModal->>DOM: Render modal + overlay
User->>SponsorshipModal: Press Tab / Arrow Keys
SponsorshipModal->>SponsorshipModal: Tab activation handler
SponsorshipModal->>DOM: Update active tab and content
User->>SponsorshipModal: Press Escape or Click Close
SponsorshipModal->>SponsorshipModal: onClose triggered
TestApp->>TestApp: setOpen(false)
SponsorshipModal->>DOM: Restore focus to previous element
SponsorshipModal->>DOM: Remove modal from DOM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx (3)
19-34: Add defensive check before restoring focus.Line 33 attempts to restore focus to
prevActivewithout verifying that the element still exists in the DOM or is focusable. If the element was removed while the modal was open, callingfocus()could fail silently or throw an error.Apply this diff to add a defensive check:
return () => { window.removeEventListener("keydown", onKey); - prevActive?.focus(); + if (prevActive && document.contains(prevActive)) { + prevActive.focus(); + } };
167-179: Simplify mailto link handling.Opening a
mailto:URL withwindow.open(mailto, "_blank")is unnecessary and may be blocked by popup blockers. Themailto:protocol doesn't open a new page; it launches the user's email client directly.Apply this diff to simplify:
<button onClick={() => { - // example contact behaviour: open mailto (safe) const mailto = `mailto:[email protected]?subject=Interested in ${encodeURIComponent( t.title || "campaign" )}`; - const win = window.open(mailto, "_blank"); - if (!win) window.location.href = mailto; + window.location.href = mailto; }} className="px-4 py-2 bg-indigo-600 text-white rounded-lg shadow" >
18-35: Consider implementing a focus trap.The current focus management focuses the modal container on open but doesn't prevent users from tabbing out to background content. This violates modal accessibility best practices, as focus should remain trapped within the modal.
Consider using a library like
focus-trap-reactor implementing a custom focus trap that cycles focus back to the first focusable element when the user tabs past the last element (and vice versa).Example with focus-trap-react:
import FocusTrap from 'focus-trap-react'; // Wrap modal content: <FocusTrap> <div ref={dialogRef} tabIndex={-1} className="..."> {/* modal content */} </div> </FocusTrap>Frontend/src/main.tsx (2)
4-4: Use English for code comments.The comment contains non-English text (Hindi/Hinglish). For maintainability in an international codebase, all comments should be in English.
Apply this diff:
-import "./index.css"; // agar file nahi hai to hata sakte ho (nahin to leave it) +import "./index.css"; // can be removed if file doesn't exist
44-45: Root element fallback may cause issues.Creating and appending a root element to
document.bodyif the#rootelement doesn't exist could lead to unexpected rendering behavior and makes debugging harder. It's better to fail fast with a clear error if the expected DOM structure is missing.Apply this diff:
-const rootEl = document.getElementById("root") || document.body.appendChild(document.createElement("div")); -createRoot(rootEl as HTMLElement).render(<TestApp />); +const rootEl = document.getElementById("root"); +if (!rootEl) { + throw new Error("Root element not found. Ensure index.html contains <div id='root'></div>"); +} +createRoot(rootEl).render(<TestApp />);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx(1 hunks)Frontend/src/main.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Frontend/src/main.tsx (1)
Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx (1)
SponsorshipModal(14-192)
🔇 Additional comments (1)
Frontend/src/main.tsx (1)
1-45: PR objectives don't match the actual changes.The PR objectives (from linked issue #87) mention implementing custom scrollbar styling with a pink-to-purple gradient in
App.cssandindex.css, but these changes are not present in this PR. Instead, the PR introduces a newSponsorshipModalcomponent and replaces the main application entry point.Please verify:
- Are the scrollbar styling changes in different files not included in this review?
- Should this PR be split into separate PRs (one for the modal component, one for scrollbar styling)?
- Is the PR description accurate, or does it need to be updated to reflect the actual scope of changes?
| <div className="p-4"> | ||
| <div className="flex gap-2 mb-4 flex-wrap"> | ||
| {tabs.map((tt) => { | ||
| const isActive = tab === tt.id; | ||
| return ( | ||
| <button | ||
| key={tt.id} | ||
| role="tab" | ||
| aria-selected={isActive} | ||
| aria-controls={`tabpanel-${tt.id}`} | ||
| id={`tab-${tt.id}`} | ||
| tabIndex={isActive ? 0 : -1} | ||
| onClick={() => setTab(tt.id)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| setTab(tt.id); | ||
| } | ||
| }} | ||
| className={`px-4 py-2 rounded-full text-sm font-medium transition-shadow focus:outline-none focus:ring-2 focus:ring-indigo-400 ${ | ||
| isActive | ||
| ? "bg-indigo-600 text-white shadow-lg" | ||
| : "bg-gray-200 text-gray-800" | ||
| }`} | ||
| > | ||
| {tt.label} | ||
| </button> | ||
| ); | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
Implement complete ARIA tab pattern for accessibility.
The tab implementation is missing key accessibility requirements per the ARIA Authoring Practices Guide:
- Line 91: The container lacks
role="tablist"to identify it as a tab list. - Lines 103-108: Arrow key navigation (Left/Right/Home/End) is not implemented; tabs should be navigable via arrow keys, not just clickable.
- Line 101: The
tabIndex={isActive ? 0 : -1}pattern prevents keyboard users from tabbing to inactive tabs. The correct pattern is to manage focus with arrow keys while keeping all tabs in the tab sequence.
Apply this diff to add the tablist role:
- <div className="flex gap-2 mb-4 flex-wrap">
+ <div role="tablist" className="flex gap-2 mb-4 flex-wrap">For arrow key navigation, extend the onKeyDown handler to manage focus between tabs:
onKeyDown={(e) => {
if (e.key === "Enter" || e.key === " ") {
e.preventDefault();
setTab(tt.id);
} else if (e.key === "ArrowRight" || e.key === "ArrowLeft") {
e.preventDefault();
const currentIndex = tabs.findIndex((t) => t.id === tab);
const nextIndex =
e.key === "ArrowRight"
? (currentIndex + 1) % tabs.length
: (currentIndex - 1 + tabs.length) % tabs.length;
setTab(tabs[nextIndex].id);
// Focus the next tab button
document.getElementById(`tab-${tabs[nextIndex].id}`)?.focus();
}
}}🤖 Prompt for AI Agents
In Frontend/src/components/SponsorshipModal/SponsorshipModal.tsx around lines
90-119, the tab implementation is missing role="tablist", full keyboard
navigation, and is using a tabIndex pattern that removes inactive tabs from the
tab sequence; add role="tablist" to the wrapping div, change tabIndex so tabs
remain focusable (all tabs should be in the tab sequence), and extend the button
onKeyDown handler to handle ArrowRight, ArrowLeft, Home and End keys: compute
the next/previous/first/last tab index from the tabs array, call setTab(...)
with the new id and programmatically focus the corresponding tab button by its
id, and keep existing Enter/Space behavior; ensure aria-selected/aria-controls
ids remain correct.
| // src/main.tsx — temporary test entry | ||
| import React from "react"; | ||
| import { createRoot } from "react-dom/client"; | ||
| import "./index.css"; | ||
| import { Provider } from "react-redux"; | ||
| import App from "./App.tsx"; | ||
| import store from "./redux/store.ts"; | ||
|
|
||
| createRoot(document.getElementById("root")!).render( | ||
| // <StrictMode> | ||
| <Provider store={store}> | ||
| <App /> | ||
| </Provider> | ||
| // </StrictMode>, | ||
| ); | ||
| import "./index.css"; // agar file nahi hai to hata sakte ho (nahin to leave it) | ||
|
|
||
| import SponsorshipModal from "./components/SponsorshipModal/SponsorshipModal"; | ||
|
|
||
| function TestApp() { | ||
| const [open, setOpen] = React.useState(false); | ||
|
|
||
| const sample = { | ||
| id: "c1", | ||
| title: "Festive Promo", | ||
| brandName: "Sparkle Co", | ||
| brandAvatar: "", | ||
| brandEmail: "[email protected]", | ||
| overview: "This is a sample campaign overview to test the modal.", | ||
| requirements: ["1 Reel", "2 Posts"], | ||
| analytics: { engagementRate: 3.4, avgViews: 12000, matchScore: 80 }, | ||
| }; | ||
|
|
||
| return ( | ||
| <div style={{ padding: 24, fontFamily: "Inter, Arial, sans-serif" }}> | ||
| <h1>Local Test: Sponsorship Modal</h1> | ||
| <button | ||
| onClick={() => setOpen(true)} | ||
| style={{ | ||
| padding: "10px 14px", | ||
| background: "#4f46e5", | ||
| color: "white", | ||
| borderRadius: 8, | ||
| border: "none", | ||
| cursor: "pointer", | ||
| }} | ||
| > | ||
| Open Modal | ||
| </button> | ||
|
|
||
| <SponsorshipModal open={open} onClose={() => setOpen(false)} campaign={sample} /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const rootEl = document.getElementById("root") || document.body.appendChild(document.createElement("div")); | ||
| createRoot(rootEl as HTMLElement).render(<TestApp />); |
There was a problem hiding this comment.
Critical: Test harness must not replace production entry point.
This file completely replaces the production application entry point with a test harness. Merging this to the main branch will break the entire application, as it removes the Redux store, App component, and all production functionality.
Recommended approach:
- Keep the original
main.tsxunchanged. - Create a separate test file (e.g.,
main.test.tsxortest-harness.tsx) for local development testing. - Update your local development script to use the test harness temporarily if needed, but do not commit changes to
main.tsx.
Alternatively, if you want to test the modal within the existing app:
- Import and use
SponsorshipModalwithin the actual application flow (e.g., triggered by a button in an existing page). - Add proper state management integration with Redux if applicable.
| const sample = { | ||
| id: "c1", | ||
| title: "Festive Promo", | ||
| brandName: "Sparkle Co", | ||
| brandAvatar: "", | ||
| brandEmail: "[email protected]", | ||
| overview: "This is a sample campaign overview to test the modal.", | ||
| requirements: ["1 Reel", "2 Posts"], | ||
| analytics: { engagementRate: 3.4, avgViews: 12000, matchScore: 80 }, | ||
| }; |
There was a problem hiding this comment.
Fix type mismatch between campaign object and Props.
The sample campaign object uses field names (brandName, id, brandAvatar, etc.) that don't match the campaign prop type defined in SponsorshipModal.tsx (which expects title?, brand?, overview?). This means the modal will ignore the passed data and fall back to its internal mock values.
Apply this diff to match the expected type:
const sample = {
- id: "c1",
title: "Festive Promo",
- brandName: "Sparkle Co",
- brandAvatar: "",
- brandEmail: "[email protected]",
+ brand: "Sparkle Co",
overview: "This is a sample campaign overview to test the modal.",
- requirements: ["1 Reel", "2 Posts"],
- analytics: { engagementRate: 3.4, avgViews: 12000, matchScore: 80 },
};Note: If you need the full campaign structure with all fields, update the Props type in SponsorshipModal.tsx to include these additional fields and use them in the rendering logic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const sample = { | |
| id: "c1", | |
| title: "Festive Promo", | |
| brandName: "Sparkle Co", | |
| brandAvatar: "", | |
| brandEmail: "[email protected]", | |
| overview: "This is a sample campaign overview to test the modal.", | |
| requirements: ["1 Reel", "2 Posts"], | |
| analytics: { engagementRate: 3.4, avgViews: 12000, matchScore: 80 }, | |
| }; | |
| const sample = { | |
| title: "Festive Promo", | |
| brand: "Sparkle Co", | |
| overview: "This is a sample campaign overview to test the modal.", | |
| }; |
🤖 Prompt for AI Agents
In Frontend/src/main.tsx around lines 11 to 20, the sample campaign object uses
field names (brandName, id, brandAvatar, brandEmail, requirements, analytics)
that don't match the campaign Props expected by SponsorshipModal.tsx (which
expects title?, brand?, overview?, etc.); update the sample object to use the
exact prop names expected by SponsorshipModal (e.g., title, brand: { name,
avatar, email } or whatever shape the component's Props defines) so the modal
receives real data, or alternatively open SponsorshipModal.tsx and extend/adjust
its Props type to include the existing fields (id, brandName, brandAvatar,
brandEmail, requirements, analytics) and update the component rendering to use
those fields consistently.
1 participant
Closes #87 (UI Enhancement for Sponsorship Modal)
📝 Description
This pull request enhances the UI of the SponsorshipModal component to improve clarity, accessibility, and overall user experience.
The update focuses on:
These improvements ensure the modal feels cleaner, more intuitive, and aligned with modern UI patterns.
🔧 Changes Made
role="tab") and ARIA labels.📷 Screenshots (Before & After)
Before
After
🤝 Collaboration
Collaborated with:
@AOSSIE-Orgmaintainers (review pending)✅ Checklist
Summary by CodeRabbit
New Features