-
Notifications
You must be signed in to change notification settings - Fork 30
Sofia Lennbom API - Happy thoughts #14
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
base: master
Are you sure you want to change the base?
Conversation
…o /documentation.
…ke as required and minlength of 5 caracters.
…not broken out yet.
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.
💛 Love the use of the in app pop up notifications!
🙂 The smileyface button to login is is cute but not super clear what it does, maybe add a Login-text too?
🌟 After creating a user, Im navigated to the homepage again, maybe this should be to login page instead so the user dont have to go back.
❌ Add a log out button for better user experience.
🌸 Would be nice to also see what user is currently logged in!
🔙 Add a back button at Login page to Home (pressed it by mistake and couldnt find my way back)
📝 When writing wrong password I still navigate to post a thought, but then get an error trying to post. Easy fix: add error msg when wrong password is submitted ✨
🔥 Show min/max password length for user
Well done Sofia, you've done a great job. Just a few things I would recommend to change/add to make the experience better for the user 🐝
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 add user input validation so you dont risk empty fields in your database.
if (!email || !password) {
return res.status(400).json({ success: false, message: "Email and password are required." });
}
}) | ||
|
||
app.get("/users", async (req, res) => { | ||
const { email } = req.params |
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 req.query or change to /users/:email
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.
Superclear structure on the models and its easy to follow your code
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.
Great job with this project! And nice implementation of the frontend too.
I'm missing some proper error message when trying to login with non-existing user. It just closes the popup and that's it. As a user I would like to see some message.
Same goes when trying to register as a user. I can't register and I don't get any message about what went wrong.
Take a look at this and ping me when you have worked it out.
user: { | ||
type: mongoose.Schema.Types.ObjectId, | ||
ref: "User", | ||
required: true | ||
} |
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.
⭐
@@ -0,0 +1,67 @@ | |||
W1: |
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.
Clear todo list! Love it.
server.js
Outdated
//RESET_DB=true npm run dev. DElete when not needed anymore. | ||
// if(process.env.RESET_DB){ | ||
// const seedDatabase = async () => { | ||
// await Thought.deleteMany({}) | ||
// data.forEach(thought => { | ||
// new Thought(thought).save() | ||
// }) | ||
// } | ||
// seedDatabase() | ||
// } | ||
|
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.
IF you dont need this, maybe delete it? Since you have the if statement it is okay to have it "active" if you want to be able to use it with that env variable.
//Endpoint to show all thoughts | ||
//Filter liked thoughts, thoughts?liked, | ||
//Filter thoughts from today thoughts?thoughtsfromtoday |
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.
good comments with explanation
server.js
Outdated
const { newThoughtMessage } = req.body | ||
|
||
try{ | ||
//const thought = await Thought.findByIdAndUpdate(id, { message: newThoughtMessage }, { new: true , runValidators: true }) |
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.
REmove ?
Hi, ready for a new review! |
Please include your Render link here: https://js-project-api-afon.onrender.com
https://happy-thoughts-happy-mind.netlify.app/