Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create todo app #1557

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Create todo app #1557

wants to merge 3 commits into from

Conversation

AlyonaV22
Copy link

DEMO LINK
Все працює, але падає тест. Було б добре, щоб глянув ментор, в чому може бути причина. Дякую.

Copy link

@natalia646 natalia646 left a comment

Choose a reason for hiding this comment

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

Good job!

Copy link

@VolodymyrKirichenko VolodymyrKirichenko left a comment

Choose a reason for hiding this comment

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

Well done 👍
But review my comments and fix them(If u agree with them)

}, {} as Loading);
};

const todoFilter = todos.filter(todo => {

Choose a reason for hiding this comment

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

move this function to separate component or use useMemo for it

src/App.tsx Outdated
Comment on lines 43 to 45
interface Loading {
[key: number]: number;
}

Choose a reason for hiding this comment

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

move it outside the function

src/App.tsx Outdated
Comment on lines 67 to 177
setTempTodo(null);
});
};

const deleteTodoItem = (todoId: number): void => {
deleteTodo(todoId)
.then(() => {
setTodos(currentTodos =>
currentTodos.filter(todo => todo.id !== todoId),
);
})
.catch(() => {
setErrorType(ErrorType.UnableToDelete);
})
.finally(() => {
setTempTodo(null);
});
};

const updateTodoItems = (
updateTodoItem: Todo,
key: keyof Todo,
value: boolean | string,
): Promise<boolean> => {
return updateTodo({ ...updateTodoItem, [key]: value })
.then(todoUpdated => {
setTodos(currentTodos =>
currentTodos.map(todo =>
todo.id === updateTodoItem.id ? todoUpdated : todo,
),
);

return false;
})
.catch(() => {
setErrorType(ErrorType.UnableToUpdate);

return true;
});
};

const loadedDeleteTodo = (): void => {
const completedTodos = todos.filter(todo => todo.completed);

setTodoLoading(loadingTodo(completedTodos));

Promise.allSettled(
completedTodos.map(todo => deleteTodo(todo.id).then(() => todo)),
)
.then(values => {
values.forEach(val => {
if (val.status === 'rejected') {
setErrorType(ErrorType.UnableToDelete);
} else {
setTodos(currentTodos => {
const todoId = val.value as Todo;

return currentTodos.filter(todo => todo.id !== todoId.id);
});
}
});
})
.finally(() => setTodoLoading({}));
};

const handleAllCompleted = (): void => {
const completedAllTodos = (
targetTodos: Todo[],
completed: boolean,
): Promise<void> => {
return Promise.all(
targetTodos.map(todo => updateTodo({ ...todo, completed })),
)
.then(() => {
setTodos(currentTodos =>
currentTodos.map(todo =>
targetTodos.some(t => t.id === todo.id)
? { ...todo, completed }
: todo,
),
);
})
.catch(() => {
setErrorType(ErrorType.UnableToUpdate);
})
.finally(() => {
setTodoLoading({});
});
};

const activeTodos = todos.filter(todo => !todo.completed);

if (activeTodos.length) {
setTodoLoading(loadingTodo(activeTodos));
completedAllTodos(activeTodos, true);
} else {
setTodoLoading(loadingTodo(todos));
completedAllTodos(todos, false);
}
};

Choose a reason for hiding this comment

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

  1. do we need useCallback for these function? for which one?
  2. use naming convention for all the functions. For all such cases

src/App.tsx Outdated
setErrorType={setErrorType}
onChangeTodoTask={setTempTodo}
tempTodo={tempTodo}
addNewTodo={addNewTodo}

Choose a reason for hiding this comment

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

Suggested change
addNewTodo={addNewTodo}
onAddNewTodo={handleAddNewTodo}

import { Status } from '../types/Status';
import { Todo } from '../types/Todo';

interface LoadingTodo {

Choose a reason for hiding this comment

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

I've seen this type before. Extract it to a separate file and export it.
Import it wherever u need it

[todos],
);

const onTodoCompleted = useMemo(

Choose a reason for hiding this comment

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

on?why?
i see that this variable contains boolean value and doesn't change anything

() => todos.some(todo => todo.completed),
[todos],
);
let onCompletedDelete = false;

Choose a reason for hiding this comment

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

useState?

Comment on lines 45 to 62
</span>
<nav className="filter" data-cy="Filter">
{filtersValue.map(filter => (
<a
key={filter}
href={`#/${filter !== Status.All && filter.toLowerCase()}`}
className={cn('filter__link', {
selected: filter === filterType,
})}
data-cy={`FilterLink${filter}`}
onClick={() => onFiltered(filter)}
>
{filter}
</a>
))}
</nav>
<button
type="button"

Choose a reason for hiding this comment

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

don't forget about indents(for all cases)

Suggested change
</span>
<nav className="filter" data-cy="Filter">
{filtersValue.map(filter => (
<a
key={filter}
href={`#/${filter !== Status.All && filter.toLowerCase()}`}
className={cn('filter__link', {
selected: filter === filterType,
})}
data-cy={`FilterLink${filter}`}
onClick={() => onFiltered(filter)}
>
{filter}
</a>
))}
</nav>
<button
type="button"
</span>
<nav className="filter" data-cy="Filter">
{filtersValue.map(filter => (
<a
key={filter}
href={`#/${filter !== Status.All && filter.toLowerCase()}`}
className={cn('filter__link', {
selected: filter === filterType,
})}
data-cy={`FilterLink${filter}`}
onClick={() => onFiltered(filter)}
>
{filter}
</a>
))}
</nav>
<button
type="button"


const todoUseRef = useRef<HTMLInputElement>(null);

const completedTodos = todos.every(todo => todo.completed);

Choose a reason for hiding this comment

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

useMemo?

Copy link

@StasSokolov1 StasSokolov1 left a comment

Choose a reason for hiding this comment

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

Looks much more better, I'm approving it 🔥

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.

4 participants