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

add task solution #741

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

Conversation

Sergej161995
Copy link

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 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.
Please check a couple comments.

Comment on lines 74 to 75
const isSomeComplete = todos.some(todo => todo.completed === true);
const allCompleted = todos.every(todo => todo.completed === true);

Choose a reason for hiding this comment

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

Suggested change
const isSomeComplete = todos.some(todo => todo.completed === true);
const allCompleted = todos.every(todo => todo.completed === true);
const isSomeComplete = todos.some(todo => todo.completed);
const allCompleted = todos.every(todo => todo.completed);

redundant to compare two boolean values

also looks like redundant to make additional loop for isSomeCoplete.
we can compare length of todos and length of noCompleteTodos and will know whether we have some completed todos.

const isSomeComplete = noCompleteTodos.length !== todos.length;

Comment on lines 21 to 23
onClick={() => {
onFilterChange(Filter.ALL);
}}

Choose a reason for hiding this comment

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

suggest to make one handler factory and use for each filter button.

const handleFilter = (filter: Filter) => () => onFilterChange(filter);
Suggested change
onClick={() => {
onFilterChange(Filter.ALL);
}}
onClick={handleFilter(Filter.ALL)}

Copy link

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

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.

2 participants