Develop#2196
Conversation
Denys-Kravchuk9988
left a comment
Denys-Kravchuk9988
left a comment
There was a problem hiding this comment.
Good job!
A few things to improve:
- It's needed to add loading state clicking on select all caret button (all uncompleted todos should have loading state). The same by click on
Clear completedbutton
| <nav className="filter" data-cy="Filter"> | ||
| <a | ||
| href="#/" | ||
| className={`filter__link ${filter === 'all' ? 'selected' : ''}`} |
There was a problem hiding this comment.
It's better to use classNames here
|
|
||
| <a | ||
| href="#/active" | ||
| className={`filter__link ${filter === 'active' ? 'selected' : ''}`} |
There was a problem hiding this comment.
It's better to use classNames here
| href="#/" | ||
| className={`filter__link ${filter === 'all' ? 'selected' : ''}`} | ||
| data-cy="FilterLinkAll" | ||
| onClick={() => changeFilter('all')} |
There was a problem hiding this comment.
I would recommend to extract 'all' to a separate enum and use this enum here
|
|
||
| <a | ||
| href="#/completed" | ||
| className={`filter__link ${filter === 'completed' ? 'selected' : ''}`} |
There was a problem hiding this comment.
It's better to use classNames here
| href="#/completed" | ||
| className={`filter__link ${filter === 'completed' ? 'selected' : ''}`} | ||
| data-cy="FilterLinkCompleted" | ||
| onClick={() => changeFilter('completed')} |
There was a problem hiding this comment.
I would recommend to extract 'completed' to a separate enum and use this enum here
| }; | ||
|
|
||
| return ( | ||
| <div data-cy="Todo" className={`todo ${todo.completed ? 'completed' : ''}`}> |
There was a problem hiding this comment.
It's better to use classNames here
| )} | ||
| <div | ||
| data-cy="TodoLoader" | ||
| className={`modal overlay ${isLoading || isSaving ? 'is-active' : ''}`} |
There was a problem hiding this comment.
It's better to use classNames here
| {todos.length > 0 && ( | ||
| <button | ||
| type="button" | ||
| className={`todoapp__toggle-all ${areAllCompleted ? 'active' : ''}`} |
There was a problem hiding this comment.
It's better to use classNames here
| {/* Add the 'hidden' class to hide the message smoothly */} | ||
| <div | ||
| data-cy="ErrorNotification" | ||
| className={`${errorMessage ? '' : 'hidden'} notification is-danger is-light has-text-weight-normal`} |
There was a problem hiding this comment.
It's better to use classNames here
| href="#/active" | ||
| className={`filter__link ${filter === 'active' ? 'selected' : ''}`} | ||
| data-cy="FilterLinkActive" | ||
| onClick={() => changeFilter('active')} |
There was a problem hiding this comment.
I would recommend to extract 'active' to a separate enum and use this enum here
| const handleToggleAllButton = async () => { | ||
| try { | ||
| await handleToggleAll(); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| setErrorMessage(error.message); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const handleClearCompleted = async () => { | ||
| try { | ||
| await clearCompleted(); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| setErrorMessage(error.message); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
you should use the promise.all() here
| updateTodoTitle, | ||
| } = useTodos(); | ||
| const [fieldValue, setFieldValue] = useState(''); | ||
| const [filter, setFilter] = useState('all'); |
There was a problem hiding this comment.
create enum for the filter parameters
| <a | ||
| href="#/" | ||
| className={classNames('filter__link', { | ||
| selected: filter === FilterType.all, | ||
| })} | ||
| data-cy="FilterLinkAll" | ||
| onClick={() => changeFilter(FilterType.all)} | ||
| > | ||
| All | ||
| </a> | ||
|
|
||
| <a | ||
| href="#/active" | ||
| className={classNames('filter__link', { | ||
| selected: filter === FilterType.active, | ||
| })} | ||
| data-cy="FilterLinkActive" | ||
| onClick={() => changeFilter(FilterType.active)} | ||
| > | ||
| Active | ||
| </a> | ||
|
|
||
| <a | ||
| href="#/completed" | ||
| className={classNames('filter__link', { | ||
| selected: filter === FilterType.completed, | ||
| })} | ||
| data-cy="FilterLinkCompleted" | ||
| onClick={() => changeFilter(FilterType.completed)} | ||
| > | ||
| Completed | ||
| </a> |
There was a problem hiding this comment.
you can simplify it. create an array with the needed parameters and then map through it to create links and avoid code repetition
4 participants
DEMO LINK