-
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
react1-week2/maryam #268
base: main
Are you sure you want to change the base?
react1-week2/maryam #268
Conversation
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.
Well done! @MaryamAarhus 👏 I really like the app, although I left more comments, just fixing the "onSave" method is required, other things are just suggestions:)
Just a note: next time please use the correct folder, right now you are using "week2" folder for the week3
assignment. Thank you!
); | ||
} | ||
|
||
TodoItem.propTypes = { |
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.
first time I am seeing someone use the proptypes in their homework, good job 👏
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 a lot for your heartwarming comments 😊 I highly appreciate it!
@@ -0,0 +1,19 @@ | |||
import React, { useState, useEffect } from 'react'; | |||
|
|||
function Timer() { |
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.
The timer is not used anywhere, you can delete it :)
const [editedDescription, setEditedDescription] = useState(description); | ||
|
||
const handleSave = () => { | ||
onSave(editedDescription); |
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.
The save
function is missing a type declaration. Also it's not working right now. Maybe you have forgotten to implement the functionality?
} else if (new Date(newTodoDeadline) < new Date()) { | ||
setErrorMessage('Deadline cannot be in the past.'); | ||
return; | ||
} |
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.
well done.
}; | ||
|
||
const deleteTodo = (id) => { | ||
setTodos(todos.filter(todo => todo.id !== id)); |
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 it's not incorrect, when you modify data with the set
method from useState
, it's better to use its previous state like this: setTodos(prevTodos => prevTodos.filter(todo => todo.id !== id));
. The reason is that the set
method is asynchronous, meaning it's not guaranteed that todo
will hold the correct data at the time you are modifying it.
function TodoList() { | ||
const [todos, setTodos] = useState([]); | ||
const [newTodoDescription, setNewTodoDescription] = useState(''); | ||
const [newTodoDeadline, setNewTodoDeadline] = useState(''); |
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.
well done with the date time picker! This is just suggestion, but feel free to use it if you want.
- You can define the current date by something like this
const today = new Date().toISOString().split("T")[0];
function TodoList() {
const [newTodoDeadline, setNewTodoDeadline] = useState(today);
Thanks to that the current day will be always as default value
2. set the current value as min
date for the input
<input
type="date"
min={today}
value={newTodoDeadline}
Thanks to this, you do not need to check whether the selected date is in the past, because the picker won't let you pick the date :) I think this could improve the User experience a bit :)
No description provided.