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

solution #1562

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

solution #1562

wants to merge 4 commits into from

Conversation

YoNiga7
Copy link

@YoNiga7 YoNiga7 commented Dec 22, 2024

Copy link

@maxsabo maxsabo 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

@BudnikOleksii BudnikOleksii left a comment

Choose a reason for hiding this comment

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

Good work 👍
Let's improve your solution slightly

src/App.tsx Outdated
<div className="todoapp">
<h1 className="todoapp__title">todos</h1>

<header className="todoapp__header">

Choose a reason for hiding this comment

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

Create a header component

src/App.tsx Outdated
<h1 className="todoapp__title">todos</h1>

<header className="todoapp__header">
{/* this button should have `active` class only if all todos are completed */}

Choose a reason for hiding this comment

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

Remove all comments

src/App.tsx Outdated
</header>

<div className="todoapp__content">
<section className="todoapp__main" data-cy="TodoList">

Choose a reason for hiding this comment

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

Create a TodoList component

src/App.tsx Outdated
</div>

<div
data-cy="ErrorNotification"

Choose a reason for hiding this comment

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

Create ErrorNotification component


export const TodoFilter: React.FC<Props> = memo(
({ filter, onFilterChange, todos, onDelete }) => {
const statuses = [

Choose a reason for hiding this comment

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

Move this constant outside the component

Choose a reason for hiding this comment

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

good point

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.

Great job, let's fix a few minor comments

src/App.tsx Outdated

const handleError = useCallback((error: ErrorType) => {
setErrorMsg(error);
setTimeout(() => setErrorMsg(ErrorType.Default), 3000);

Choose a reason for hiding this comment

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

please don't forget to clear setTimeout

export const USER_ID = 2176;

export const getTodos = () => {
return client.get<Todo[]>(`/todos?userId=${USER_ID}`);

Choose a reason for hiding this comment

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

let's move /todos into const and reuse


export const TodoFilter: React.FC<Props> = memo(
({ filter, onFilterChange, todos, onDelete }) => {
const statuses = [

Choose a reason for hiding this comment

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

good point

@YoNiga7 YoNiga7 requested a review from StasSokolov1 December 29, 2024 19:50
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 good to me!

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