Solution #2682

save-v · 2024-12-13T19:15:12Z

https://save-v.github.io/react_dynamic-list-of-todos/

minimal005 · 2024-12-14T19:13:31Z

src/App.tsx

+  const [todos, setTodos] = useState<Todo[]>([]);
+  const [isLoading, setIsLoading] = useState<boolean>(false);
+  const [query, setQuery] = useState<string>('');
+  const [selectedTodoId, setSelectedTodoId] = useState<number | null>(null);


Якщо ми вказуємо конкретне значення в useState, то можно явно не прописувати його тип, він визначиться автоматично, наприклад,

const [query, setQuery] = useState('');

Він сам визначає, що це буде рядок.
Але якщо ми в useState прописуємо null, то тоді дійсно треба визначити, який тип там має бути

Хороша робота, вітаю тебе ))

Якщо ми вказуємо конкретне значення в useState, то можно явно не прописувати його тип, він визначиться автоматично, наприклад,

const [query, setQuery] = useState('');

Він сам визначає, що це буде рядок. Але якщо ми в useState прописуємо null, то тоді дійсно треба визначити, який тип там має бути

Good point

mateacademy-ai-mentor

Thank you for your hard work on this project! There are a few critical issues that need to be addressed before we can approve the pull request. The most significant issue is the consistent typo in the СompletedStatus and SetStateСompletedStatus imports, where a Cyrillic 'С' is used instead of the Latin 'C'. This could lead to runtime errors and should be corrected for proper functionality. Additionally, ensure that the value from event.target.value is correctly cast to the CompletedStatus type to prevent potential runtime errors. Lastly, consider changing the let flag = null; to const for better code clarity. Once these issues are resolved, your code will be in great shape! Keep up the great work! 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2024-12-14T19:16:26Z

src/App.tsx

@@ -8,7 +8,39 @@ import { TodoFilter } from './components/TodoFilter';
 import { TodoModal } from './components/TodoModal';
 import { Loader } from './components/Loader';

+import { Todo } from './types/Todo';
+import { getTodos } from './api';
+import { СompletedStatus } from './types/СompletedStatus';


There seems to be a typo in the import statement for СompletedStatus. Ensure that the file name and path are correct. The character 'С' might be a Cyrillic letter instead of the Latin 'C'.

mateacademy-ai-mentor · 2024-12-14T19:16:26Z

src/App.tsx

+    СompletedStatus.all,
+  );
+
+  let flag = null;


The variable flag is declared with let but is not reassigned. Consider using const instead for better code clarity and to prevent accidental reassignment.

mateacademy-ai-mentor · 2024-12-14T19:16:26Z

src/components/TodoFilter/TodoFilter.tsx

-      <span className="icon is-left">
-        <i className="fas fa-magnifying-glass" />
-      </span>
+import { SetStateСompletedStatus } from '../../types/SetStateСompletedStatus';


There seems to be a typo in the import statement for SetStateСompletedStatus. Ensure that the file name and path are correct. The character 'С' might be a Cyrillic letter instead of the Latin 'C'.

mateacademy-ai-mentor · 2024-12-14T19:16:26Z

src/components/TodoFilter/TodoFilter.tsx

-        <i className="fas fa-magnifying-glass" />
-      </span>
+import { SetStateСompletedStatus } from '../../types/SetStateСompletedStatus';
+import { СompletedStatus } from '../../types/СompletedStatus';


There seems to be a typo in the import statement for СompletedStatus. Ensure that the file name and path are correct. The character 'С' might be a Cyrillic letter instead of the Latin 'C'.

mateacademy-ai-mentor · 2024-12-14T19:16:26Z

src/components/TodoFilter/TodoFilter.tsx

+        <span className="select">
+          <select
+            onChange={event =>
+              setFilterStatus(event.target.value as СompletedStatus)


Ensure that the value from event.target.value is correctly cast to the СompletedStatus type. This is important to prevent runtime errors if the value does not match the expected enum type.

mateacademy-ai-mentor · 2024-12-14T19:16:26Z

src/components/TodoModal/TodoModal.tsx

+  }
+
+  useEffect(() => {
+    getUser(userId).then(respons => {


There is a typo in the variable name respons. It should be response to correctly reflect the data being received from the getUser function.

mateacademy-ai-mentor

Review Summary

Great job on addressing the previous issues! Your code is mostly well-structured and follows React best practices. Here are some important points to consider:

Typographical Corrections: Ensure that the imports for CompletedStatus and SetStateCompletedStatus use the correct Latin 'C' instead of a Cyrillic 'С'. This is crucial to prevent runtime errors.
Type Casting: Make sure that the value from event.target.value is correctly cast to the CompletedStatus type to avoid potential runtime errors.
Destructuring Safety: In the TodoModal component, ensure that the todo object is always found before destructuring. Consider adding a check to handle cases where todo might be undefined.
Code Clarity: Consider changing let flag = null; to const flag = null; for better code clarity and to prevent unintended reassignment.

You've made significant progress, and your solution meets the task requirements. Keep up the good work and continue to refine your skills by addressing these issues in future projects. This decision is made to help you move forward with your learning journey. 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2024-12-14T21:47:38Z

src/App.tsx

+    CompletedStatus.all,
+  );
+
+  let flag = null;


Consider changing let flag = null; to const flag = null; for better code clarity and to prevent unintended reassignment.

mateacademy-ai-mentor · 2024-12-14T21:47:38Z

src/components/TodoFilter/TodoFilter.tsx

-      <span className="icon is-left">
-        <i className="fas fa-magnifying-glass" />
-      </span>
+import { SetStateCompletedStatus } from '../../types/SetStateCompletedStatus';


Ensure that the import SetStateCompletedStatus uses the correct Latin 'C' instead of a Cyrillic 'С'. This could lead to runtime errors if not corrected.

mateacademy-ai-mentor · 2024-12-14T21:47:38Z

src/components/TodoFilter/TodoFilter.tsx

+            onChange={event =>
+              setFilterStatus(event.target.value as CompletedStatus)


Ensure that the value from event.target.value is correctly cast to the CompletedStatus type. This is crucial to prevent potential runtime errors.

mateacademy-ai-mentor · 2024-12-14T21:47:38Z

src/components/TodoModal/TodoModal.tsx

+  const [isLoading, setIsLoading] = useState<boolean>(true);
+  const [user, setUser] = useState<User | null>(null);
+  const todo = todos.find(innertodo => innertodo.id === selectedTodoId);
+  const { id, title, userId, completed } = todo as Todo;


Ensure that the todo object is always found before destructuring. Consider adding a check to handle cases where todo might be undefined to prevent potential runtime errors.

mateacademy-ai-mentor · 2024-12-14T21:47:38Z

src/types/SetStateCompletedStatus.ts

@@ -0,0 +1,4 @@
+import { CompletedStatus } from './CompletedStatus';


Ensure that the import CompletedStatus uses the correct Latin 'C' instead of a Cyrillic 'С'. This could lead to runtime errors if not corrected.

BudnikOleksii

Good work 👍
Let's improve your solution slightly

BudnikOleksii · 2024-12-17T18:48:29Z

src/App.tsx

+  const [todos, setTodos] = useState<Todo[]>([]);
+  const [isLoading, setIsLoading] = useState<boolean>(false);
+  const [query, setQuery] = useState<string>('');
+  const [selectedTodoId, setSelectedTodoId] = useState<number | null>(null);


Якщо ми вказуємо конкретне значення в useState, то можно явно не прописувати його тип, він визначиться автоматично, наприклад,

const [query, setQuery] = useState('');

Він сам визначає, що це буде рядок. Але якщо ми в useState прописуємо null, то тоді дійсно треба визначити, який тип там має бути

Good point

BudnikOleksii · 2024-12-17T18:51:08Z

src/App.tsx

+    CompletedStatus.all,
+  );
+
+  let flag = null;


What flag is it? I hope it's Ukrainian 🎏

BudnikOleksii · 2024-12-17T18:53:52Z

src/App.tsx

+  if (filterStatus !== CompletedStatus.all) {
+    flag = filterStatus === CompletedStatus.completed ? true : false;
+  }


This logic isn't obvious. It's better to create a function that incapsulates filter logic so you don't need to think about it in the component

BudnikOleksii · 2024-12-17T18:55:19Z

src/App.tsx

+    setIsLoading(true);
+    getTodos().then(response => {
+      setTodos(response);
+      setIsLoading(false);


You should remove the loading state in case of an error as well. In your case if an error occurs a user will see an infinite loader

BudnikOleksii · 2024-12-17T18:57:01Z

src/components/TodoFilter/TodoFilter.tsx

+        </span>
+
+        {query && (
+          <span className="icon is-right " style={{ pointerEvents: 'all' }}>


Avoid inline styles, use classes instead

BudnikOleksii · 2024-12-17T18:59:16Z

src/components/TodoList/TodoList.tsx

+          return (
+            <tr
+              key={id}
+              data-cy="todo"


Create a todo component

BudnikOleksii · 2024-12-17T19:00:06Z

src/components/TodoList/TodoList.tsx

+                    <i
+                      className={cN(
+                        'far',
+                        selectedTodoId !== id ? 'fa-eye' : 'fa-eye-slash',


You compare selectedTodoId !== id twice, create a variable

BudnikOleksii · 2024-12-17T19:00:49Z

src/components/TodoModal/TodoModal.tsx

+  todos: Todo[];
+  selectedTodoId: number;


Just pass a selectedTodo instead

BudnikOleksii · 2024-12-17T19:01:05Z

src/components/TodoModal/TodoModal.tsx

+  const [isLoading, setIsLoading] = useState<boolean>(true);
+  const [user, setUser] = useState<User | null>(null);
+  const todo = todos.find(innertodo => innertodo.id === selectedTodoId);
+  const { id, title, userId, completed } = todo as Todo;


Avoid using as

BudnikOleksii · 2024-12-17T19:02:26Z

src/types/SetStateCompletedStatus.ts

+import { CompletedStatus } from './CompletedStatus';
+import { Dispatch, SetStateAction } from 'react';
+
+export type SetStateCompletedStatus = Dispatch<SetStateAction<CompletedStatus>>;


No need to create a type for this if you use it only once

Solution

0f58a59

minimal005 approved these changes Dec 14, 2024

View reviewed changes

mateacademy-ai-mentor suggested changes Dec 14, 2024

View reviewed changes

fix typo

970635e

save-v requested a review from mateacademy-ai-mentor December 14, 2024 21:46

mateacademy-ai-mentor approved these changes Dec 14, 2024

View reviewed changes

BudnikOleksii suggested changes Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solution #2682

Solution #2682

save-v commented Dec 13, 2024

minimal005 Dec 14, 2024

minimal005 Dec 14, 2024

BudnikOleksii Dec 17, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

mateacademy-ai-mentor Dec 14, 2024

BudnikOleksii left a comment

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

BudnikOleksii Dec 17, 2024

		onChange={event =>
		setFilterStatus(event.target.value as CompletedStatus)

		@@ -0,0 +1,4 @@
		import { CompletedStatus } from './CompletedStatus';

Solution #2682

Are you sure you want to change the base?

Solution #2682

Conversation

save-v commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudnikOleksii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment