Solution #1569
Solution #1569
Conversation
There was a problem hiding this comment.
Good job! I left some comments below. Maybe that will be useful.
Also, I couldn't open App.tsx in review mode, so I've written some notes here:
-
You can drop
finally {}in try\catch block. The thing is, that everything after catch{} block will execute automatically regardless the result. -
Your statements in
if(...)(I guess, I saw that in handleUpdate func and all sorts of it) look a bit too complicated - I guess, that's the result of having too many states and variables. Try to reduce anything that seems useless. You can even reuse your own code here. That will definitely clean it up a bit and make statements easier.
Anyway, that's just small suggestions. The todo app works as indented and that's the point! Good job!
| import { Todo } from '../../types/Todo'; | ||
|
|
||
| type Props = { | ||
| onDoubleClickTodo: (todo: Todo) => void; |
There was a problem hiding this comment.
You have many props here. Some could be created within associated components, so your component won't rely on all of them from the outside.
There was a problem hiding this comment.
You have many props here. Some could be created within associated components, so your component won't rely on all of them from the outside.
Good point
| data-cy="TodoLoader" | ||
| className={classNames('modal overlay', { | ||
| 'is-active': | ||
| idDeletedTodo === id || |
There was a problem hiding this comment.
For this statement, you could create some variable, which combines all possible scenarios. For example: isLoading =todo.id === 0 || isDeleting === todo.id || isUpdating === todo.id || loadingTodos.includes(todo.id) .... Something like that. You can create this variable in your component, outside the render.
There was a problem hiding this comment.
For this statement, you could create some variable, which combines all possible scenarios. For example:
isLoading =todo.id === 0 || isDeleting === todo.id || isUpdating === todo.id || loadingTodos.includes(todo.id) ....Something like that. You can create this variable in your component, outside the render.
I think it should be simplified, just one condition is enough for example store loading todo ids and as AnderZerfall said loadingTodoIds.includes(todo.id)
| {errorState[ErrorType.Input] && `Title should not be empty`} | ||
| {errorState[ErrorType.Add] && `Unable to add a todo`} | ||
| {(errorState[ErrorType.Delete] || | ||
| errorState[ErrorType.CompletedDelete]) && | ||
| `Unable to delete a todo`} | ||
| {errorState[ErrorType.Update] && `Unable to update a todo`} |
There was a problem hiding this comment.
You can just store a message in an error state. As it's a bad idea to do like you, you have to add more and more conditions here for each case
src/components/Footer/Footer.tsx
Outdated
| <a | ||
| href="#/" | ||
| className={classNames('filter__link', { | ||
| selected: filterType === FilterType.All, | ||
| })} | ||
| data-cy="FilterLinkAll" | ||
| onClick={() => { | ||
| setFilterType(FilterType.All); | ||
| }} | ||
| > | ||
| All | ||
| </a> | ||
|
|
||
| <a | ||
| href="#/active" | ||
| className={classNames('filter__link', { | ||
| selected: filterType === FilterType.Active, | ||
| })} | ||
| data-cy="FilterLinkActive" | ||
| onClick={() => { | ||
| setFilterType(FilterType.Active); | ||
| }} |
There was a problem hiding this comment.
The links look almost the same. You can transform FilterType into an array with Object.values/keys/entries and map through them to generate links
src/components/Header/Header.tsx
Outdated
|
|
||
| return ( | ||
| <header className="todoapp__header"> | ||
| {!todos.length || ( |
There was a problem hiding this comment.
| {!todos.length || ( | |
| {!!todos.length && ( |
| import { Todo } from '../../types/Todo'; | ||
|
|
||
| type Props = { | ||
| onDoubleClickTodo: (todo: Todo) => void; |
There was a problem hiding this comment.
You have many props here. Some could be created within associated components, so your component won't rely on all of them from the outside.
Good point
| data-cy="TodoLoader" | ||
| className={classNames('modal overlay', { | ||
| 'is-active': | ||
| idDeletedTodo === id || |
There was a problem hiding this comment.
For this statement, you could create some variable, which combines all possible scenarios. For example:
isLoading =todo.id === 0 || isDeleting === todo.id || isUpdating === todo.id || loadingTodos.includes(todo.id) ....Something like that. You can create this variable in your component, outside the render.
I think it should be simplified, just one condition is enough for example store loading todo ids and as AnderZerfall said loadingTodoIds.includes(todo.id)
| inputEditRef.current.removeEventListener('keydown', handleKeyDown); | ||
| } | ||
| }; | ||
| }, [todoChangedTitle, inputEditRef]); |
There was a problem hiding this comment.
Fix linter errors. And it's better to avoid DOM manipulations in react
src/App.tsx
Outdated
| setTimeout(() => { | ||
| setErrorState(ErrorType.Default); | ||
| }, 3000); |
There was a problem hiding this comment.
please don't forget to clear setTimeout
src/App.tsx
Outdated
| event.preventDefault(); | ||
| setIsAllCompleted(false); | ||
|
|
||
| if (!title.trim().length) { |
There was a problem hiding this comment.
please move title.trim() to const and reuse
src/api/todos.ts
Outdated
| export const USER_ID = 2179; | ||
|
|
||
| export const getTodos = () => { | ||
| return client.get<Todo[]>(`/todos?userId=${USER_ID}`); |
There was a problem hiding this comment.
let's move /todos to const and reuse
src/types/ErrorType.ts
Outdated
| Input = 'Title should not be empty', | ||
| Add = 'Unable to add a todo', | ||
| Delete = 'Unable to delete a todo', | ||
| // CompletedDelete = 'Unable to delete a completed todo', |
DEMO LINK