add task solution #2729
add task solution #2729
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. The repeated typographical errors with selecteValue and setSelecteValue need to be corrected to selectedValue and setSelectedValue to ensure clarity and prevent potential errors. Additionally, the data fetching logic in the useEffect hook should be optimized to avoid excessive network requests, and error handling should be added to manage potential issues with fetch requests. Once these issues are resolved, your code will be in great shape! Keep up the good work! 🚀
✨ 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.
src/App.tsx
Outdated
| const [todos, setTodos] = useState<Todo[]>([]); | ||
| const [loading, setLoading] = useState(false); | ||
| const [selectedTodo, setSelectedTodo] = useState<Todo | null>(null); | ||
| const [selecteValue, setSelecteValue] = useState('all'); |
There was a problem hiding this comment.
Typographical Error: The state variable selecteValue seems to be a typo. It should likely be selectedValue to better reflect its purpose and avoid confusion.
src/App.tsx
Outdated
| https://mate-academy.github.io/react_dynamic-list-of-todos/api/todos.json`) | ||
| .then(response => response.json()) | ||
| .then((todosFromServer: Todo[]) => { | ||
| switch (selecteValue) { |
There was a problem hiding this comment.
Typographical Error: The variable selecteValue is used here, which seems to be a typo. Consider renaming it to selectedValue.
src/App.tsx
Outdated
| return; | ||
| } | ||
| }); | ||
| }, [selecteValue, inputValue]); |
There was a problem hiding this comment.
Data Fetching Logic: The useEffect hook fetches data every time selecteValue or inputValue changes. This could lead to excessive network requests. Consider fetching the data once and then filtering the todos locally based on these values.
src/App.tsx
Outdated
| @@ -17,18 +68,32 @@ | |||
| <h1 className="title">Todos:</h1> | |||
|
|
|||
| <div className="block"> | |||
| <TodoFilter /> | |||
| <TodoFilter | |||
| selecteValue={selecteValue} | |||
There was a problem hiding this comment.
Typographical Error: The prop selecteValue is likely a typo and should be selectedValue.
| @@ -1,8 +1,24 @@ | |||
| export const TodoFilter = () => ( | |||
| type Props = { | |||
| selecteValue: string; | |||
There was a problem hiding this comment.
Typographical Error: The prop selecteValue seems to be a typo. It should likely be selectedValue to better reflect its purpose and avoid confusion.
|
|
||
| export const TodoFilter: React.FC<Props> = ({ | ||
| selecteValue, | ||
| setSelecteValue, |
There was a problem hiding this comment.
Typographical Error: The function setSelecteValue is used here, which seems to be a typo. Consider renaming it to setSelectedValue.
| <select data-cy="statusSelect"> | ||
| <select | ||
| data-cy="statusSelect" | ||
| value={selecteValue} |
There was a problem hiding this comment.
Typographical Error: The attribute value={selecteValue} is likely a typo and should be value={selectedValue}.
| <select | ||
| data-cy="statusSelect" | ||
| value={selecteValue} | ||
| onChange={event => setSelecteValue(event.target.value)} |
There was a problem hiding this comment.
Typographical Error: The function setSelecteValue is likely a typo and should be setSelectedValue.
| .then(response => response.json()) | ||
| .then(userFromServer => setUser(userFromServer)) | ||
| .finally(() => setLoading(false)); | ||
| }, 1000); |
There was a problem hiding this comment.
Unnecessary Delay: The setTimeout introduces a 1-second delay before fetching user data. Consider removing this delay unless it's needed for a specific purpose, as it can lead to a poor user experience.
| .then(userFromServer => setUser(userFromServer)) | ||
| .finally(() => setLoading(false)); | ||
| }, 1000); | ||
| }, [selectedTodo]); |
There was a problem hiding this comment.
Lack of Error Handling: The fetch request does not handle potential errors. Consider adding error handling to manage network issues or invalid responses.
| const [todos, setTodos] = useState<Todo[]>([]); | ||
| const [loading, setLoading] = useState(false); | ||
| const [selectedTodo, setSelectedTodo] = useState<Todo | null>(null); | ||
| const [selectedValue, setSelectedValue] = useState('all'); |
There was a problem hiding this comment.
better to make enum for or union type for selectedValue state. and better naming will be activeFilter for example.
DEMO LINK