Added to-do-app #21
Added to-do-app #21
Conversation
There was a problem hiding this comment.
Hey Jai - good job on the assignment! 🚀 In general you seem to have a good grasp on React and how to work with components. I have a few things in general I think you could look at to become even better:
- Could you refactor your app so you don't have to use ".getElementById"? It is not really the React way of doing things.
- If the user inputs (todo text and deadline) is not valid, could the user get a message of some sort to know what they have done wrong?
- Your code formatting could be a little more strict. A few places you have some indented code that should not be indented, some missing spaces here and there etc.
Let me know if you have any questions about my feedback 👍 Have a nice weekend!
| nowDate.setHours(0,0,0,0); | ||
| date.setHours(0,0,0,0); |
There was a problem hiding this comment.
Do you need to set these? new Date() should just return the current time.
| const onAddToDo = () => { | ||
| const newDescription=document.getElementById('newDescription').value; | ||
| const newDeadLine=startDate.getFullYear()+'-'+(startDate.getMonth()+1)+'-'+startDate.getDate(); | ||
| const validDesc=(newDescription.length>0)?true:false; |
There was a problem hiding this comment.
| const validDesc=(newDescription.length>0)?true:false; | |
| const validDesc = newDescription.length > 0; |
In this case you don't need a ternary operator, you can just check if the condition is true or false like this.
| {todos.length>0 | ||
| ? | ||
| todos.map((item,index) => | ||
| <ToDo key={index+1} id={index+1} description={item.description} |
| useEffect(() => { | ||
| let secCounter=0; | ||
| setInterval(function(){ | ||
| secCounter+=1; | ||
| document.getElementById('counter').innerHTML='You have used '+ secCounter +' seconds'; | ||
| }, 1000); | ||
| return () => {}; | ||
| }, []); |
There was a problem hiding this comment.
Could this be set in it's own component, for example in a component called Timer or something?
| return date.getTime()>=nowDate.getTime(); | ||
| }; | ||
| const onAddToDo = () => { | ||
| const newDescription=document.getElementById('newDescription').value; |
There was a problem hiding this comment.
In general when using React it should not be necessary to use the "document.getElementById". In this case, it would be better to have a state variable that holds the text that is currently written in the input field. Then you could use that state instead of using "getElementById". Let me know if it makes sense, else I would be happy to show a more elaborate example.
| setEditMode(true); | ||
| } | ||
| const handleUpdate = (id) => { | ||
| const updatedDesc=document.getElementById('input'+id).value; |
There was a problem hiding this comment.
Kind of the same thing as I commented on above. It should not really be necessary to use getElementById, there are better ways to do it when we are working with React.
| const loadData = async () => { | ||
| const res = await fetch(DEFAULT_API_URL); | ||
| setTodos(await res.json()); | ||
| }; | ||
| useEffect(() => { | ||
| loadData(); | ||
| return () => {}; | ||
| }, []); |
There was a problem hiding this comment.
Nice job of abstracting it into it's own function. If you wanted, you could put the loadData function in it's own file. That would create a better file structure once the app grows and has more api calls.
React/Week3 Homework