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 solution #1015

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

add solution #1015

wants to merge 3 commits into from

Conversation

RimmaBuzuluk
Copy link

Copy link

@ArturErmolenko ArturErmolenko 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 clean up a bit ;)

src/App.tsx Outdated
export const App = () => {
const [isLoading, setIsLoading] = useState(false);
const dispatch = useDispatch();
const currentUser = useSelector((state: RootState) => state.currentTodo);

Choose a reason for hiding this comment

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

currentUser !== state.currentTodo

src/App.tsx Outdated
getTodos()
.then(todos => dispatch(setTodos(todos)))
.finally(() => setIsLoading(false));
}, []);

Choose a reason for hiding this comment

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

React Hook useEffect has a missing dependency: 'dispatch'. Either include it or remove the dependency array

src/App.tsx Outdated
<TodoModal />
</>
);
{currentUser && <TodoModal todo={currentUser} />}

Choose a reason for hiding this comment

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

todo != user

getUser(todo.userId)
.then(userByTodo => setUser(userByTodo))
.finally(() => setIsLoading(false));
}, []);

Choose a reason for hiding this comment

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

React Hook useEffect has a missing dependency: 'todo.userId'. Either include it or remove the dependency array

Comment on lines 15 to 19
case 'active':
return filtred.filter(todo => todo.completed === false);
case 'completed':
return filtred.filter(todo => todo.completed === true);
case 'all':

Choose a reason for hiding this comment

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

magic strings

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.

Good job 👍
But review my comments and fix them 🥹

src/App.tsx Outdated
Comment on lines 35 to 36
{isLoading && <Loader />}
{!isLoading && <TodoList />}

Choose a reason for hiding this comment

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

Change syntax(In all such cases)

Suggested change
{isLoading && <Loader />}
{!isLoading && <TodoList />}
{
isLoading
? <Loader />
: <TodoList />
}

return (
<form
className="field has-addons"
onSubmit={event => event.preventDefault()}
>
<p className="control">
<span className="select">
<select data-cy="statusSelect">
<select data-cy="statusSelect" value={status} onChange={handleChange}>

Choose a reason for hiding this comment

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

if u have 3 or more attributes, move them to separate lines

Suggested change
<select data-cy="statusSelect" value={status} onChange={handleChange}>
<select
data-cy="statusSelect"
value={status}
onChange={handleChange}
>

Copy link
Author

Choose a reason for hiding this comment

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

I can't do this because if I display the information in three rows, after saving, it automatically returns to the blackberry row

Comment on lines 20 to 22
const handleQuery = (queryPayload: string) => {
dispatch(changeQuary(queryPayload));
};

Choose a reason for hiding this comment

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

this function receive event by default, change it

Suggested change
const handleQuery = (queryPayload: string) => {
dispatch(changeQuary(queryPayload));
};
const handleQuery = (e: ChangeEvent<HTMLInputElement>) => {
dispatch(changeQuary(e.target.value));
};

</td>
</tr>
))}
{/*
<tr data-cy="todo">

Choose a reason for hiding this comment

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

remove all comments from project

Comment on lines 32 to 34
{isLoading && <Loader />}

<Loader />
{!isLoading && (

Choose a reason for hiding this comment

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

The same problem with syntax

isLoading ? (
  <Loader />
) : (
  ......
)

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 🔥

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