react1-week3/anastasiia #275
Conversation
| @@ -0,0 +1,34 @@ | |||
| import React, { useState } from 'react' | |||
|
|
|||
| function AddTodo({addTodo}){ | |||
There was a problem hiding this comment.
Great idea to extract this into its own component with its own state! 👍 Perhaps the validation logic should reside in here as well? I lost my changes when my input failed validation and had to type it again. 🥺
You can also leverage the browser to do some of the validation! If you put the required attribute onto <input>s, the browser will tell the user to fill the fields out before submitting the form. 😄
| @@ -0,0 +1,30 @@ | |||
| import React, { useState, useEffect } from 'react'; | |||
|
|
|||
| const Timer = () => { | |||
There was a problem hiding this comment.
Nice work! ⭐ It does seem like you do not need the startTime state as it is only listed as a dependency which is not in use in your effect. A tiny nitpick is that the React import is unused. 🙂 Love the use of a helper function to format the time to keep the HTML clean! 🤩
| import PropTypes from "prop-types"; | ||
| import Border from "./Border"; | ||
|
|
||
| function TodoList({ todos, toggleDone, deleteTodo, editTodo }) { |
There was a problem hiding this comment.
This component is quite crowded, but it gets the job done. 💪 Can you think of a way to split the functionality up a bit to make it more digestible? 😄
There was a problem hiding this comment.
@NVedsted I do not know actually how to do it correct, or you mean separate it into smaller components in the separate files? Like TodoItem, TodoEdit,TodoList smth like that? :)
| ) | ||
| .then((response) => response.json()) | ||
| .then((data) => { | ||
| setTodos(data); |
There was a problem hiding this comment.
If this fetch was very slow, it could potentially overwrite todos created by the user! 😮 Can you think of a way to avoid this happening?
There was a problem hiding this comment.
@NVedsted I did some changes,
you can have a look?🥺
| <p>No items</p> | ||
| ) : ( | ||
| <TodoList | ||
| todos={todos} |
There was a problem hiding this comment.
The prop types for <TodoList> requires each todo to have a boolean containing whether it is done. This is not the case for the todos provided by the fetch. Perhaps some processing should be done to those fetched todos or the prop types should be modified. What do you think is a good solution? 😄
This also causes some issues with your checkboxes as React assumes them to be uncontrolled when their value is undefined, but then they get a value when you check them off. You can read more about this issue here.
There was a problem hiding this comment.
Nice with some additional error handling and an easier to read async function! I think you are on the right track with the loading state, but you are not using the value anywhere at the moment. If you only show the todo list once data has loaded, then we get around the issue. 😄
React week3 homework:
Add a deadline to the todo item
Create new item using description and deadline that the user inputs
Possibility to update a todoitem
Create proptypes for at least one of the components
Create a border component that wraps a component in a black border