-
Notifications
You must be signed in to change notification settings - Fork 71
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
implement private route component #234
base: main
Are you sure you want to change the base?
Conversation
Hi @Owobilum, this is currently under review. could you please leave a comment under the issue? That way, I can assign it to you. Thanks! |
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.
Nice work @Owobilum! I noticed though that from the ResponsiveAppBar
the user can click 'Home' and be directed to the Homepage still.
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.
setLoggedIn(true); | ||
navigate("/"); | ||
navigate("/", { state:user }); // store user in route state because "/" route will render with the old user state at this point |
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.
QQ here. If we update the state via our context as we do a few lines up from this, does route "/" render old user state data? I haven't really used this feature from react router dom, but it looks like its a way to carry state across routes. If so our user context is already doing this, correct?
|
||
function PrivateRoute({ children }) { | ||
const location = useLocation(); | ||
const userInLocation = location.state; |
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.
what's the difference here vs the state we are tracking on userContext?
I just have a few questions, it's looking great though! Coming to think of it, we will need additional protected routes but for users who are already signed in, that can't visit pages such as /login, since they are already logged in. This can be taken care of on a different issue! |
Hi @Owobilum , any update? |
What does this PR do?