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

add task #1177

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,5 @@ Install Prettier Extention and use this [VSCode settings](https://mate-academy.g
1. Implement comment deletion
- Delete the commnet immediately not waiting for the server response to improve the UX.
1. (*) Handle `Add` and `Delete` errors so the user can retry

[DEMO LINK](https://YuliiaKosenchuk.github.io/react_dynamic-list-of-posts/)
9 changes: 9 additions & 0 deletions src/App.scss
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,12 @@
.message-body {
white-space: pre-line;
}

.tile.is-ancestor {
display: flex;
}

.tile.is-parent {
flex: 1;
padding: 0.75rem;
}
202 changes: 165 additions & 37 deletions src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* eslint-disable @typescript-eslint/indent */

import classNames from 'classnames';

import 'bulma/css/bulma.css';
Expand All @@ -8,53 +10,179 @@ import { PostsList } from './components/PostsList';
import { PostDetails } from './components/PostDetails';
import { UserSelector } from './components/UserSelector';
import { Loader } from './components/Loader';
import { useEffect, useState } from 'react';
import { User } from './types/User';
import {
addNewComment,
deleteComment,
getUserPostComments,
getUserPosts,
getUsers,
} from './api/post';
import { Post } from './types/Post';
import { Comment, CommentData } from './types/Comment';

export const App = () => (
<main className="section">
<div className="container">
<div className="tile is-ancestor">
<div className="tile is-parent">
<div className="tile is-child box is-success">
<div className="block">
<UserSelector />
</div>
export const App = () => {
const [users, setUsers] = useState<User[]>([]);

Choose a reason for hiding this comment

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

You are initializing your state with props. This can lead to bugs and make the component harder to think about. You should avoid copying props into state. Instead, try to make the component fully controlled or fully uncontrolled with a key. You can read more about it here: Stack Overflow: Why is it bad to initialize state with props in React?

const [userSelected, setUserSelected] = useState<User | null>(null);

Choose a reason for hiding this comment

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

You are initializing your state with props. This can lead to bugs and make the component harder to think about. You should avoid copying props into state. Instead, try to make the component fully controlled or fully uncontrolled with a key. You can read more about it here: Stack Overflow: Why is it bad to initialize state with props in React?

const [showPostLoader, setShowPostLoader] = useState<boolean>(false);
const [showCommentLoader, setShowCommentLoader] = useState<boolean>(false);
const [isAddingComment, setIsAddingComment] = useState(false);
const [userPosts, setUserPosts] = useState<Post[]>([]);
const [selectedPost, setSelectedPost] = useState<Post | null>(null);
const [isSidebarVisible, setIsSidebarVisible] = useState<number | null>(null);
const [postComments, setPostComments] = useState<Comment[]>([]);
const [hasPostError, setHasPostError] = useState<boolean>(false);

Choose a reason for hiding this comment

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

The variable hasPostError is not very clear. It would be more understandable if it was named hasPostLoadError or postLoadErrorOccurred.

const [hasCommentError, setHasCommentError] = useState<boolean>(false);

Choose a reason for hiding this comment

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

The variable hasCommentError is not very clear. It would be more understandable if it was named hasCommentLoadError or commentLoadErrorOccurred.

const [isCommentFormVisible, setIsCommentFormVisible] = useState(false);

<div className="block" data-cy="MainContent">
<p data-cy="NoSelectedUser">No user selected</p>
useEffect(() => {

Choose a reason for hiding this comment

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

You should handle errors that might occur during API calls. Wrap the getUsers() call in a try-catch block and set an appropriate state in case of an error.

getUsers()
.then(res => {
setUsers(res as User[]);
})
.catch(() => new Error('Failed to fetch users'));
}, []);

Choose a reason for hiding this comment

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

In your useEffect hook for fetching users, you are catching any errors that might occur but you're not handling them. This might lead to a situation where an error occurs but the user has no idea about it. It's a good practice to provide some sort of feedback to the user when an error occurs. You could, for example, show a notification or an error message in the UI. Also, you are not storing the error message anywhere. It's a good practice to store the error message in the state and display it to the user. You can use useState to store the error message.

Choose a reason for hiding this comment

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

It's a good practice to handle errors properly. In your case, if the request fails, you just throw a new Error, but nothing catches it. It's better to show some notification to the user about the problem. You can use a try-catch block for error handling.


<Loader />
const fetchUserPosts = (user: User) => {
setShowPostLoader(true);
getUserPosts(user.id)
.then(res => {
setUserPosts(res as Post[]);
setHasPostError(false);
})
.catch(() => {
setHasPostError(true);
})
.finally(() => {
setShowPostLoader(false);
});

Choose a reason for hiding this comment

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

Similar to the previous comment, you're catching errors here but not providing any feedback to the user. You should handle these errors in a way that the user is informed about it. You can do this by setting an error state and displaying a message in the UI when an error occurs.

Choose a reason for hiding this comment

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

You are using the setShowPostLoader method twice in this function. To make your code clearer, it's better to avoid using setState several times in one function call. Consider creating a separate function to handle the loading state.

};

Choose a reason for hiding this comment

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

The function handleUserPosts is interacting with the DOM directly by setting the loading state. This is not recommended in React. Instead, try to handle loading states using React's state and render methods. Also, make sure to handle errors during API calls.


<div
className="notification is-danger"
data-cy="PostsLoadingError"
>
Something went wrong!
</div>
const fetchPostComments = (currentPost: Post) => {
setIsSidebarVisible(currentPost.id);
setShowCommentLoader(true);

setSelectedPost(currentPost);

getUserPostComments(currentPost.id)
.then(res => {
setPostComments(res as Comment[]);
setHasCommentError(false);
})
.catch(() => {
setHasCommentError(true);
})
.finally(() => {
setShowCommentLoader(false);
});

Choose a reason for hiding this comment

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

Same issue here with error handling. You're catching the error but not doing anything with it. Make sure to handle these errors properly so the user is informed about it.

Choose a reason for hiding this comment

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

You are using the setShowCommentLoader method twice in this function. To make your code clearer, it's better to avoid using setState several times in one function call. Consider creating a separate function to handle the loading state.

};

Choose a reason for hiding this comment

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

The function handleShowComment is interacting with the DOM directly by setting the loading state. This is not recommended in React. Instead, try to handle loading states using React's state and render methods. Also, make sure to handle errors during API calls.


const removeComment = (commentId: number) => {
deleteComment(commentId)
.then(() => {
setPostComments(prevPostComments =>
prevPostComments.filter(
(comment: Comment) => comment.id !== commentId,
),
);
})
.catch(() => new Error('Failed to delete comment'));

Choose a reason for hiding this comment

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

Again, you're catching the error but not handling it. Make sure to provide some feedback to the user when an error occurs.

};

Choose a reason for hiding this comment

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

In the handleDeleteComment function, you are not handling potential errors that might occur during the deleteComment API call. Always handle errors during API calls.

Choose a reason for hiding this comment

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

You should handle errors properly. In your case, if the request fails, you just throw a new Error, but nothing catches it. It's better to show some notification to the user about the problem. You can use a try-catch block for error handling.


const createComment = (newComment: CommentData) => {
setIsAddingComment(true);
addNewComment(newComment)
.then(res => {
setPostComments([...postComments, res as Comment]);
setIsAddingComment(false);
})
.catch(() => new Error('Failed to add comment'));

Choose a reason for hiding this comment

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

You're catching the error but not handling it. Consider providing some feedback to the user when an error occurs.

};

Choose a reason for hiding this comment

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

In the addComment function, you are not handling potential errors that might occur during the addNewComment API call. Always handle errors during API calls.

Choose a reason for hiding this comment

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

You should handle errors properly. In your case, if the request fails, you just throw a new Error, but nothing catches it. It's better to show some notification to the user about the problem. You can use a try-catch block for error handling.


<div className="notification is-warning" data-cy="NoPostsYet">
No posts yet
return (
<main className="section">
<div className="container">
<div className="tile is-ancestor">
<div className="tile is-parent">
<div className="tile is-child box is-success">
<div className="block">
<UserSelector
users={users}
setUserSelected={setUserSelected}
userSelected={userSelected}
handleUserPosts={fetchUserPosts}
setShowSideBar={setIsSidebarVisible}
/>
</div>

<PostsList />
<div className="block" data-cy="MainContent">
{!userSelected && (
<p data-cy="NoSelectedUser">No user selected</p>
)}

{showPostLoader && <Loader />}

{hasPostError && (
<div
className="notification is-danger"
data-cy="PostsLoadingError"
>
Something went wrong!
</div>
)}

{userPosts.length > 0 && !showPostLoader && (
<PostsList
userPosts={userPosts}
handleShowComment={fetchPostComments}
showSideBar={isSidebarVisible}
setShowSideBar={setIsSidebarVisible}
setShowCommentForm={setIsCommentFormVisible}
/>
)}
{userSelected &&
userPosts.length === 0 &&
!showPostLoader &&
!hasPostError && (
<div
className="notification is-warning"
data-cy="NoPostsYet"
>
No posts yet
</div>
)}
</div>
</div>
</div>
</div>

<div
data-cy="Sidebar"
className={classNames(
'tile',
'is-parent',
'is-8-desktop',
'Sidebar',
'Sidebar--open',
)}
>
<div className="tile is-child box is-success ">
<PostDetails />
<div
data-cy="Sidebar"
className={classNames(
'tile',
'is-parent',
'is-8-desktop',
'Sidebar',
{ 'Sidebar--open': isSidebarVisible },
)}
>
<div className="tile is-child box is-success ">
<PostDetails
isCommentLoading={showCommentLoader}
postComments={postComments}
selectedPost={selectedPost}
errorsComment={hasCommentError}
onDeleteComment={removeComment}
onAddComment={createComment}
isButtonAddLoading={isAddingComment}
onShowCommentFormChange={setIsCommentFormVisible}
isCommentFormVisible={isCommentFormVisible}
userSelected={userSelected}
/>
</div>
</div>
</div>
</div>
</div>
</main>
);
</main>
);
};
22 changes: 22 additions & 0 deletions src/api/post.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { CommentData } from '../types/Comment';
import { client } from '../utils/fetchClient';

export const getUsers = () => {
return client.get('/users');
};

export const getUserPosts = (userId: number) => {
return client.get(`/posts?userId=${userId}`);
};

export const getUserPostComments = (postId: number) => {
return client.get(`/comments?postId=${postId}`);
};

export const addNewComment = (newComment: CommentData) => {
return client.post('/comments', newComment);
};

export const deleteComment = (commentId: number) => {
return client.delete(`/comments/${commentId}`);
};
Loading
Loading