Solution #2671
Solution #2671
Conversation
src/App.tsx
Outdated
| const [filteredField, setFilteredField] = useState<string>('All'); | ||
| const [query, setQuery] = useState<string>(''); | ||
|
|
||
| const preparedTodos = todos.filter(todo => { | ||
| if (filteredField === 'completed') { | ||
| return todo.completed === true; | ||
| } | ||
|
|
||
| if (filteredField === 'active') { | ||
| return todo.completed === false; | ||
| } |
There was a problem hiding this comment.
Recommendation:
Consider using Enum for limited options eg.
Enum Field {
All = 'all',
Active = 'active',
Completed = 'completed',
}
And replace 'All' with Field.All
There was a problem hiding this comment.
Recommendation: Consider using Enum for limited options eg. Enum Field { All = 'all', Active = 'active', Completed = 'completed', } And replace 'All' with Field.All
Good point
And use switch case instead
src/App.tsx
Outdated
| <TodoModal | ||
| user={user} | ||
| todo={selectedTodo} | ||
| loading={loading} |
There was a problem hiding this comment.
Recommendation: The TodoModal component should have a personal loading state. When you click on the Todo button, the loadings duplicate.
There was a problem hiding this comment.
Recommendation: The TodoModal component should have a personal loading state. When you click on the Todo button, the loadings duplicate.
Good point
There was a problem hiding this comment.
Great job on implementing the task! 🎉 The code meets all the core requirements and functions as expected. There's just a minor typo in the function name handelTodo, which should be corrected to handleTodo for consistency. Keep up the good work and happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/App.tsx
Outdated
| .finally(() => setLoading(false)); | ||
| }, []); | ||
|
|
||
| const handelTodo = (todo: Todo) => { |
There was a problem hiding this comment.
There is a typo in the function name handelTodo. It should be handleTodo to maintain consistency and avoid confusion.
src/App.tsx
Outdated
| {loading && <Loader />} | ||
| <TodoList | ||
| todos={filteredTodos} | ||
| showSelectedTodo={handelTodo} |
There was a problem hiding this comment.
The function handelTodo is used here, but it contains a typo. It should be handleTodo to match the corrected function name.
src/App.tsx
Outdated
|
|
||
| export const App: React.FC = () => { | ||
| const [todos, setTodos] = useState<Todo[]>([]); | ||
| const [loading, setLoading] = useState(false); |
There was a problem hiding this comment.
Boolean variables should start from is or has
src/App.tsx
Outdated
| const [loading, setLoading] = useState(false); | ||
| const [selectedTodo, setSelectedTodo] = useState<Todo | null>(null); | ||
| const [user, setUser] = useState<User | null>(null); | ||
| const [filteredField, setFilteredField] = useState<string>('All'); |
There was a problem hiding this comment.
all/active/completed are magic values
src/App.tsx
Outdated
| const [filteredField, setFilteredField] = useState<string>('All'); | ||
| const [query, setQuery] = useState<string>(''); | ||
|
|
||
| const preparedTodos = todos.filter(todo => { | ||
| if (filteredField === 'completed') { | ||
| return todo.completed === true; | ||
| } | ||
|
|
||
| if (filteredField === 'active') { | ||
| return todo.completed === false; | ||
| } |
There was a problem hiding this comment.
Recommendation: Consider using Enum for limited options eg. Enum Field { All = 'all', Active = 'active', Completed = 'completed', } And replace 'All' with Field.All
Good point
And use switch case instead
src/App.tsx
Outdated
| const filteredTodos = preparedTodos.filter(todo => { | ||
| if (query) { | ||
| return todo.title.toLowerCase().includes(query.toLowerCase()); |
There was a problem hiding this comment.
If you don't have query you don't need to filter at all
src/App.tsx
Outdated
| <TodoModal | ||
| user={user} | ||
| todo={selectedTodo} | ||
| loading={loading} |
There was a problem hiding this comment.
Recommendation: The TodoModal component should have a personal loading state. When you click on the Todo button, the loadings duplicate.
Good point
src/components/Todo/Todo.tsx
Outdated
| <span className="icon"> | ||
| <i className="far fa-eye-slash" /> | ||
| </span> | ||
| ) : ( | ||
| <span className="icon"> | ||
| <i className="far fa-eye" /> |
There was a problem hiding this comment.
It's almost the same in both cases, only one class is different
| <button data-cy="clearSearchButton" type="button" className="delete" /> | ||
| </span> | ||
| {query && ( | ||
| <span className="icon is-right" style={{ pointerEvents: 'all' }}> |
There was a problem hiding this comment.
Avoid inline styles, use classes instead
src/App.tsx
Outdated
| const filteredByField = (() => { | ||
| switch (filteredField) { | ||
| case Field.Completed: | ||
| return todo.completed === true; |
There was a problem hiding this comment.
| return todo.completed === true; | |
| return todo.completed; |
src/App.tsx
Outdated
| case Field.Completed: | ||
| return todo.completed === true; | ||
| case Field.Active: | ||
| return todo.completed === false; |
There was a problem hiding this comment.
| return todo.completed === false; | |
| return !todo.completed; |
src/App.tsx
Outdated
| .finally(() => setIsLoading(false)); | ||
| }, []); | ||
|
|
||
| const handelTodo = (todo: Todo) => { |
There was a problem hiding this comment.
| const handelTodo = (todo: Todo) => { | |
| const handleTodo = (todo: Todo) => { |
typo in word
src/components/Todo/Todo.tsx
Outdated
| <span className="icon"> | ||
| <i | ||
| className={classNames({ | ||
| 'far fa-eye-slash': todo.id === selectedTodo?.id, |
There was a problem hiding this comment.
let's create isSelectedTodo const and reuse
DEMO LINK