-
Notifications
You must be signed in to change notification settings - Fork 1
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
#539 Chris/add user settings #544
Conversation
Gridiron Survivor Application
Project name: Gridiron Survivor Application
Project name: Gridiron Survivor Application
Only deployments on the production branch are activated automatically. If you'd like to activate this deployment, navigate to your deployments. Learn more about Appwrite Function deployments.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
api/apiFunctions.ts
Outdated
} catch (error) { | ||
console.error(error); | ||
throw new Error('Error logging out user'); | ||
console.error('Password reset failed:', error); |
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.
Yes. Please review.
api/apiFunctions.ts
Outdated
}, | ||
); | ||
} catch (error) { | ||
console.error('Error updating user email:', error); |
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 checked Vercel's error logging. We should remove console.errors because the REST calls should be caught automatically already. Therefore, we dont need console.error.
|
||
form.reset({ oldPassword: '', newPassword: '' }); | ||
} catch (error) { | ||
console.error('Password Update Failed', error); |
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.
Don't think we need this since vercel will pick up all REST api responses already.
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.
✅ resolved all the above comments.
<p className="text-sm"> | ||
This will update the password for your account login. If you | ||
forgot your password you can{' '} | ||
<a |
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.
Use component here.
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.
updated using the built in Next Link component as that is what we use when linking to a specific route in the application. Otherwise we use LinkCustom for external links.
app/(main)/account/settings/page.tsx
Outdated
</LinkCustom> | ||
</div> | ||
<Heading | ||
as="h2" |
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.
H1 if it's the main title.
components/Nav/Nav.test.tsx
Outdated
const drawerTrigger = screen.getByTestId('drawer-trigger'); | ||
fireEvent.click(drawerTrigger); |
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 should really only create a var if it's being reused. Otherwise, it's overkill.
@ryandotfurrer For all the CSS request in this PR, create a new ticket for the UXE team. There are no designs for this, so these are considered nit comments. |
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.
LGTM 🚀
@shashilo done, here is the issue: https://github.com/orgs/LetsGetTechnical/projects/2/views/5?pane=issue&itemId=83698866&issue=LetsGetTechnical%7Cgridiron-survivor%7C612 |
fixes #539
NOTE: This is a branch off my password reset branch which is why it shows so many files changed.
SCREENSHOT