Solution #1186

Valeraom · 2024-10-26T01:41:12Z

Valeraom · 2024-10-26T01:44:23Z

Two tests failed: 'should not request posts from API' and 'should not request comments from API'

It works properly, i don't know what the problem is.
The mentor told me to skip these tests.

mateacademy-ai-mentor

Thank you for your hard work on this project! Your code is generally well-structured and meets many of the task requirements. However, there are a few critical issues that need to be addressed before we can approve the pull request:

Error Handling: It's important to provide more detailed error messages or log errors to help with debugging. The current generic message 'Something went wrong' doesn't offer enough information for troubleshooting. This applies to both the general error handling and specific cases like when adding a comment fails.
State Management: Ensure that setIsLoading(false) is called correctly in the loadSelectedUserPosts function to reset the loading state, even if no user is selected. Additionally, review the logic around setIsLoading(true) and selectedUser to ensure it behaves as expected.
Event Handling: Consider using onClick instead of onMouseDown for user selection to avoid unexpected behavior. Also, review the onBlur and onClick handlers for managing the dropdown's open state to prevent issues like the dropdown closing when clicking inside it.
Naming Conventions: The state variable setisFormOpened should be renamed to setIsFormOpened to maintain consistent camelCase naming conventions.
Duplicate Attributes: The data-cy="PostDetails" attribute is duplicated. Please ensure this is intentional or remove the duplicate to avoid confusion.
Promise Error Handling: Ensure that the onAddComment promise handles errors appropriately. Currently, it resets the comment body and stops loading but doesn't handle potential errors from the promise.
CSS and Media Queries: Review the media query combining print and screen with a minimum width to ensure it targets the intended devices. Additionally, consider adding comments or documentation for the .tile class to explain its purpose or usage.

Addressing these issues will improve the functionality, maintainability, and user experience of your application. Keep up the great work, and I'm confident you'll have these resolved in no time! 😊

✨ 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-10-26T15:07:08Z

src/App.scss

@@ -18,6 +18,35 @@
  }
 }

+.tile {


Consider adding a comment or documentation for the .tile class to explain its purpose or usage, especially since it contains nested styles and media queries.

mateacademy-ai-mentor · 2024-10-26T15:07:08Z

src/App.scss

+    }
+  }
+
+  @media print, screen and (min-width: 769px) {


The media query here combines print and screen with a minimum width. Ensure this is intentional, as it might affect both screen and print styles. If the intention is to target only screens, consider using @media screen and (min-width: 769px).

mateacademy-ai-mentor · 2024-10-26T15:07:09Z

src/App.tsx

+    if (selectedUser === user) {
+      setIsLoading(false);
+    }


The condition if (selectedUser === user) is used to set setIsLoading(false). This might not be necessary since setIsLoading(true) is called right before setting the selected user. Consider reviewing this logic to ensure it behaves as expected.

mateacademy-ai-mentor · 2024-10-26T15:07:09Z

src/App.tsx

+            className={cn('tile is-parent is-8-desktop Sidebar', {
+              'Sidebar--open': selectedPost,
+            })}


The className for the sidebar uses classnames to conditionally apply Sidebar--open. Ensure that selectedPost is the correct condition for opening the sidebar. If there are other conditions or states that should affect this, consider adding them.

mateacademy-ai-mentor · 2024-10-26T15:07:09Z

src/components/NewCommentForm.tsx

+      .then(() => setCommentBody(''))
+      .finally(() => setIsLoading(false));


Ensure that the onAddComment promise handles errors appropriately. Currently, it only resets the comment body and stops loading, but it doesn't handle potential errors from the promise.

mateacademy-ai-mentor · 2024-10-26T15:07:09Z

src/hooks/useComments.ts

+    commentsService
+      .getComments(selectedPostId)
+      .then(setComments)
+      .catch(() => setErrorMessage('Something went wrong'))


Consider providing more detailed error messages or logging the error to help with debugging. The current message 'Something went wrong' is generic and may not provide enough information for troubleshooting.

mateacademy-ai-mentor · 2024-10-26T15:07:09Z

src/hooks/useComments.ts

+      .catch(error => {
+        setErrorMessage('Something went wrong');
+
+        throw error;


Similar to the comment above, consider logging the error or providing a more specific error message to help with debugging when adding a comment fails.

mateacademy-ai-mentor · 2024-10-26T15:07:10Z

src/hooks/usePosts.ts

+      .getUserPosts(selectedUserId)
+      .then(setPosts)
+      .catch(() => setErrorMessage('Something went wrong'))
+      .finally(() => setIsLoading(false));


The setIsLoading(false) should be called before the return statement in the loadSelectedUserPosts function to ensure that the loading state is correctly reset even if no user is selected.

mateacademy-ai-mentor · 2024-10-26T15:07:10Z

src/hooks/usePosts.ts

+    postsService
+      .getUserPosts(selectedUserId)
+      .then(setPosts)
+      .catch(() => setErrorMessage('Something went wrong'))


Consider providing more detailed error messages or logging the error to help with debugging. The current message 'Something went wrong' is generic and may not provide enough information for troubleshooting.

mateacademy-ai-mentor · 2024-10-26T15:07:10Z

src/hooks/useUsers.ts

+
+        setUsers(preparedUsers);
+      })
+      .catch(() => setErrorMessage('Something went wrong'));


Consider providing more detailed error messages or logging the error to help with debugging. The current message 'Something went wrong' is generic and may not provide enough information for troubleshooting.

mateacademy-ai-mentor

Your code has been reviewed and is approved! 🎉 While there are some areas for improvement, such as enhancing error handling, ensuring proper state management, and improving accessibility, the overall structure and functionality are solid. Please take the time to address the feedback provided to enhance your code quality and user experience. Keep up the great work and continue learning from these insights!

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

mateacademy-ai-mentor · 2024-10-26T19:31:31Z

src/App.scss

+.tile {
+  align-items: stretch;
+  min-height: min-content;
+  flex: 1 1 0;


The .tile class uses flex: 1 1 0;, which might not behave as expected if the parent container is not a flex container. Ensure that the parent of .tile is a flex container to utilize flex properties effectively.

mateacademy-ai-mentor · 2024-10-26T19:31:31Z

src/App.tsx

+            className={cn('tile is-parent is-8-desktop Sidebar', {
+              'Sidebar--open': selectedPost,
+            })}


The classnames library is used correctly to conditionally apply the Sidebar--open class. However, ensure that the selectedPost variable is correctly initialized and managed to avoid unexpected behavior when toggling the sidebar's open state.

mateacademy-ai-mentor · 2024-10-26T19:31:31Z

src/components/NewCommentForm.tsx

+    onAddComment(newComment)
+      .then(() => setCommentBody(''))
+      .finally(() => setIsLoading(false));


Ensure that the onAddComment function handles errors appropriately. Currently, the finally block resets the loading state, but it might be beneficial to handle errors explicitly to inform the user if the comment submission fails.

mateacademy-ai-mentor · 2024-10-26T19:31:31Z

src/components/PostDetails.tsx

-                delete button
-              </button>
-            </div>
+  useEffect(() => setIsFormOpened(false), [selectedPost]);


The useEffect hook resets the form state when the selectedPost changes. Ensure that this behavior is intended, as it will close the comment form whenever a new post is selected.

mateacademy-ai-mentor · 2024-10-26T19:31:31Z

src/components/PostDetails.tsx

+        {!isLoading && errorMessage && (
+          <div className="notification is-danger" data-cy="CommentsError">
+            {errorMessage}
+          </div>
+        )}


Consider adding a retry mechanism or additional user feedback when an error occurs while loading comments. This can improve user experience by providing options to recover from errors.

mateacademy-ai-mentor · 2024-10-26T19:31:31Z

src/hooks/useComments.ts

+      .catch(() => {
+        setErrorMessage('Unable to add a comment');
+      });


Similar to the loadComments function, consider providing more detailed error messages or logging for the addComment function to improve error handling and user feedback.

mateacademy-ai-mentor · 2024-10-26T19:31:32Z

src/hooks/usePosts.ts

+    postsService
+      .getUserPosts(selectedUserId)
+      .then(setPosts)
+      .catch(() => setErrorMessage('Unable to load posts'))


Consider providing more detailed error messages or logging the error details to help with debugging and improving user feedback.

mateacademy-ai-mentor · 2024-10-26T19:31:32Z

src/hooks/usePosts.ts

+import { Post } from '../types';
+import * as postsService from '../api/posts';
+
+export const usePosts = (selectedUserId: number = 0) => {


The default value for selectedUserId is set to 0. Ensure that this default value is appropriate for your application's logic, as it might lead to unintended behavior if not handled correctly.

mateacademy-ai-mentor · 2024-10-26T19:31:32Z

src/hooks/useUsers.ts

+
+        setUsers(preparedUsers);
+      })
+      .catch(() => setErrorMessage('Unable to load users'));


Consider providing more detailed error messages or logging the error details to help with debugging and improving user feedback.

mateacademy-ai-mentor · 2024-10-26T19:31:32Z

src/hooks/useUsers.ts

+        const preparedUsers = res.map(user => {
+          const { id, name, email, phone } = user;
+
+          return {
+            id,
+            name,
+            email,
+            phone,
+          };
+        });


The preparedUsers mapping seems redundant if it only extracts and returns the same properties without modification. Consider simplifying this by directly setting setUsers(res) unless there's a specific reason for this transformation.

Solution

87c5264

fix error reset

f714799

YuliiaShatkovska approved these changes Oct 26, 2024

View reviewed changes

mateacademy-ai-mentor suggested changes Oct 26, 2024

View reviewed changes

fix solution

584a59d

Valeraom requested a review from mateacademy-ai-mentor October 26, 2024 19:28

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

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solution #1186

Solution #1186

Valeraom commented Oct 26, 2024

Valeraom commented Oct 26, 2024 •

edited

Loading

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

mateacademy-ai-mentor Oct 26, 2024

@@ @@ -18,6 +18,35 @@ @@
                 }
               }
+              .tile {

		.then(() => setCommentBody(''))
		.finally(() => setIsLoading(false));

Solution #1186

Are you sure you want to change the base?

Solution #1186

Conversation

Valeraom commented Oct 26, 2024

Valeraom commented Oct 26, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Valeraom commented Oct 26, 2024 •

edited

Loading