Conversation
leec18
commented
Apr 6, 2026
leec18
commented
- Created EmailForm.tsx, OtpForm.tsx, and page.tsx for the login UI
- Implement two-step form state: email step → OTP step
- Includes a resend code button with a 15-second cooldown timer
- Not connected to the backend yet
- Login page can be accessed by the /login route
|
Note: adding in the SGA logo added a browser warning regarding size styling "[browser] Image with src "http://localhost:3000/vercel.svg" has either width or height modified, but not the other. If you use CSS to change the size of your image, also include the styles 'width: "auto"' or 'height: "auto"' to maintain the aspect ratio." |
pataniaeli
left a comment
pataniaeli
left a comment
There was a problem hiding this comment.
Review written by a Claude agent.
General implementation review — this one's UI-only and not security-sensitive, so no sequencing caveats needed here, just concrete items:
- Logo path case mismatch that will 404 in production (see inline comment on
page.tsx). format:checkcurrently fails on every changed file (verified withprettier --check) — this repo's CLAUDE.md treats that as a merge gate.- New CSS added as bare element selectors bleeds outside this page and skips the existing dark-mode token system (see inline comment on
globals.css). - Missing
autocompleteattributes and<label>s on both inputs (see inline comment onEmailForm.tsx). OtpForm's submit is currently a no-op — flagging in case it's not intentional, but this reads like an expected UI-only stub given the PR scope (see inline comment).
Nice-to-have, not blocking: EmailForm and OtpForm share identical wrapper/button class strings — might be worth a shared component given the same login pattern will likely repeat for the other project integrations mentioned in CLAUDE.md.
| return ( | ||
| <main className="min-h-screen bg-gradient-to-b from-white to-red-200"> | ||
| <div className="flex flex-col items-center justify-center"> | ||
| <Image className="m-8" src="/SGA_logo.png" alt="Logo" width={350} height={150} style={{ height: "auto" }} loading="eager" /> |
There was a problem hiding this comment.
Small but will bite in prod: this references /SGA_logo.png (lowercase "l"), but the asset this PR adds is public/SGA_Logo.png (capital "L"). Works locally since most dev filesystems are case-insensitive, but Vercel/Linux serves public/ case-sensitively, so this 404s once deployed.
| font-family: Arial, Helvetica, sans-serif; | ||
| } | ||
|
|
||
| h1 { |
There was a problem hiding this comment.
These are bare element selectors (h1, button) added outside the existing @layer base block above, so under CSS cascade-layer rules they'll take precedence over any Tailwind utility class on a button or h1 anywhere else in the app — not just this login page — and they don't participate in the dark-mode variables already defined earlier in this file. Scoping this to the two buttons on the login page (Tailwind utility classes, or a .btn-primary class) would avoid the app-wide side effect.
| <form onSubmit={handleSubmit} className="w-125 bg-white flex flex-col items-center justify-center p-8 text-xl rounded-4xl shadow-lg"> | ||
| <input | ||
| className="w-full p-4 m-4 bg-gray-200 rounded-lg" | ||
| type="email" |
There was a problem hiding this comment.
Worth adding autoComplete="email" here (and autoComplete="one-time-code" on the OTP input in OtpForm.tsx) so browsers/password managers and mobile OTP autofill work as expected. Also, both inputs rely on placeholder alone with no <label>/aria-label — placeholder text isn't an accessible name substitute once the field has focus or a value.
| return () => clearTimeout(t); | ||
| }, [cooldown]); | ||
|
|
||
| function handleSubmit(e: React.SubmitEvent) { |
There was a problem hiding this comment.
Assuming this is intentional for a UI-only stub (nothing in this PR wires up sendOtp/verifyOtp yet, which tracks with the PR description), but flagging so it's not mistaken for a bug during review or a demo: this handler currently just calls preventDefault() with no state change and no onVerify/onSuccess prop for the parent to hook into, unlike EmailForm's equivalent handler which at least calls onNext().
2 participants
