react1-week3/GizemSavci #286
Conversation
There was a problem hiding this comment.
Besides few things, as always @GizemSavci amazing job!I can see all the hard work behind it and I appreciate that you are not afraid to do that one extra step to achieve better results. Big Kudos 👏
| function App() { | ||
| const [todos, setTodos] = useState([]); | ||
|
|
||
| const { data, error } = useSWR("https://gist.githubusercontent.com/benna100/391eee7a119b50bd2c5960ab51622532/raw", fetcher); |
There was a problem hiding this comment.
well, this is cool. good job! Here is some really popular alternative if you are interested :) https://tanstack.com/query/latest
| } | ||
| }, [data]); | ||
|
|
||
| if (!data) return <div>Loading...</div>; |
There was a problem hiding this comment.
we should show the loading state only if the data is really loading and it is empty, otherwise, it could confuse the user by letting him stuck on the loading page even if there is no data. please add isLoading prop from useSwr as well :)
| } | ||
|
|
||
| const removeTodo = (todoIdToRemove) => { | ||
| setTodos(todos.filter(todo => todo.id !== todoIdToRemove)); |
There was a problem hiding this comment.
to keep the code consistent, please use the same approach as you did above in addTodo with prevTodos
|
|
||
| function BorderWrapper({ children }){ | ||
| return ( | ||
| <div style={{ |
There was a problem hiding this comment.
there is no need for inline styling, please use CSS styles from CSS file
|
|
||
| function DateSelector({ selectedDate, onDateChange }) { | ||
| return ( | ||
| <div className='App'> |
There was a problem hiding this comment.
I've noticed that you are using this class in multiple places but I cannot see such a style anywhere if it's not doing anything please remove it :)
|
|
||
| export default Deadline; | ||
|
|
||
| Deadline.propTypes = { |
| @@ -0,0 +1,6 @@ | |||
| let nextId = 0; | |||
| export function generateId() { | |||
There was a problem hiding this comment.
This approach is not safe, it would work only till the user won't refresh the page.
Lets say that you would store the user's data,
- The user comes to the page and will create a todo "some todo" id: 0 (nextId current value)
- He decided that he will the rest later so he closes the browser, then he comes back after some time and create another todo, but because the previous instance of the page was terminated the
nextIdstart from 0 again
There are other ways how to achieve a uniq ID:
- using timestamp, you can use Date API to get the current timestamp, to make it more robust, you can combine it with the todo description
- Math.random method
- maybe overkill for the app of this scale, but the safest approach is using uuid library https://github.com/uuidjs/uuid
| const todo = { | ||
| id:generateId(), | ||
| description: description, | ||
| deadline: selectedDate.toISOString().split('T')[0], |
There was a problem hiding this comment.
This will also cause some issues with in your timezone as well. If you've noticed, the displayed date will differ from the selected one by one day. This is caused by the toISOString method, which converts the date to UTC time. Alternatively, you can use this approach if you know that only people from Denmark will use it.
selectedDate.toLocaleDateString("en-CA", {
timeZone: "Europe/Copenhagen",
});
No description provided.