Skip to content

version 1#2258

Open
dianakomar wants to merge 3 commits into
mate-academy:masterfrom
dianakomar:develop
Open

version 1#2258
dianakomar wants to merge 3 commits into
mate-academy:masterfrom
dianakomar:develop

Conversation

@dianakomar

Copy link
Copy Markdown

@Denys-Kravchuk9988 Denys-Kravchuk9988 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

A few things to improve:

  1. Edit input should have as equal height as not editable inputs
Image

Comment thread src/components/TodoItem.tsx Outdated
{isProcessed && (
<div
data-cy="TodoLoader"
className={`modal overlay ${isLoading ? 'is-active' : ''}`}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to use classNames here

Comment thread src/components/TodoItem.tsx Outdated
value={editingTitle}
onChange={e => setEditingTitle(e.target.value)}
onBlur={handleSave}
onKeyUp={e => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to extract this handler into a separate callback and use it here

Comment thread src/components/TodoList.tsx Outdated
{tempTodo && (
<TodoItem
todo={tempTodo}
onDelete={async () => {}}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to make this callback optional

Comment thread src/App.tsx Outdated
Comment on lines +32 to +35
case 'Active':
return allTodos.filter(todo => !todo.completed);
case 'Completed':
return allTodos.filter(todo => todo.completed);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this values to separate enum

Comment thread src/components/Filter.tsx
Comment on lines +9 to +13
const filters: { status: FilterType; label: string; href: string }[] = [
{ status: 'All', label: 'All', href: '#/' },
{ status: 'Active', label: 'Active', href: '#/active' },
{ status: 'Completed', label: 'Completed', href: '#/completed' },
];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same way use Enum instead of string values

@dianakomar dianakomar requested a review from brespect June 26, 2026 09:04

@brespect brespect left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants