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

implement editing functionality #778

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

Conversation

voievodik
Copy link

@voievodik voievodik commented Sep 21, 2023

Copy link

@roman-mirzoian roman-mirzoian 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 there is quite a bit of work to be done.
Also, currently there are problems with updating the todo and deleting it if the todo title is empty:
https://gyazo.com/dfe0984f277df2035e92bef659455220

const [newTitle, setNewTitle] = useState(todo.title);

const handleEditTodo = () => {
if (newTitle === '') {

Choose a reason for hiding this comment

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

Suggested change
if (newTitle === '') {
if (!newTitle) {

Comment on lines 19 to 27
todos
.filter(({ completed }) => completed)
.forEach(({ id }) => {
deleteTodo(id)
.then(() => {
setTodos(prevState => prevState.filter(todo => todo.id !== id));
})
.catch(() => setErrorMessage('Unable to delete a todo'));
});

Choose a reason for hiding this comment

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

There is no need to do unnecessary filtering and go through the array twice, it is enough to check the status of the todo during the traversal through the forEach.

Comment on lines 38 to 40
const todosCopy = [...currentTodos];

return todosCopy.map(todo => ({

Choose a reason for hiding this comment

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

Map is a non-mutating array method, so you don't need to make a copy manually.

} = useContext(ErrorContext);

useEffect(() => {
if (errorMessage.length !== 0) {

Choose a reason for hiding this comment

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

Suggested change
if (errorMessage.length !== 0) {
if (errorMessage.length) {

Comment on lines 21 to 29
// useEffect(() => {
// const todosStorage = localStorage.getItem('todos');

// setTodos(JSON.parse(todosStorage || '[]') as Todo[]);
// }, []);

// useEffect(() => {
// localStorage.setItem('todos', JSON.stringify(todos));
// }, [todos]);

Choose a reason for hiding this comment

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

Do not leave unnecessary comments.

Copy link

@AndrMar1939 AndrMar1939 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 with Context!

Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

  1. Not implemented
image
  1. I can rename todo and enter just spaces
image
  1. Why I see loader on all todos, if I'm making changes just in 1
image
  1. "All, Active, Completed" buttons should not jump when I select or unselect all todos
image image
  1. When I have 1 todo checked and click "toggleAll" button, I expect that all todos now will be selected, but in your app, it works in a different way, check the working example how it should work

src/App.tsx Outdated
Comment on lines 34 to 36
<TodoList
isActive={isActive}
/>

Choose a reason for hiding this comment

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

If you have just 1 prop -> better to write your component in 1 line

Comment on lines 65 to 93
{todoTemp && (
<div data-cy="Todo" className="todo">
<label className="todo__status-label">
<input
data-cy="TodoStatus"
type="checkbox"
className="todo__status"
/>
</label>

<span data-cy="TodoTitle" className="todo__title">
{todoTemp?.title}
</span>

<button type="button" className="todo__remove" data-cy="TodoDelete">
×
</button>

<div
data-cy="TodoLoader"
className={classnames(
'modal overlay', {
'is-active': isActive,
},
)}
>
<div className="modal-background has-background-white-ter" />
<div className="loader" />
</div>

Choose a reason for hiding this comment

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

I would move this to separate component

aria-label="Delete"
onClick={() => setIsErrorHidden(true)}
/>

Choose a reason for hiding this comment

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

Suggested change

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 👍
But not all works as expected, left some suggestions, check them.
And a lot of tests failed, fix your solution:
image

src/App.tsx Outdated
Comment on lines 12 to 13
const { todos, setTodos } = useContext(TodoContext);
const { setErrorMessage } = useContext(ErrorContext);

Choose a reason for hiding this comment

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

Create hooks for contexts so you don't need to pass context every time, for example:

const useTodo = () => useContext(TodoContext);

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

<div className="todoapp__content">
<TodoHeader onHandleActive={(value) => setIsActive(value)} />

Choose a reason for hiding this comment

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

Suggested change
<TodoHeader onHandleActive={(value) => setIsActive(value)} />
<TodoHeader onHandleActive={setIsActive} />

src/App.tsx Outdated

<TodoList
isActive={isActive}
onHandleActive={(value) => setIsActive(value)}

Choose a reason for hiding this comment

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

Suggested change
onHandleActive={(value) => setIsActive(value)}
onHandleActive={setIsActive}

Comment on lines 40 to 47
const todosCopy = [...todos];
const index = todos.findIndex(({ id: currentId }) => currentId === id);
const updatedTodo = {
...todosCopy[index],
title: newTitle,
};

todosCopy.splice(index, 1, updatedTodo);

Choose a reason for hiding this comment

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

Don't use splice and findIndex, just use map instead

Comment on lines 19 to 27
todos.forEach(({ id, completed }) => {
if (completed) {
deleteTodo(id)
.then(() => {
setTodos(prevState => prevState.filter(todo => todo.id !== id));
})
.catch(() => setErrorMessage('Unable to delete a todo'));
}
});

Choose a reason for hiding this comment

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

Don't use forEach for async operations, use for of with async/await or collect requests with mapand thenPromise.all`.

Also, add loaders to notify users what todos will be deleted.

Comment on lines 36 to 54
const toggleAll = () => {
const allCompleted = todos.every(({ completed }) => completed);

if (allCompleted) {
setTodos(currentTodos => {
return currentTodos.map(todo => ({
...todo,
completed: false,
}));
});
} else {
setTodos(currentTodos => {
return currentTodos.map(todo => ({
...todo,
completed: true,
}));
});
}
};

Choose a reason for hiding this comment

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

It should also change the status of todos in the database, as when the user reloads the page todos have the same status as previously.

onDelete={handleDeleteTodo}
onEditedId={setEditedId}
isDeleteActive={isDeleteActive}
onDeleteActive={(value) => setIsDeleteActive(value)}

Choose a reason for hiding this comment

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

Suggested change
onDeleteActive={(value) => setIsDeleteActive(value)}
onDeleteActive={setIsDeleteActive}

onEditedId={() => setEditedId(null)}
onDelete={handleDeleteTodo}
isLoading={isLoading}
onLoad={(value) => setIsLoading(value)}

Choose a reason for hiding this comment

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

Suggested change
onLoad={(value) => setIsLoading(value)}
onLoad={setIsLoading}

isActive: boolean;
};

export const TodoTempItem: React.FC<Props> = ({ isActive }) => {

Choose a reason for hiding this comment

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

You can use your TodoItem component

Copy link
Author

Choose a reason for hiding this comment

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

My TodoItem has so many other functions that compared to TodoTempItem will be redundant

Choose a reason for hiding this comment

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

My TodoItem has so many other functions that compared to TodoTempItem will be redundant

But, the design is the same, so if something changes you have to change two components instead TodoItem.
You can create a UI component TodoItem without any logic and use it in both cases

Choose a reason for hiding this comment

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

Not fixed and without reply

Copy link
Author

Choose a reason for hiding this comment

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

TodoItem focuses on displaying individual todo with their specific details, while TodoTempItem is dedicated to indicating a loading state when data is still being processed. They serve different purposes and have different logic to fulfill those purposes.

Choose a reason for hiding this comment

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

TodoItem focuses on displaying individual todo with their specific details, while TodoTempItem is dedicated to indicating a loading state when data is still being processed. They serve different purposes and have different logic to fulfill those purposes.

It's the same component, they have only different logic so as I mentioned before you can create a representing component with optional props and use it in both cases.

Choose a reason for hiding this comment

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

A detailed explanation of why creating two components with the same UI but different logic can lead to several issues and is generally not considered a good practice. Here are some reasons why it's a bad idea:

  1. Code Duplication: By having two separate components with similar UI, you are duplicating code. This can lead to maintenance challenges because any updates or bug fixes to the UI will need to be applied to both components, increasing the chances of inconsistencies and errors.

  2. Inconsistent Behavior: When components have different logic, it can result in inconsistent behavior for seemingly similar UI elements. Users may not understand why similar-looking elements behave differently, which can lead to confusion and a poor user experience.

  3. Difficulty in Maintenance: As your application grows, maintaining multiple components with similar UI but different logic becomes increasingly complex. It can be challenging to keep track of changes and ensure that both components remain in sync with each other.

  4. Scalability Issues: If you decide to introduce additional variations of the same UI with different logic in the future, you will end up creating more and more similar components, making your codebase harder to manage and extending development time.

  5. Reduced Reusability: One of the fundamental principles of component-based UI development is reusability. When you have multiple components with distinct logic but the same UI, you lose the benefits of component reusability, which can save time and effort in the long run.

To address these issues, it's advisable to create a single representing component with props or configuration options that allow you to customize the behavior of the component. This approach promotes code reuse, maintainability, and consistency in your application.

In your case, you can create a single "TodoItem" component that accepts props or configuration options to handle both regular todos and temporary todos. This way, you can have a single, well-maintained component that can adapt to different scenarios, ensuring a cleaner and more maintainable codebase.

Comment on lines 25 to 30
const value = useMemo(() => ({
errorMessage,
setErrorMessage,
isErrorHidden,
setIsErrorHidden,
}), [errorMessage, isErrorHidden]);

Choose a reason for hiding this comment

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

useMemo is redundant, remove in all contexts

@voievodik
Copy link
Author

voievodik commented Sep 26, 2023

Good work 👍 But not all works as expected, left some suggestions, check them. And a lot of tests failed, fix your solution: image

Locally, for some reason, all the tests fail, but on GitHub, some tests work
image

Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

  1. Should be "1 item" without "s" at the end
image
  1. !IMPORTANT Status of Todo is not saved on API:

    I selected all todos

image
After reloading status of todos was not saved
image
  1. Not implemented:
image

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.

Better, but still some functionality has to be implemented:

  1. You don't save the updated status for todo, when I reload the page it's the same.
  2. When toggleAll it saves on the server, but you need to notify the user which todos are currently updating, so add a loader for them.

Recorded video:
https://www.loom.com/share/f8c1e8a49af34f3d89998a13bd9989f1?sid=a4d39569-dab5-41e1-acec-c0295a443b80

import { useTodo } from '../../context/TodoContext';
import { useError } from '../../context/ErrorContext';
import { updateTodo } from '../../api/todos';
// import { useTitle } from '../../context/TitleContext';

Choose a reason for hiding this comment

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

Remove comments

import { useError } from '../../context/ErrorContext';

export const TodoFooter = () => {
const { selectedFilter, setSelectedFilter } = useContext(FilterContext);

Choose a reason for hiding this comment

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

You created hooks for contexts, but not for this one) Let's create for the FilterContext too

};

export const TodoHeader: React.FC<Props> = ({ onHandleActive }) => {
const { setTodoTemp } = useContext(TodoTempContext);

Choose a reason for hiding this comment

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

And for this one too

Comment on lines 42 to 56
todos.forEach(todo => {
if (todo.completed) {
const updatedTodo = { ...todo, completed: false };

updatePromises.push(updateTodo(todo.id, updatedTodo));
}
});
} else {
todos.forEach(todo => {
if (!todo.completed) {
const updatedTodo = { ...todo, completed: true };

updatePromises.push(updateTodo(todo.id, updatedTodo));
}
});

Choose a reason for hiding this comment

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

It's better to use map instead of forEach and push promises

Comment on lines 83 to 88
setTodoTemp({
id: 0,
userId: USER_ID,
title: title.trim(),
completed: false,
});

Choose a reason for hiding this comment

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

Suggested change
setTodoTemp({
id: 0,
userId: USER_ID,
title: title.trim(),
completed: false,
});
const newTodo = {
userId: USER_ID,
title: title.trim(),
completed: false,
};
setTodoTemp({
...newTodo,
id: 0,
});

Create an object for a new todo as you do the same when passing to createTodo

Comment on lines 25 to 31
setTodos(prev => prev.map(currentTodo => {
if (currentTodo.id === todo.id) {
return { ...currentTodo, completed: event.target.checked };
}

return currentTodo;
}));

Choose a reason for hiding this comment

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

You don't update the todo on the server, when I reload page status doesn't change

isActive: boolean;
};

export const TodoTempItem: React.FC<Props> = ({ isActive }) => {

Choose a reason for hiding this comment

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

My TodoItem has so many other functions that compared to TodoTempItem will be redundant

But, the design is the same, so if something changes you have to change two components instead TodoItem.
You can create a UI component TodoItem without any logic and use it in both cases

Copy link

@Moroz-Dmytro Moroz-Dmytro left a comment

Choose a reason for hiding this comment

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

There is still bug 🐞 with saving todos statuses:
I have next todos selected:

image

I click toggle all:

image

But after reloading only next todos are selected:

image

Then when I start editing second selected todo, the todo status disappear:

image
  1. onBlur still send a request to update todo even if todo title was not changed(save on Enter click is fixed)

Comment on lines 89 to 90
'is-active': (isDeleteActive && id === deletedTodoId)
|| isToggleActive.includes(id) || isCompleteActive,

Choose a reason for hiding this comment

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

Better to create constant for this

Comment on lines 77 to 80
onClick={() => {
setDeletedTodoId(id);
onDelete(todo);
}}

Choose a reason for hiding this comment

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

Cerate function for this

isActive: boolean;
};

export const TodoTempItem: React.FC<Props> = ({ isActive }) => {

Choose a reason for hiding this comment

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

Not fixed and without reply

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 👍
I left a few suggestions and described why it's a bad idea to duplicate components.

Comment on lines 41 to 69
if (isAllCompleted) {
const allCompleted = todos
.filter(({ completed }) => completed)
.map(({ id }) => id);

onToggleActive(allCompleted);
} else {
const allActive = todos
.filter(({ completed }) => !completed)
.map(({ id }) => id);

onToggleActive(allActive);
}

const updatePromises: Promise<Todo>[] = [];

if (isAllCompleted) {
todos.map(todo => {
const updatedTodo = { ...todo, completed: false };

return updatePromises.push(updateTodo(todo.id, updatedTodo));
});
} else {
todos.map(todo => {
const updatedTodo = { ...todo, completed: true };

return updatePromises.push(updateTodo(todo.id, updatedTodo));
});
}

Choose a reason for hiding this comment

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

Suggested change
if (isAllCompleted) {
const allCompleted = todos
.filter(({ completed }) => completed)
.map(({ id }) => id);
onToggleActive(allCompleted);
} else {
const allActive = todos
.filter(({ completed }) => !completed)
.map(({ id }) => id);
onToggleActive(allActive);
}
const updatePromises: Promise<Todo>[] = [];
if (isAllCompleted) {
todos.map(todo => {
const updatedTodo = { ...todo, completed: false };
return updatePromises.push(updateTodo(todo.id, updatedTodo));
});
} else {
todos.map(todo => {
const updatedTodo = { ...todo, completed: true };
return updatePromises.push(updateTodo(todo.id, updatedTodo));
});
}
if (isAllCompleted) {
const allCompleted = todos
.filter(({ completed }) => completed)
.map(({ id }) => id);
onToggleActive(allCompleted);
} else {
const allActive = todos
.filter(({ completed }) => !completed)
.map(({ id }) => id);
onToggleActive(allActive);
}
const updatePromises: Promise<Todo>[] = [];
if (isAllCompleted) {
todos.map(todo => {
const updatedTodo = { ...todo, completed: false };
return updatePromises.push(updateTodo(todo.id, updatedTodo));
});
} else {
todos.map(todo => {
const updatedTodo = { ...todo, completed: true };
return updatePromises.push(updateTodo(todo.id, updatedTodo));
});
}

You have the same if/else blocks, also if all are completed you actually don't need to filter them.

You can do something like this:

const isAllCompleted = todos.every(({ completed }) => completed);
const todosForUpdate = isAllCompleted ? todos : todos.filter(({ completed }) => !completed);
const todosIds = todosForUpdate.map(({ id }) => id);
const updatePromises = todos.map(todo => (
  updateTodo(todo.id, { ...todo, completed: !isAllCompleted })
));

onToggleActive(todosIds);

.catch(() => {
setIsInputDisabled(false);
setErrorMessage('Unable to add a todo');
inputRef?.current?.focus();

Choose a reason for hiding this comment

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

You set focus in finally so in catch block it's redundant

isActive: boolean;
};

export const TodoTempItem: React.FC<Props> = ({ isActive }) => {

Choose a reason for hiding this comment

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

TodoItem focuses on displaying individual todo with their specific details, while TodoTempItem is dedicated to indicating a loading state when data is still being processed. They serve different purposes and have different logic to fulfill those purposes.

It's the same component, they have only different logic so as I mentioned before you can create a representing component with optional props and use it in both cases.

isActive: boolean;
};

export const TodoTempItem: React.FC<Props> = ({ isActive }) => {

Choose a reason for hiding this comment

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

A detailed explanation of why creating two components with the same UI but different logic can lead to several issues and is generally not considered a good practice. Here are some reasons why it's a bad idea:

  1. Code Duplication: By having two separate components with similar UI, you are duplicating code. This can lead to maintenance challenges because any updates or bug fixes to the UI will need to be applied to both components, increasing the chances of inconsistencies and errors.

  2. Inconsistent Behavior: When components have different logic, it can result in inconsistent behavior for seemingly similar UI elements. Users may not understand why similar-looking elements behave differently, which can lead to confusion and a poor user experience.

  3. Difficulty in Maintenance: As your application grows, maintaining multiple components with similar UI but different logic becomes increasingly complex. It can be challenging to keep track of changes and ensure that both components remain in sync with each other.

  4. Scalability Issues: If you decide to introduce additional variations of the same UI with different logic in the future, you will end up creating more and more similar components, making your codebase harder to manage and extending development time.

  5. Reduced Reusability: One of the fundamental principles of component-based UI development is reusability. When you have multiple components with distinct logic but the same UI, you lose the benefits of component reusability, which can save time and effort in the long run.

To address these issues, it's advisable to create a single representing component with props or configuration options that allow you to customize the behavior of the component. This approach promotes code reuse, maintainability, and consistency in your application.

In your case, you can create a single "TodoItem" component that accepts props or configuration options to handle both regular todos and temporary todos. This way, you can have a single, well-maintained component that can adapt to different scenarios, ensuring a cleaner and more maintainable codebase.

Copy link

@roman-mirzoian roman-mirzoian 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 👍

Comment on lines +38 to +40
{isActiveItemsLeft > 1
? `${isActiveItemsLeft} items left`
: `${isActiveItemsLeft} item left`}

Choose a reason for hiding this comment

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

You only have to check one word:

Suggested change
{isActiveItemsLeft > 1
? `${isActiveItemsLeft} items left`
: `${isActiveItemsLeft} item left`}
{`${isActiveItemsLeft} ${isActiveItemsLeft > 1 ? 'items' : 'item'} left`}

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.

5 participants