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 #758

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

Conversation

MariaSnegireva
Copy link

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

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

GJ 👍
Let's add a few small fixes (the main - a lot of lint disabling, why?)

// eslint-disable-next-line
export const TodoProvider = ({ children }: { children: React.ReactNode }) => {
const [todos, setTodos] = useLocalStorage('todos', []);
// eslint-disable-next-line

Choose a reason for hiding this comment

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

why did you disable eslint here? Don't do it w/o good reason - try to figure out with the problem instead

Choose a reason for hiding this comment

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

not fixed

Copy link
Author

Choose a reason for hiding this comment

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

but it's mean about line 22. that I was fixed.

});

// eslint-disable-next-line
export const TodoProvider = ({ children }: { children: React.ReactNode }) => {

Choose a reason for hiding this comment

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

You may use PropsWithChildren<{}> type (import from 'react')

case TodoActions.Completed:
return todo.completed;
default:
return todo;

Choose a reason for hiding this comment

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

explicitly return boolean value - true
also, join TodoActions.All and default cases

Choose a reason for hiding this comment

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

this isn't fixed

export const TodosFilter: React.FC = () => {
const { selectedFilter, setSelectedFilter } = useContext(TodosContext);

const TodoActionsToArr = useCallback((data: typeof TodoActions): string[] => {

Choose a reason for hiding this comment

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

Suggested change
const TodoActionsToArr = useCallback((data: typeof TodoActions): string[] => {
const todoActionsToArr = useCallback((data: typeof TodoActions): string[] => {

camelCase for function names
but you don't need it at all - just use Object.values(TodoActions) w/o memorization (this process also need some resources etc.) or at least return Object.values(TodoActions) from useMemo callback

const TodoActionsToArr = useCallback((data: typeof TodoActions): string[] => {
return Object.values(data);
}, []);
// eslint-disable-next-line

Choose a reason for hiding this comment

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

why disabled?

setIsEditing(false);
};

const handleOnBlur = () => {

Choose a reason for hiding this comment

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

Suggested change
const handleOnBlur = () => {
const handleBlur = () => {

import React, { useContext } from 'react';
import { TodosContext } from '../contexts/TodosContext';

export const ClearCompletedButton: React.FC = () => {

Choose a reason for hiding this comment

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

it's the very small component you could keep in the Footer.tsx - just FYI

Copy link

@denys-danyliuk denys-danyliuk left a comment

Choose a reason for hiding this comment

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

you should fix all the comments before requesting new review, if you have any questions ask them in fe_chat

case TodoActions.Completed:
return todo.completed;
default:
return todo;

Choose a reason for hiding this comment

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

this isn't fixed

// eslint-disable-next-line
export const TodoProvider = ({ children }: { children: React.ReactNode }) => {
const [todos, setTodos] = useLocalStorage('todos', []);
// eslint-disable-next-line

Choose a reason for hiding this comment

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

not fixed

Copy link

@sTorba24 sTorba24 left a comment

Choose a reason for hiding this comment

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

Much better! 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.

4 participants