Skip to content
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-week3/yousseftopaji #283

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

yousseftopaji
Copy link
Contributor

No description provided.

@github-actions github-actions bot changed the title React1 week3/yousseftopaji react1-week3/yousseftopaji Apr 3, 2024
Copy link

@BrantalikP BrantalikP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done @yousseftopaji 👏 I left few comments, but overall very good.

Just next time please push only the relevant parts of code, so it's clear what is a part of the assignment and whats not. :) Thank you


const handleUpdate = () => {
if (newDescription.trim()) {
if (todo.deadline && new Date(todo.deadline) < new Date()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think it is necessary to check the deadline if you want to change just the description and not the deadline itself, also right now if you try to edit an element with the deadline in past, there is no other choice than delete the element otherwise you got stuck

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think you were supposed to update all todo's props deadline included

const [deadline, setDeadline] = useState(null);

useEffect(() => {
fetch(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

although not an issue it's not a good practice to have an anonymous function inside of the useEffect hook
you can assign the fetch logic to a variable (e.g fetchAndSetTodos) and call it afterward inside of the hooks :)

<h1>Todo List</h1>
{todos.length === 0 ? (
<>
{" "}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can delete this line :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants