-
Notifications
You must be signed in to change notification settings - Fork 7
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
RFR: Create login/signup functionality #29
Conversation
4fdedff
to
2bb168f
Compare
213170b
to
0720257
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.
Here's my first pass! The only thing I didn't review in depth is the Login
and Register
render code. Thanks for the great work, and for considering lots of different cases and user flows.
frontend/src/pages/Login.js
Outdated
/> | ||
</FormGroup> | ||
<FormGroup> | ||
{/* <Label for="password">Password</Label> */} |
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.
Is this supposed to be commented out?
frontend/src/pages/Register.js
Outdated
|
||
getLoggedInMessage() { | ||
if (this.state.isLoggedIn) { | ||
return <Alert>You are logged in!</Alert>; |
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.
Maybe we should alert "You are registered and logged in!" instead of "logged in"
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.
Although actually, I do wonder whether this message would be seen, if we're doing redirects.
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're definitely right - this was for us to mostly be able to visually debug the JWT storage to avoid having to go into DevTools every time. Now that we're redirecting, we can remove, but upon signup, should we display an alert that's like "welcome"?
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.
Ah I see 😄 Maybe for register, we can delay the redirect (using a timeout
?). So it would be:
- Make register request
- Receive success response from BE, update cookie w JWT, start timeout of 1sec
- Show a success state message, also mention you will be redirected in 1 second
- Redirect occurs when timeout triggers
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.
Reviewing the render content of Register
and Login
-- looks good, just change the text of the submit button
9bf4aaa
to
d76ea83
Compare
frontend/src/components/Login.js
Outdated
} | ||
return this.state.errors.map(({ message, key }) => { | ||
return <FormAlert key={key}>{message}</FormAlert>; | ||
}); | ||
} | ||
|
||
render() { | ||
const message = this.getLoggedInMessage(); |
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.
Style thing -- in the case of functions that return components, my practice has been to put the function inside render
. That is:
render() {
return (
<div>
{this.renderSomePart()}
</div>
);
}
frontend/src/components/FormAlert.js
Outdated
super(props); | ||
} | ||
getColor() { | ||
if (this.props.isRed) { |
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.
Let's do "isDanger" instead of "isRed" -- more meaning
Very small final changes and then we can merge it! |
14fe4eb
to
1449ab9
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.
🚢 🚀 !! Thanks for the thoughtfulness and initiative 🙌
Status:
🚀 Ready
Deploy link
https://phillyreads-oogncirjbb.now.sh
Description
We would like to have working login and register pages integrating with the JWT from the backend. We would also like the page to be styled and easy-to-use.
Related issues: #4
Todos
Screenshots