react1-week3/anjali #281
react1-week3/anjali #281
Conversation
github-actions
bot
changed the title
There was a problem hiding this comment.
Good job @anjali744, The app looks good and everything is "kinda of" working, but since Developer experience is important as well, lets please go through the comments and try to address them once you once time :) And as always do not hesitate to reach out to me if you get stuck anywhere. Good luck!
| console.log(API_URL) | ||
|
|
||
| const App=()=>{ | ||
| const [json,setjson]=useState([]); |
There was a problem hiding this comment.
the common convention is to jsou always a Capital letter after "set" key, e.g [ json, setJson ] = useState([]) also its not clear what data json holds which can confuse some people, please rename it to todoItems or listData or to something more specific, please :)
| fetch(API_URL) | ||
| .then((response)=>response.json()) | ||
| .then((json)=>{ | ||
| setTimeout(()=>{ |
There was a problem hiding this comment.
- there is no need for setting
timeout, thethenmethod is triggered immediately when you receive your response loadingstate should be set totruebefore you start fetching data not after.- it's a good practice not to trigger any anonymous functions inside of
useEffecthook. Lets make it clearer what the logic does and lets name it 💪 meaning assigning the function
fetch(API_URL)
.then((response)=>response.json())
.then((json)=>{
to a variable (something like const fetchAndSetListData = fetch ..... ) and then call it below, this will increase the readability :)
| <Timer /> | ||
| </div> | ||
| <div> | ||
| {loading ? ( |
There was a problem hiding this comment.
I think you want to do the exact opposite :) The loading should be shown when the data is loading not other way around
| import {FaCalendarAlt} from 'react-icons/fa'; | ||
|
|
||
| //function for calender icon | ||
| function CustomInput({value,onClick}){ |
There was a problem hiding this comment.
move this component to a separate file pls :)
| import React, { useState, useEffect } from 'react'; | ||
| import DatePicker from "react-datepicker"; | ||
| import "react-datepicker/dist/react-datepicker.css"; | ||
| import 'bootstrap/dist/css/bootstrap.min.css' |
There was a problem hiding this comment.
good job with using bootstrap :) but it's better to import it directly in index.js or App.jsx then you can use it everywhere :) https://react-bootstrap.netlify.app/docs/getting-started/introduction/#css
| const [selectedDate, setDate] = useState(new Date()); | ||
|
|
||
| useEffect(() => { | ||
| setTodos(jsonTodos); |
There was a problem hiding this comment.
I think you can move the whole fetching logic here :) There is no need to have useEffect 2x doing the same thing
| checked={todo.completed} | ||
| onChange={() => handleCheckboxChange(todo.id)} | ||
| /> | ||
| <span style={{ marginRight: "10px" }}>{todo.title}</span> |
There was a problem hiding this comment.
the data from your API does not have title parameter but rather description therefore you cannot see the labels. you should either unify it or you can use || condition to allow both variants {todo.title || todo.descsription}
|
|
||
|
|
||
| return ( | ||
| <div className="todo-container"style={{backgroundColor: "#2FBC8F",width:"80%"}} > |
There was a problem hiding this comment.
why you are using somewhere className but somewhere else style prop? I think you can style the whole app just using CSS
| //function for calender icon | ||
| function CustomInput({value,onClick}){ | ||
| return( | ||
| <div className='input-group'> |
There was a problem hiding this comment.
for better User experience I would attach the onClick={onClick} function to this div, then you will be able to open the calendar by clicking on the calendar icon :)
| deadline:selectedDate, | ||
| completed: false | ||
| }; | ||
| setTodos([...todos, newTodoItem]); |
No description provided.