-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: list of posts provided #876
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, approving! I really liked the way you created custom hooks and divided context providers 💣I left some small fixes for you to consider. Also, I wonder why you used models, types and constants, but maybe it's just an approach I am not familiar with.
// <div className="tile is-ancestor"> | ||
// <div className="tile is-parent"> | ||
// <div className="tile is-child box is-success"> | ||
// <div className="block"> | ||
// <UserSelector /> | ||
// </div> | ||
// | ||
// <div className="block" data-cy="MainContent"> | ||
// <p data-cy="NoSelectedUser"> | ||
// No user selected | ||
// </p> | ||
// | ||
// <Loader /> | ||
// | ||
// <div | ||
// className="notification is-danger" | ||
// data-cy="PostsLoadingError" | ||
// > | ||
// Something went wrong! | ||
// </div> | ||
// | ||
// <div className="notification is-warning" data-cy="NoPostsYet"> | ||
// No posts yet | ||
// </div> | ||
// | ||
// <PostsList /> | ||
// </div> | ||
// </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's nice not to leave unnecessary comments
const [emailError, setEmailError] = useState(false); | ||
const [textError, setTextError] = useState(false); | ||
|
||
const [loading, setLoading] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder, variables with boolean values usually start with is/has
const handleSubmit = async (event: FormEvent) => { | ||
event.preventDefault(); | ||
try { | ||
setLoading(true); | ||
|
||
if (validate() && selectedPost) { | ||
await addComment({ | ||
name: inputName, | ||
email: inputEmail, | ||
postId: selectedPost?.id, | ||
body: inputText, | ||
}); | ||
} | ||
|
||
setInputText(''); | ||
} catch (error) { | ||
setNameError(!inputName.trim()); | ||
setEmailError(!inputEmail.trim()); | ||
setTextError(!inputText.trim()); | ||
} finally { | ||
setLoading(false); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to setLoading before the try block, and setInputText in the finally block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
I found some issues, please check and fix them
Best regards
@@ -0,0 +1,23 @@ | |||
import { useContext } from 'react'; | |||
// eslint-disable-next-line import/no-cycle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code contains a circular dependency as indicated by the eslint-disable-next-line comment. Circular dependencies can lead to unexpected behaviors and should be avoided.
</div> | ||
</main> | ||
); | ||
}; | ||
|
||
// <div className="tile is-ancestor"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented out code should not be left in the production code. It makes the codebase less readable and could potentially introduce confusion for other developers. If you need to save the code for future reference, consider using version control systems like git.
import { ApiEndpoint } from '../utils/constants'; | ||
|
||
export const getCommentsByPost = (postId: number) => { | ||
return client.get<IComment[]>(ApiEndpoint.GetCommentsByPost + postId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concatenation of the postId to the ApiEndpoint.GetCommentsByPost might lead to incorrect URLs if not properly formatted. Consider using template strings for clarity and to avoid potential errors.
const loadPosts = async (userId: number) => { | ||
setPostsLoading(true); | ||
setSelectedPost(null); | ||
const postsFromApi = await getPostsByUser(userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getPostsByUser asynchronous function execution is not wrapped in a try-catch block. If the function throws an error, it would not be caught and it could break the application.
useEffect, | ||
useState, | ||
} from 'react'; | ||
// eslint-disable-next-line import/no-cycle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import statement has a cyclic dependency, which could lead to issues where one module fails because it's dependent on a second module, which is in turn dependent on the first module.
comment => comment.id !== commentId, | ||
)); | ||
|
||
await deleteCommentById(commentId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting a comment in the state before ensuring it can be deleted on the server can lead to data inconsistency. Move line 59 after line 63.
import React, { | ||
createContext, PropsWithChildren, useEffect, useState, | ||
} from 'react'; | ||
// eslint-disable-next-line import/extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid disabling ESLint rules whenever possible. If you have a good reason to disable a rule, you should document it with a comment.
|
||
setComments(prevState => [...prevState, newComment]); | ||
} catch (error) { | ||
setCommentsLoadingError(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set a more detailed error message. This will help you debug issues in the future.
getCommentsByPost, | ||
postComment, | ||
} from '../api/comment.api'; | ||
// eslint-disable-next-line import/no-cycle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should avoid cyclical dependencies as they can lead to unexpected behavior. If the 'usePost' hook depends on this file and this file depends on 'usePost', it may be a sign that you need to refactor your code to remove the circular dependency.
} | ||
|
||
export const CommentContext = createContext<ICommentContext>( | ||
{} as ICommentContext, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating context with an empty object could lead to potential issues. Consider providing a default implementation of the context interface.
DEMO LINK