got some problem with loader #2629
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this Todo app! While you've done a great job overall, there are a few critical issues that need to be addressed before we can approve the pull request:
-
Typo in Variable Name: The variable
fitchedTodosshould be corrected tofetchedTodos. This is important for code readability and to avoid potential confusion. -
Missing State Update: In the
Filter.Completedcase, thesetFlag(Filter.Completed)is missing. This is crucial to ensure that the application state correctly reflects the current filter.
These issues impact the core functionality of the app, so it's important to address them. Once these are fixed, your code will be in great shape! Keep up the good work, and don't hesitate to reach out if you need any help. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| getTodos() | ||
| .then(fitchedTodos => fitchedTodos.filter(t => !t.completed)) |
There was a problem hiding this comment.
There is a typo in the variable name fitchedTodos. It should be fetchedTodos to maintain consistency and avoid confusion.
| case Filter.Completed: | ||
| getTodos() | ||
| .then(fetchTodos => fetchTodos.filter(t => t.completed)) | ||
| .then(setTodos); | ||
| break; |
There was a problem hiding this comment.
In the Filter.Completed case, the setFlag(Filter.Completed) is missing. This should be added to ensure the state reflects the current filter.
| const handleFilterByValue = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| setInputValue(e.target.value); | ||
| setShowButton(true); | ||
| filteredByValue(inputValue); |
There was a problem hiding this comment.
In handleFilterByValue, the filteredByValue(inputValue) call uses the state inputValue which might not be updated immediately after setInputValue(e.target.value). Consider using e.target.value directly in filteredByValue to ensure the correct value is passed.
| <span | ||
| className="icon" | ||
| onClick={() => getUserAndTodo(todo.userId)} |
There was a problem hiding this comment.
The onClick event handler is placed on the span element inside the button. It would be more appropriate to place the onClick handler directly on the button element to ensure the entire button is clickable, not just the icon.
|
|
||
| <a href="mailto:[email protected]">Leanne Graham</a> | ||
| </p> | ||
| {loader || !user || !todo ? ( |
There was a problem hiding this comment.
The condition loader || !user || !todo is used to decide whether to show the loader. This is correct, but ensure that the logic aligns with the expected behavior: the loader should display when data is being fetched or if the data is incomplete.
No description provided.