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

Develop #1480

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

Conversation

RostyslavSharuiev
Copy link

Copy link

@EustasTheMonk EustasTheMonk left a comment

Choose a reason for hiding this comment

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

Everything looks good but u need to fix the test

Copy link

@igorkruz igorkruz 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 👍
let's improve your solution

Watch the attached video and fix the setting the completed or active status
https://www.loom.com/share/97916aa3be0542939a584748959138ac?sid=b820ea2a-21eb-4aae-879d-4404a2af64fc

Copy link

@polosanya polosanya 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!

Comment on lines +87 to +94
{tempTodo && (
<TodoItem
todo={tempTodo}
setIdsForDelete={setIdsForDelete}
setIdsForUpdate={setIdsForUpdate}
setNewTodoData={setNewTodoData}
/>
)}

Choose a reason for hiding this comment

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

I guess it can be put inside TodoList component

Copy link
Author

Choose a reason for hiding this comment

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

Its makes according to part 2 of the todo app
image

/>
</label>

{!isEditing ? (

Choose a reason for hiding this comment

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

Suggested change
{!isEditing ? (
{isEditing ? (

It is a good practice to use positive boolean for ternary operator condition, not the objection of it

data-cy="TodoLoader"
className={cn('modal overlay', {
'is-active':
!id || idsForDelete?.includes(id) || idsForUpdate?.includes(id),

Choose a reason for hiding this comment

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

Would be great to create a variable for it for better readability

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