feat: Quality updates for login/registration pages #46
Conversation
ryanpolasky
changed the title
ryanpolasky
left a comment
ryanpolasky
left a comment
There was a problem hiding this comment.
Just a few changes here as well for you to take a look at. Great stuff as always!
| @@ -1,27 +1,53 @@ | |||
| import React, { useState } from "react"; | |||
There was a problem hiding this comment.
We already show explicit password requirements elsewhere, so the strength meter feels redundant and adds vertical bloat to a screen that’s already dense. The requirements list is more actionable than a “weak/strong” bar. My vote would be to keep just the original requirements to avoid mixed signals.
There was a problem hiding this comment.
We should also verify that these exact requirements exist & are checked on the backend. Pretty sure they are, but I'll double check that. Just writing this here as a note for myself :) Disregard this comment! Totally forgot we're operating off of Firebase's existing restrictions, these are are already handles on the backend! :)
| console.error("Signup error:", err); | ||
| if (err && typeof err === "object" && "code" in err && err.code === "auth/email-already-in-use") { | ||
| setEmailError("An account with this email already exists."); | ||
| const { message, code } = getAuthErrorMessage(err); |
There was a problem hiding this comment.
We should be careful about which Firebase error messages are exposed to users. Some codes reveal account existence (like user-not-found, email-already-in-use). Might be worth mapping these to generic user-safe messages instead of returning Firebase’s raw strings. P sure this is an issue anywhere this line occurs, which I think is:
src/app/authentication/page.tsxsrc/app/authentication/createAccount/page.tsxsrc/app/authentication/newPassword/page.tsxforgotPassword/page.tsx
| !getEmailValidationError(email) && | ||
| passwordValidation.isValid && | ||
| !!confirmPassword && | ||
| !validatePasswordMatch(password, confirmPassword); |
There was a problem hiding this comment.
Unless I'm readin it wrong, validatePasswordMatch returns string, not bool, so !validatePasswordMatch() relies on truthiness and will be fragile. We should prolly normalize it to bool like validatePasswordMatch() === "" or !Boolean(validatePasswordMatch()).
| return `${Date.now()}-${Math.random().toString(16).slice(2)}`; | ||
| } | ||
|
|
||
| export function ToastProvider({ children }: { children: React.ReactNode }) { |
There was a problem hiding this comment.
We may want to stack (like x2, x3, etc.) or just outright dedupe identical errors (i.e. rapid login retries or repeated network issues) so we don’t flood the user with stacked popups.
| "resolved": "https://registry.npmjs.org/@firebase/app/-/app-0.14.4.tgz", | ||
| "integrity": "sha512-pUxEGmR+uu21OG/icAovjlu1fcYJzyVhhT0rsCrn+zi+nHtrS43Bp9KPn9KGa4NMspCUE++nkyiqziuIvJdwzw==", | ||
| "license": "Apache-2.0", | ||
| "peer": true, |
There was a problem hiding this comment.
The "peer": true flags added across a bunch entries here look weird for npm output. This doesn’t usually happen from a normal npm install and could Can we regenerate package-lock.json from a clean npm install to make sure dependency resolution is all good?
| <EmailInput | ||
| value={email} | ||
| onChange={handleEmailChange} | ||
| onBlur={() => setEmailError(getEmailValidationError(email))} |
There was a problem hiding this comment.
Validating on blur is nice, but it shows errors while the user is still typing/navigating the page. Notably, going to any of the pages w/ an email entry & then clicking "back" shows an email error before the back is actually processed. Might be worth only showing after the field has been “touched” once.
| <p className="text-red-500 text-xs mt-1">{emailError}</p> | ||
| )} | ||
| </div> | ||
| <form onSubmit={onSubmit} className="flex flex-col"> |
There was a problem hiding this comment.
Wrapping inputs in <form> + handling Enter submit is a great improvement, small change but I'm v glad you caught this!
Added: