add task solution #2700

Andrew77788 · 2024-12-18T13:17:16Z

mateacademy-ai-mentor

Thank you for your hard work on this project! 😊 While your implementation is mostly correct and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:

Dynamic Email Address: The email address in the anchor tag is currently hardcoded. It's important to dynamically set this based on the user object to ensure it reflects the correct user's email.
Loading State Logic: The logic for setting the loading state is inefficient. It sets the loading state to true and then immediately to false after a timeout. This should be revised to better reflect the actual loading state of the application.
Typo in Variable Name: There's a typo in the variable name updatedToods. It should be corrected to updatedTodos to maintain consistency and clarity in your code.

Please address these issues to ensure the functionality and clarity of your code. Once these changes are made, we can proceed with the approval. Keep up the great work, and don't hesitate to reach out if you need any assistance! 🚀

✨ 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-18T13:18:41Z

src/App.tsx

+
+  useEffect(() => {
+    setLoading(true);
+    let updatedToods = [...todos];


There is a typo in the variable name updatedToods. It should be updatedTodos to match the context of the code.

mateacademy-ai-mentor · 2024-12-18T13:18:41Z

src/App.tsx

+    setLoading(true);
+    let updatedToods = [...todos];
+
+    if (filter === 'active') {
+      updatedToods = updatedToods.filter(event => !event.completed);
+    } else if (filter === 'completed') {
+      updatedToods = updatedToods.filter(event => event.completed);
+    }
+
+    if (searchFilter) {
+      updatedToods = updatedToods.filter(todo =>
+        todo.title.toLowerCase().includes(searchFilter.toLowerCase()),
+      );
+    }
+
+    setTimeout(() => {
+      setFilterTodos(updatedToods);
+      setLoading(false);


The logic for setting the loading state is inefficient. The loading state is set to true and then immediately set to false after a timeout. Consider revising this logic to better reflect the actual loading state of the application.

mateacademy-ai-mentor · 2024-12-18T13:18:41Z

src/components/TodoModal/TodoModal.tsx


              {' by '}

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


The email address in the anchor tag is hardcoded. Consider dynamically setting the email address based on the user object to ensure it reflects the correct user's email.

mateacademy-ai-mentor

Great job on implementing the solution! 🎉 While there are a few critical issues to address, such as dynamically setting the email address based on the user object, correcting the typo in updatedTodos, and revising the loading state logic for efficiency, your code meets the task requirements. These improvements will enhance the functionality and clarity of your code. Remember to review the comments and learn from these points to refine your skills further. Keep up the good work and continue to improve! 💪

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

mateacademy-ai-mentor · 2024-12-18T13:31:00Z

src/App.tsx

+  useEffect(() => {
+    if (selectedTodo?.userId) {
+      setLoadingModal(true);
+      getUser(selectedTodo?.userId).then(fetchedUser => setUser(fetchedUser));


The email address in the anchor tag should be dynamically set based on the user object to ensure it reflects the correct user's email. Please update this logic accordingly.

mateacademy-ai-mentor · 2024-12-18T13:31:00Z

src/App.tsx

+      setTimeout(() => {
+        setLoadingModal(false);
+      }, 500);


The logic for setting the loading state is inefficient. It sets the loading state to true and then immediately to false after a timeout. This should be revised to better reflect the actual loading state of the application.

mateacademy-ai-mentor · 2024-12-18T13:31:00Z

src/App.tsx

+
+  useEffect(() => {
+    setLoading(true);
+    let updatedTodos = [...todos];


There's a typo in the variable name updatedToods. It should be corrected to updatedTodos to maintain consistency and clarity in your code.

mateacademy-ai-mentor · 2024-12-18T13:31:00Z

src/components/TodoModal/TodoModal.tsx


              {' by '}

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


Ensure that the email address is dynamically set based on the user object. This is important to reflect the correct user's email in the anchor tag.

volodymyr-soltys97

Good job 👍
Let's improve your code
You need to fix these failed tests, you need to add additional class for eye icon and data attribute, if you need some help feel free in the fe_chat

volodymyr-soltys97 · 2024-12-19T08:08:53Z

src/App.tsx

+    if (filter === 'active') {
+      updatedTodos = updatedTodos.filter(event => !event.completed);
+    } else if (filter === 'completed') {


Create a enum for 'all', 'active', 'completed' and use it everywhere

volodymyr-soltys97 · 2024-12-19T08:09:11Z

src/components/TodoItem/TodoItem.tsx

+      </td>
+      <td className="is-vcentered is-expanded">
+        <p className={todo.completed ? 'has-text-success' : 'has-text-danger'}>
+          {todo.title}{' '}


Suggested change

{todo.title}{' '}

{todo.title}

volodymyr-soltys97 · 2024-12-19T08:09:31Z

src/components/TodoItem/TodoItem.tsx

+        )}
+      </td>
+      <td className="is-vcentered is-expanded">
+        <p className={todo.completed ? 'has-text-success' : 'has-text-danger'}>


Use the classnames library for add classes with condition, fix it everywhere

volodymyr-soltys97

Good job 👍
Let's improve your code

volodymyr-soltys97 · 2024-12-24T10:21:25Z

src/App.tsx


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


Don't need to create additional state for filtered todos, use only one state

Suggested change

const [filterTodos, setFilterTodos] = useState<Todo[]>([]);

volodymyr-soltys97 · 2024-12-24T10:21:36Z

src/App.tsx


 export const App: React.FC = () => {
+  const [todos, setTodo] = useState<Todo[]>([]);
+  const [filterTodos, setFilterTodos] = useState<Todo[]>([]);
+  const [filter, setFilter] = useState<string>('all');


Use enum here for 'all'

volodymyr-soltys97 · 2024-12-24T10:21:58Z

src/App.tsx

+    if (filter === FilterType.Active) {
+      updatedTodos = updatedTodos.filter(event => !event.completed);
+    } else if (filter === FilterType.Completed) {
+      updatedTodos = updatedTodos.filter(event => event.completed);
+    }


Use switch case here, it will be better

volodymyr-soltys97 · 2024-12-24T10:22:08Z

src/App.tsx

+    let updatedTodos = [...todos];
+
+    if (filter === FilterType.Active) {
+      updatedTodos = updatedTodos.filter(event => !event.completed);


Suggested change

updatedTodos = updatedTodos.filter(event => !event.completed);

updatedTodos = updatedTodos.filter(todo => !todo.completed);

volodymyr-soltys97 · 2024-12-24T10:22:24Z

src/components/TodoFilter/TodoFilter.tsx

+            <option value="all">All</option>
+            <option value="active">Active</option>
+            <option value="completed">Completed</option>


Also, use a created enum here

volodymyr-soltys97 · 2024-12-24T10:22:45Z

src/components/TodoItem/TodoItem.tsx

+            'has-text-danger': !todo.completed,
+          })}
+        >
+          {todo.title}


Use destructuting for todo

VitaliyBondarenko1982

Good job.

add task solution

f97ed3a

mateacademy-ai-mentor suggested changes Dec 18, 2024

View reviewed changes

add task solution

d3c8128

Andrew77788 requested a review from mateacademy-ai-mentor December 18, 2024 13:29

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

View reviewed changes

volodymyr-soltys97 suggested changes Dec 19, 2024

View reviewed changes

add task solution

8f74136

Andrew77788 requested review from volodymyr-soltys97 and mateacademy-ai-mentor December 23, 2024 16:23

add task solution

fe490d7

volodymyr-soltys97 suggested changes Dec 24, 2024

View reviewed changes

add task solution

6b2361c

Andrew77788 requested a review from volodymyr-soltys97 December 24, 2024 11:08

VitaliyBondarenko1982 approved these changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add task solution #2700

add task solution #2700

Andrew77788 commented Dec 18, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 18, 2024

mateacademy-ai-mentor Dec 18, 2024

mateacademy-ai-mentor Dec 18, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 18, 2024

mateacademy-ai-mentor Dec 18, 2024

mateacademy-ai-mentor Dec 18, 2024

mateacademy-ai-mentor Dec 18, 2024

volodymyr-soltys97 left a comment

volodymyr-soltys97 Dec 19, 2024

volodymyr-soltys97 Dec 19, 2024

volodymyr-soltys97 Dec 19, 2024

volodymyr-soltys97 left a comment

volodymyr-soltys97 Dec 24, 2024

volodymyr-soltys97 Dec 24, 2024

volodymyr-soltys97 Dec 24, 2024

volodymyr-soltys97 Dec 24, 2024

volodymyr-soltys97 Dec 24, 2024

volodymyr-soltys97 Dec 24, 2024

VitaliyBondarenko1982 left a comment

	updatedTodos = updatedTodos.filter(event => !event.completed);
	updatedTodos = updatedTodos.filter(todo => !todo.completed);

add task solution #2700

Are you sure you want to change the base?

add task solution #2700

Conversation

Andrew77788 commented Dec 18, 2024

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

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

volodymyr-soltys97 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

volodymyr-soltys97 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

VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment