Add hamburger menu, settings accordion, and success confirmation#19
Add hamburger menu, settings accordion, and success confirmation#19andrewsoonqn wants to merge 26 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds a new navigation/settings UI on the dashboard (hamburger menu + settings accordion) and introduces a “settings saved” confirmation experience, while also adjusting initial navigation/auth behavior and updating the production backend URL.
Changes:
- Add
HamburgerMenuandSettingsAccordioncomponents (with success confirmation modal). - Update
AdminDashboardto render the new UI and display event/venue from settings state. - Change init flow to redirect to
/dashboardand update backend prod URL.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/InitDataPage.tsx | Redirects to dashboard and comments out init-data auth flow. |
| frontend/src/pages/AdminDashboard.tsx | Integrates hamburger/settings UI and introduces mocked session/data initialization. |
| frontend/src/components/utils.ts | Updates production backend base URL used by createPath. |
| frontend/src/components/SettingsAccordion.tsx | New accordion UI for event settings + success modal on save. |
| frontend/src/components/HamburgerMenu.tsx | New hamburger/drawer navigation for accessing settings. |
| frontend/package-lock.json | Updates package name metadata in lockfile. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| navigate("/dashboard"); | ||
| }, [navigate]); |
There was a problem hiding this comment.
The page navigates to "/dashboard" unconditionally on mount, which makes the missing-init-data Placeholder effectively unreachable and also bypasses the init-data auth flow. Consider only navigating after successful auth (or at least gate the redirect on initDataRaw being present), and avoid redirecting when initDataRaw is missing.
| @@ -30,6 +37,7 @@ export const InitDataPage: FC = () => { | |||
| } | |||
| initiateAuth(); | |||
| }, [initDataRaw]); | |||
There was a problem hiding this comment.
The init-data authentication effect is commented out, so no JWT/user-type is set from Telegram init data anymore (and createPath is only referenced in this commented block). If auth is intentionally disabled for development, consider guarding a dev-only code path with import.meta.env.DEV and keeping the production auth flow enabled; otherwise, re-enable/remove this block to avoid shipping disabled auth logic.
| // ✅ FIRST useEffect - Setup fake data | ||
| useEffect(() => { | ||
| if (!sessionStorage.getItem("jwt")) { | ||
| sessionStorage.setItem("jwt", "fake-jwt-token-dev"); | ||
| sessionStorage.setItem("user-type", "admin"); | ||
| } | ||
|
|
||
| setQueue([ | ||
| { id: "1", name: "John", joinedAt: new Date() }, | ||
| { id: "2", name: "Jane", joinedAt: new Date() }, | ||
| { id: "3", name: "Bob", joinedAt: new Date() }, | ||
| ]); | ||
| }, []); |
There was a problem hiding this comment.
This effect seeds a fake JWT and forces "user-type" to "admin" in sessionStorage. If this ships beyond local development it effectively bypasses authentication/authorization. Please gate any mock token/data behind import.meta.env.DEV (similar to mockEnv.ts) and keep production behavior dependent on the real auth flow.
| const userType = sessionStorage.getItem("user-type") as ("admin" | "user"); | ||
|
|
There was a problem hiding this comment.
userType is obtained via a forced cast from sessionStorage; if "user-type" is missing or has an unexpected value, userType will be null/invalid at runtime while still being treated as 'admin' | 'user' (and passed into child components). Please validate the value (e.g., default/redirect on missing) instead of casting.
| const userType = sessionStorage.getItem("user-type") as ("admin" | "user"); | |
| const storedUserType = sessionStorage.getItem("user-type"); | |
| if (storedUserType !== "admin" && storedUserType !== "user") { | |
| return (<div>Error</div>); | |
| } | |
| const userType: "admin" | "user" = storedUserType; |
| // ✅ SECOND useEffect - Fetch data (commented out for now) | ||
| useEffect(() => { | ||
| // Skip backend calls for development | ||
| // const fetchData = async () => { ... } | ||
| // fetchData(); | ||
| }, []); |
There was a problem hiding this comment.
This useEffect is currently a no-op (all logic commented out) and can be removed to reduce noise. If it's meant to be a placeholder, consider leaving a short TODO instead of an empty effect so it doesn't look like a missed implementation.
| // ✅ SECOND useEffect - Fetch data (commented out for now) | |
| useEffect(() => { | |
| // Skip backend calls for development | |
| // const fetchData = async () => { ... } | |
| // fetchData(); | |
| }, []); | |
| // TODO: Add backend data fetching here when the API integration is ready. |
| interface Settings { | ||
| eventName: string; | ||
| venue: string; | ||
| notifyBefore: number; | ||
| } |
There was a problem hiding this comment.
The Settings shape is defined locally here and again in SettingsAccordion.tsx. To avoid the two definitions drifting (and to keep props/types consistent), consider extracting this Settings type to a shared module and importing it in both places.
| const handleSave = () => { | ||
| setShowSuccess(true); | ||
| // Auto-hide after 3 seconds | ||
| setTimeout(() => setShowSuccess(false), 3000); | ||
| }; |
There was a problem hiding this comment.
handleSave schedules a setTimeout that updates state later, but the timeout isn't cleared if the component unmounts (or if Save is clicked repeatedly). Track the timer id and clear it in a cleanup (e.g., useEffect cleanup) to avoid updating state on an unmounted component.
| <input | ||
| type="number" | ||
| value={settings.notifyBefore} | ||
| onChange={(e) => handleChange('notifyBefore', parseInt(e.target.value))} | ||
| placeholder="e.g., 5" | ||
| min="0" | ||
| className="w-full px-3 py-2 bg-slate-700 border border-slate-600 rounded-lg text-white placeholder-slate-400 focus:outline-none focus:border-blue-500 focus:ring-1 focus:ring-blue-500" | ||
| /> |
There was a problem hiding this comment.
The notifyBefore onChange uses parseInt(e.target.value); when the input is cleared, parseInt('') becomes NaN and will propagate into settings.notifyBefore. Please handle the empty-string case and/or guard against NaN (e.g., default to 0) before calling onSettingsChange.
| {/* ✅ NEW - Success Modal */} | ||
| {showSuccess && ( | ||
| <div className="fixed inset-0 bg-black/50 flex items-center justify-center z-50"> | ||
| <div className="bg-slate-800 rounded-xl p-8 border border-slate-700 shadow-lg animate-in fade-in zoom-in duration-300"> | ||
| <div className="flex flex-col items-center gap-4"> |
There was a problem hiding this comment.
The success modal is rendered as a generic div without dialog semantics. For accessibility, consider adding role="dialog"/aria-modal, a labelled title (aria-labelledby), and keyboard/focus handling (e.g., focus the modal, allow closing via Escape and/or a close button).
| <button | ||
| onClick={() => setIsOpen(!isOpen)} | ||
| className="fixed top-4 left-4 z-50 p-2 bg-slate-800 rounded-lg hover:bg-slate-700 transition" | ||
| > | ||
| {isOpen ? ( | ||
| <X size={24} className="text-white" /> | ||
| ) : ( | ||
| <Menu size={24} className="text-white" /> | ||
| )} | ||
| </button> |
There was a problem hiding this comment.
The hamburger toggle button has no accessible name/state. Please add an aria-label (or visible text), and expose menu state via aria-expanded plus aria-controls pointing at the nav; also consider basic keyboard support (e.g., closing the menu with Escape and managing focus when the drawer opens).
# Conflicts: # frontend/src/pages/AdminDashboard.tsx
|
Integrated queue setting UI |
No description provided.