-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create password page #95
base: main
Are you sure you want to change the base?
Conversation
…the border for what problem are you facing
Visit the preview URL for this PR (updated for commit c85b5c2): https://extendafamily-7613e--pr95-create-password-page-wlip7tb8.web.app (expires Sun, 16 Feb 2025 04:23:09 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f8bb7b5cd838ab636347dc54fd0ab08ab3715d31 |
changing the colours for Hover in from what I see in our colour palette, the ordering from lightest to darkest is Light < Hover < Default < Pressed... I'm thinking we'll want to use Pressed to achieve a darker colour for the hover effect. we should confirm the palette with Gaurav again :D |
504d372
to
b3c0a98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUPER AMAZING WORK EVERYTHING WORKS WELL!!! 🧑🍳🧑🍳
I left a few minor comments :D A summary of the more important ones:
- If a user is on the Create Password Page, this means they are a newly invited Admin or Learner that just logged in for the first time. So, after creating their password, they can be sent directly to the HOME_PAGE.
- The original functionality in the
onSubmitNewPasswordClick
event handler of the Create Password Page needs to be kept! This ensures the user's new password actually makes it to our backend, etc. Could the previous logic be restored?
We can discuss more at the work session if needed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light: "#F5FDFF", | ||
Hover: "#CCF5FF", | ||
Hover: "#005566", | ||
Default: "#006877", | ||
Pressed: "#00363F", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want to keep these colours as is and just use the Pressed
variant as the darker shade? this might be a Gaurav question :)
frontend/src/createAdminAccount.ts
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing this file is for testing? 😃
I should have mentioned in the ticket there are /inviteLearner
and /inviteAdmin
routes available for you to invite new users - my bad!
what you have here works too, just remember to remove it before merging :))
frontend/src/components/pages/CreatePasswordConfirmationPage.tsx
Outdated
Show resolved
Hide resolved
useEffect(() => { | ||
validatePassword(newPassword); | ||
}, [newPassword]); | ||
|
||
useEffect(() => { | ||
const passwordsMatch = | ||
newPassword === confirmPassword && newPassword.trim() !== ""; | ||
|
||
setPasswordCriteria((prev) => ({ | ||
...prev, | ||
passwordsMatch, | ||
})); | ||
|
||
const isValid = | ||
passwordCriteria.minLength && | ||
passwordCriteria.uppercase && | ||
passwordCriteria.lowercase && | ||
passwordCriteria.specialChar && | ||
passwordsMatch; | ||
|
||
onValidationChange(isValid); | ||
}, [newPassword, confirmPassword, passwordCriteria, onValidationChange]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: instead of having these as useEffect
s, you can move the logic in each into its own event handler:
function handleNewPasswordChange(newPassword) { ... }
function handleConfirmPasswordChange(confirmPassword) { ... }
and fire these in the onChange
event of the respective input fields
(I can show you what I mean at the next work session if this doesn't make sense!)
functionality-wise shouldn't make a difference, but I believe it's better practice to use event handlers instead of useEffects when possible - https://react.dev/learn/keeping-components-pure#recap
const onSubmitNewPasswordClick = async () => { | ||
const changePasswordSuccess = await AuthAPIClient.updateTemporaryPassword( | ||
authenticatedUser.email, | ||
newPassword, | ||
authenticatedUser.role, | ||
); | ||
if (!changePasswordSuccess) { | ||
setAuthenticatedUser(null); | ||
localStorage.removeItem(AUTHENTICATED_USER_KEY); | ||
await AuthAPIClient.logout(authenticatedUser.id); | ||
const theme = useTheme(); | ||
|
||
// change this later to not use an alert | ||
const onSubmitNewPasswordClick = () => { | ||
if (isFormValid) { | ||
setIsPasswordConfirmed(true); | ||
} else { | ||
// eslint-disable-next-line no-alert | ||
alert("Error occurred when changing your password. Please log in again."); | ||
return; | ||
alert("Passwords do not match or do not meet the criteria."); | ||
} | ||
}; | ||
|
||
const updateStatusSuccess = await AuthAPIClient.updateUserStatus("Active"); | ||
if (!updateStatusSuccess) { | ||
// change this later to not use an alert | ||
// eslint-disable-next-line no-alert | ||
alert('Failed to update user status to "Active"'); | ||
return; | ||
} | ||
const handleOpenHelpModal = () => setIsHelpModalOpen(true); | ||
const handleCloseHelpModal = () => setIsHelpModalOpen(false); | ||
|
||
setAuthenticatedUser({ | ||
...authenticatedUser, | ||
status: "Active", | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should keep the existing logic in onSubmitNewPasswordClick (the red stuff above that was deleted) - this includes the API calls to update the user's password + to update the user's status + to update the authenticatedUser that we need to make the update work :)
variant="headlineLarge" | ||
gutterBottom | ||
sx={{ | ||
fontWeight: theme.typography.headlineLarge?.fontWeight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line is likely redundant - variant="headlineLarge"
should plop the entire style definition, including the fontWeight
, of headlineLarge
fontSize: theme.typography.headlineMedium.fontSize, | ||
fontStyle: "normal", | ||
fontWeight: theme.typography.headlineMedium.fontWeight, | ||
lineHeight: theme.typography.headlineMedium.lineHeight, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do <Typography variant="headlineMedium" ...
and you won't need to write out each of these as styles separately :)
const history = useHistory(); | ||
const theme = useTheme(); | ||
const handleBackToHome = () => { | ||
user.status = "Active"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to change the user's status on the database itself to active because it'll just stay as invited :(
Notion Ticket Link
Create Password Design Implementation
Figma File
Implementation Description
Steps to Test
/create-password
.What should reviewers focus on?
*** * Is the code formatted proeprly? Let us know where additional changes could be made
Checklist