[feat] created AuthContext #20
Conversation
| @@ -14,9 +14,10 @@ | |||
| "pre-commit": "(pnpm run tsc || true) && (pnpm run lint || true) && pnpm run prettier" | |||
There was a problem hiding this comment.
cc @ccatherinetan, maybe we want to remove this change & also the package-lock.json change? (do we want/need this change)?
There was a problem hiding this comment.
@pragyakallanagoudar what change are you referring to?
There was a problem hiding this comment.
this comment is fully on the wrong line lol but i'm referring to the one on line 17/20 of this file
There was a problem hiding this comment.
yes sg! we'll delete the @next/swc-darwin-arm64 line
There was a problem hiding this comment.
Looking good so far! I left a few comments and I imagine @ccatherinetan will also want to review soon as well. Good job!
package-lock.json
Outdated
There was a problem hiding this comment.
@ccatherinetan It seems that devs may've accidentally used npm commands and now we have both npm and pnpm lock files. This may cause dependency issues in the future. I'd look into cleaning up the repository by deleting this package-lock.json file.
app/utils/AuthProvider.tsx
Outdated
|
|
||
| // Define the shape of the context | ||
| interface AuthContextType { | ||
| authUser: any; |
There was a problem hiding this comment.
Create an AuthUser type/interface that contains user profile related attributes. This will get rid of the eslint error.
There was a problem hiding this comment.
The authUser attribute would likely have an AuthUser | null type.
app/utils/AuthProvider.tsx
Outdated
| setAuthUser(null); | ||
| setIsLoggedIn(false); |
There was a problem hiding this comment.
Consider when a logout event is sent from the server. This should redirect a user to the /login screen.
| @@ -14,9 +14,10 @@ | |||
| "pre-commit": "(pnpm run tsc || true) && (pnpm run lint || true) && pnpm run prettier" | |||
There was a problem hiding this comment.
@pragyakallanagoudar what change are you referring to?
| export default function LoginLayout() { | ||
| return ( | ||
| <AuthProvider> | ||
| <Login /> | ||
| </AuthProvider> | ||
| ); | ||
| } |
There was a problem hiding this comment.
You only need to wrap the layout.tsx file with the AuthProvider to give its child screens access to context data. It seems that an Auth context would need to wrap the entire app, so look into wrapping the root layout.tsx file with the auth provider.
There was a problem hiding this comment.
+1! See outermost layout.tsx in IJP's codebase
app/dashboard/page.tsx
Outdated
| <p>User is currently: {isLoggedIn ? 'Logged In' : 'Logged Out'}</p> | ||
| {authUser && <p>User name: {authUser.email}</p>}{' '} | ||
| {/* Display user's email */} | ||
| <button onClick={signOut}>Log Out</button> {/* Logout button */} |
There was a problem hiding this comment.
Nitpick: it's better to have separate pages/flows for logged in/out users. However, this page looks like it'll be changed in the future, so keep this in mind when/if it does change.
There was a problem hiding this comment.
nice work @rachaelch3n ! have just left some comments / questions
| export default function LoginLayout() { | ||
| return ( | ||
| <AuthProvider> | ||
| <Login /> | ||
| </AuthProvider> | ||
| ); | ||
| } |
There was a problem hiding this comment.
+1! See outermost layout.tsx in IJP's codebase
app/(auth)/login/layout.tsx
Outdated
| @@ -0,0 +1,5 @@ | |||
| import { AuthProvider } from '../../utils/AuthProvider'; | |||
There was a problem hiding this comment.
You can probably get rid of this whole file when you move the <AuthProvider> wrap out to the outer layout.tsx.
app/utils/AuthProvider.tsx
Outdated
|
|
||
| // Define the shape of the context | ||
| interface AuthContextType { | ||
| authUser: any; |
There was a problem hiding this comment.
@ronniebeggs curious if you have thoughts on whether storing the AuthUser object vs. the Session in the AuthContext would be better? i did the latter in ijp but tbh i have no idea why
What's new in this PR 🧑🌾
Description
Screenshots
How to review
Next steps
Relevant links
Online sources
Related PRs
CC: @ccatherinetan