solution #2594

YuliiaShatkovska · 2024-10-11T10:31:21Z

hma-3

Good job!
Have just a few suggestions⬇️

hma-3 · 2024-10-11T11:18:35Z

src/App.tsx

+  useEffect(() => {
+    setIsLoading(true);


you don't have to set loader to true here, just change the initial value in the state

const [isLoading, setIsLoading] = useState(true);

hma-3 · 2024-10-11T11:30:47Z

src/components/TodoFilter/TodoFilter.tsx

+    <form className="field has-addons">
+      <p className="control">
+        <span className="select">
+          <select data-cy="statusSelect" onChange={handleSelectFilter}>


add value to select

hma-3 · 2024-10-11T11:34:32Z

src/components/TodoList/TodoList.tsx

-        </td>
-      </tr>
+        return (
+          <tr data-cy="todo" className="" key={id}>


row needs to have class 'has-background-info-light' when selected

hma-3 · 2024-10-11T11:36:10Z

src/components/TodoList/TodoList.tsx

+            {completed ? (
+              <td className="is-vcentered">
+                <span className="icon" data-cy="iconCompleted">
+                  <i className="fas fa-check" />
+                </span>
+              </td>
+            ) : (
+              <td className="is-vcentered" />
+            )}


move condition inside td and then you won't have to write it twice

hma-3 · 2024-10-11T11:39:24Z

src/components/TodoModal/TodoModal.tsx

+  useEffect(() => {
+    setIsLoading(true);


you don't have to set loader to true here, just change the initial value in the state
const [isLoading, setIsLoading] = useState(true);

same

hma-3 · 2024-10-11T11:44:31Z

src/components/TodoModal/TodoModal.tsx

+              {completed ? (
+                <strong className="has-text-success">Done</strong>
+              ) : (
+                <strong className="has-text-danger">Planned</strong>
+              )}


I have mixed feelings about this😅
it may be worth moving the condition inside strong to avoid duplication, but I think this is a moot point here

hma-3 · 2024-10-11T11:48:37Z

src/utils/getFilteredTodos.ts

+      default:
+        return todo;


filter callback should return boolean value. here it's still working, but it would be better to return just true

mateacademy-ai-mentor

Great job on the code! 👏 However, there are a few improvements that need to be made. Please address the issues in the getFilteredTodos function and consider the provided feedback for naming conventions and direct DOM manipulation. Keep up the good work! 💪

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

mateacademy-ai-mentor · 2024-10-11T11:52:11Z

src/App.tsx


 export const App: React.FC = () => {
+  const [todos, setTodos] = useState<Todo[]>([]);


It seems like you are initializing your state with props. This can be problematic because when the props change, your state won't be updated. You should consider using a different approach. Here are some resources that explain this in more detail: here, here and here

mateacademy-ai-mentor · 2024-10-11T11:52:11Z

src/App.tsx

+      .finally(() => setIsLoading(false));
+  }, []);
+
+  const visibleTodos = getFilteredTodos(todos, { query, selectedFilter });


You are generating a new array on each render by calling getFilteredTodos(todos, { query, selectedFilter }). This can lead to performance issues as a new array is generated on every render. Consider using useMemo to avoid unnecessary array generation.

mateacademy-ai-mentor · 2024-10-11T11:52:11Z

src/App.tsx

@@ -17,18 +36,32 @@
            <h1 className="title">Todos:</h1>

            <div className="block">
-              <TodoFilter />
+              <TodoFilter
+                setSelectedFilter={setSelectedFilter}


The setSelectedFilter and setQuery methods do not follow the naming conventions for event handler functions. According to the conventions, handler names should start with 'handle' and include the name of the event. For example, setSelectedFilter could be renamed to handleFilterChange. Here is a link to the article that explains this in detail: here

mateacademy-ai-mentor · 2024-10-11T11:52:12Z

src/components/TodoFilter/TodoFilter.tsx

+        <span className="select">
+          <select data-cy="statusSelect" onChange={handleSelectFilter}>
+            {filters.map(filter => (
+              <option value={filter.toLowerCase()} key={filter}>


You are generating keys for your list items on render. It's not the best approach because React uses keys to identify which items have changed, are added, or are removed and it may negatively impact performance. Keys should be stable, predictable, and unique for every set of elements. You can read more about it here.

mateacademy-ai-mentor · 2024-10-11T11:52:12Z

src/components/TodoFilter/TodoFilter.tsx

+  const handleSelectFilter = (event: React.ChangeEvent<HTMLSelectElement>) => {
+    setSelectedFilter(event.target.value as Filters);
+  };
+
+  const handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+    setQuery(event.target.value.trimStart());
+  };


Your method names handleSelectFilter and handleInputChange are not following the naming conventions for methods in React. It's a good practice to name event handlers with the prefix 'on' and the part that describes what the event is. For example, handleSelectFilter can be renamed to onFilterChange and handleInputChange can be renamed to onInputChange. You can read more about it here.

mateacademy-ai-mentor · 2024-10-11T11:52:13Z

src/components/TodoModal/TodoModal.tsx


-export const TodoModal: React.FC = () => {
  return (


You are interacting with the DOM directly by using class names. This is not recommended in React. Consider using a library like classnames to handle class names conditionally.

mateacademy-ai-mentor · 2024-10-11T11:52:13Z

src/components/TodoModal/TodoModal.tsx


              {' by '}

-              <a href="mailto:[email protected]">Leanne Graham</a>
+              <a href={`mailto:${user?.email}`}>{user?.name}</a>


You are creating a mailto link here. Consider checking if the user's email exists before creating the link to avoid errors.

mateacademy-ai-mentor · 2024-10-11T11:52:14Z

src/utils/getFilteredTodos.ts

+  todos: Todo[],
+  { query, selectedFilter }: Params,
+) => {
+  let filteredTodos = [...todos];


There's no need to create a copy of the todos array here, because the filter method does not mutate the original array but returns a new one.

mateacademy-ai-mentor · 2024-10-11T11:52:14Z

src/utils/getFilteredTodos.ts

+      default:
+        return todo;


In the default case of the switch statement, you're returning the entire todo object, which will always be truthy. This means that if the selected filter is not 'ACTIVE' or 'COMPLETED', all todos will be included in the filteredTodos array, regardless of their completed status. If this is the intended behavior, it would be clearer to return true in the default case to explicitly indicate that all todos should be included.

mateacademy-ai-mentor · 2024-10-11T11:52:14Z

src/utils/getFilteredTodos.ts

+  const normilizedQuery = query.toLowerCase().trim();
+
+  if (normilizedQuery) {
+    filteredTodos = filteredTodos.filter(({ title }) => {
+      const normilizedTitle = title.toLowerCase();
+
+      return normilizedTitle.includes(normilizedQuery);
+    });
+  }


The search query filtering is applied after the status filter. This means that if a user selects the 'ACTIVE' filter and then enters a search query, the search will only be applied to the active todos, not to all todos. If this is the intended behavior, it would be good to clarify it in the function's documentation. If not, you should apply the search query filter to the original todos array before applying the status filter.

mateacademy-ai-mentor

Great work so far! There are just a few minor issues that need to be addressed. Please correct the typo in the getFilteredTodos.ts file and consider the suggested improvements regarding naming conventions. Keep up the good work! 👍

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

mateacademy-ai-mentor · 2024-10-11T12:18:54Z

src/components/TodoFilter/TodoFilter.tsx

+            onChange={onFilterChange}
+          >
+            {filters.map(filter => (
+              <option value={filter.toLowerCase()} key={filter}>


Avoid generating keys on render. This can lead to unexpected behavior in your application. Consider using a unique and consistent property from the data you're iterating over, if available.

mateacademy-ai-mentor · 2024-10-11T12:18:54Z

src/components/TodoFilter/TodoFilter.tsx

+    handleFilterChange(event.target.value as Filters);
+  };
+
+  const inInputChange = (event: React.ChangeEvent<HTMLInputElement>) => {


The method name 'inInputChange' does not follow the recommended naming conventions. Consider renaming it to 'handleInputChange' or similar.

mateacademy-ai-mentor · 2024-10-11T12:18:54Z

src/components/TodoFilter/TodoFilter.tsx

+  query,
+  selectedFilter,
+}) => {
+  const onFilterChange = (event: React.ChangeEvent<HTMLSelectElement>) => {


The method name 'onFilterChange' does not follow the recommended naming conventions. Consider renaming it to 'handleFilterChange' or similar.

mateacademy-ai-mentor · 2024-10-11T12:18:55Z

src/components/TodoList/TodoList.tsx

+          <tr
+            data-cy="todo"
+            className={isSelected ? 'has-background-info-light' : ''}
+            key={id}


It's not recommended to generate keys on render. The key should remain consistent across renders for performance reasons. Please consider using another approach.

mateacademy-ai-mentor · 2024-10-11T12:18:55Z

src/components/TodoList/TodoList.tsx

+                data-cy="selectButton"
+                className="button"
+                type="button"
+                onClick={() => setSelectedTodo(todo)}


The naming convention for the method is not followed. Please consider following the recommended naming conventions for methods. For example, the method could be renamed to handleSelectTodo.

mateacademy-ai-mentor · 2024-10-11T12:18:55Z

src/components/TodoModal/TodoModal.tsx

+export const TodoModal: React.FC<Props> = ({
+  selectedTodo,
+  setSelectedTodo,
+}) => {


You are initializing your state with props, which is generally not recommended. This can lead to bugs and unpredictable behavior. Here are some resources explaining why this is a bad practice: here, here and here.

mateacademy-ai-mentor · 2024-10-11T12:18:55Z

src/components/TodoModal/TodoModal.tsx

+
+  const handleCloseModal = () => {
+    setSelectedTodo(null);
+  };


Your method name handleCloseModal does not follow the recommended naming conventions for event handlers in React. Consider renaming it to onCloseModal or handleModalClose. Here are some handy naming conventions for event handler functions/props in React.

mateacademy-ai-mentor · 2024-10-11T12:18:55Z

src/components/TodoModal/TodoModal.tsx


-export const TodoModal: React.FC = () => {
  return (


You are interacting with the DOM directly by using className to define classes. It is recommended to use the classnames library for defining classes conditionally in React.

mateacademy-ai-mentor

Great job on your code! There are a few minor issues that need attention, such as avoiding direct DOM manipulation, following naming conventions for event handlers, and considering potential issues with initializing state with props or empty values. Nonetheless, these are not critical and won't affect the functionality of your code. I believe you can handle these improvements. Keep up the good work! 🚀

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

mateacademy-ai-mentor · 2024-10-11T12:29:50Z

src/App.tsx


 export const App: React.FC = () => {
+  const [todos, setTodos] = useState<Todo[]>([]);


You are initializing your state with empty arrays and null values. This is not a problem per se, but it can potentially lead to unnecessary re-renders and bugs. It would be a good idea to consider the potential issues that might arise from this. Here are some resources that might help you understand this better: here, here and here.

mateacademy-ai-mentor · 2024-10-11T12:29:50Z

src/components/TodoList/TodoList.tsx

+          <tr
+            data-cy="todo"
+            className={isSelected ? 'has-background-info-light' : ''}
+            key={id}


You're generating a key based on the id property of the todo item. While this is fine if the id is unique and does not change, it can lead to issues if the id is not unique or changes over time. This is because React uses the key to identify elements and can have issues with re-rendering if the key changes. Consider using a more reliable way of generating keys. Read more about it here.

mateacademy-ai-mentor · 2024-10-11T12:29:50Z

src/components/TodoList/TodoList.tsx

+                data-cy="selectButton"
+                className="button"
+                type="button"
+                onClick={() => setSelectedTodo(todo)}


The function setSelectedTodo is directly modifying the state. This is a direct interaction with the DOM, which is something we want to avoid in React. Try to handle this kind of logic in the component's methods and use React's built-in methods to update the state. Also, consider following the naming conventions for event handlers as described here.

mateacademy-ai-mentor · 2024-10-11T12:29:50Z

src/components/TodoList/TodoList.tsx

+        return (
+          <tr
+            data-cy="todo"
+            className={isSelected ? 'has-background-info-light' : ''}


You're defining classes conditionally here. This is a good practice, but it can be done in a more readable and maintainable way using the classnames library. This library allows you to define classes in a declarative way and takes care of the conditional logic for you.

mateacademy-ai-mentor · 2024-10-11T12:29:50Z

src/components/TodoModal/TodoModal.tsx

+  setSelectedTodo,
+}) => {
+  const [isLoading, setIsLoading] = useState(false);
+  const [user, setUser] = useState<User | null>(null);


You're initializing state with props. It might lead to bugs because React state updates are asynchronous. It's recommended to avoid initializing state with props. Here are some resources that explain why it's a bad practice: here, here and here.

mateacademy-ai-mentor · 2024-10-11T12:29:51Z

src/components/TodoModal/TodoModal.tsx

+      .finally(() => setIsLoading(false));
+  }, [userId]);
+
+  const handleModalClose = () => {


According to the naming conventions for methods in React, handlers for events should be named starting with 'handle' and the name of the event. For example, in your case, a better name for the method would be 'handleCloseModal'. It's a good practice to follow these conventions to make the code more readable and easier to understand. You can read more about it here.

mateacademy-ai-mentor · 2024-10-11T12:29:51Z

src/components/TodoModal/TodoModal.tsx


-export const TodoModal: React.FC = () => {
  return (


You're interacting with the DOM directly by using the 'className' attribute. In React, it's recommended to avoid direct DOM manipulation as much as possible. Instead, you can use the 'classnames' library to define classes conditionally. You can find more information about it here.

solution

e89bcc3

hma-3 approved these changes Oct 11, 2024

View reviewed changes

mateacademy-ai-mentor suggested changes Oct 11, 2024

View reviewed changes

fixed

2fa2a7c

YuliiaShatkovska requested a review from mateacademy-ai-mentor October 11, 2024 12:18

mateacademy-ai-mentor suggested changes Oct 11, 2024

View reviewed changes

fixed

08d81c2

YuliiaShatkovska requested a review from mateacademy-ai-mentor October 11, 2024 12:29

mateacademy-ai-mentor approved these changes Oct 11, 2024

View reviewed changes


		export const App: React.FC = () => {
		const [todos, setTodos] = useState<Todo[]>([]);

solution #2594

Are you sure you want to change the base?

solution #2594

Conversation

YuliiaShatkovska commented Oct 11, 2024

hma-3 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

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

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

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

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

Choose a reason for hiding this comment