-
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
solution #1185
base: master
Are you sure you want to change the base?
solution #1185
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.
Good job!
There are several no big problems, I mentioned below.
Chelsey Dietrich | ||
</a> | ||
{isClicked && ( | ||
<div className="dropdown-menu" id="dropdown-menu" role="menu"> |
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 dropdown menu should close after clicking on other elements (when it loses focus)
const [isClicked, setIsClicked] = useState(false); | ||
|
||
const handleSelectUser = (user: User) => { | ||
setUserSelected(user); |
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 need also to reset the selected post
/> | ||
)} | ||
|
||
{!isShownForm && !isLoadingComments && ( |
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.
If the error occurs. "write a comment" button should be hidden
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.
Thank you for your hard work on this pull request! While your code is mostly well-implemented, there are a few critical issues that need to be addressed before we can approve it.
-
Error Handling: The error handling across various parts of the code is minimal and generic. It's important to provide more informative error messages, possibly including status codes or response texts, to aid in debugging and user feedback. Specifically, the
handleDeleteComment
function lacks error handling, and theaddComment
function should set an error state to inform the user when an addition fails. -
Variable Naming: There's a repeated typo in the variable name
comentText
. It should be corrected tocommentText
for consistency and to avoid confusion. -
Finally Block: The
finally
block in youruseEffect
is currently empty. Consider removing it if no cleanup or final actions are needed.
By addressing these issues, you'll improve the robustness and clarity of your code. Keep up the great work, and I'm confident you'll nail it with these adjustments! 😊
✨ 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.
src/App.tsx
Outdated
.catch(() => setError('Something went wrong!')) | ||
.finally(() => {}); |
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 error handling here is quite generic. It might be beneficial to provide more specific error messages based on the type of error encountered. This will help in debugging and understanding the issue better.
src/App.tsx
Outdated
getUsers() | ||
.then(setUsers) | ||
.catch(() => setError('Something went wrong!')) | ||
.finally(() => {}); |
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 finally
block is currently empty and does not perform any actions. Consider removing it if no cleanup or final actions are needed after the promise resolution.
src/components/NewCommentForm.tsx
Outdated
const [authorNameError, setAuthorNameError] = useState(false); | ||
const [authorEmail, setAuthorEmail] = useState(''); | ||
const [authorEmailError, setAuthorEmailError] = useState(false); | ||
const [comentText, setComentText] = useState(''); |
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.
There is a typo in the variable name comentText
. It should be commentText
to maintain consistency and avoid confusion.
src/components/NewCommentForm.tsx
Outdated
|
||
case 'body': | ||
setCommentTextError(false); | ||
setComentText(event.target.value.trim()); |
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 variable comentText
is misspelled. It should be commentText
.
src/components/NewCommentForm.tsx
Outdated
|
||
const preparedAuthorName = authorName.trim(); | ||
const preparedAuthorEmail = authorEmail.trim(); | ||
const preparedCommentText = comentText.trim(); |
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 variable comentText
is misspelled. It should be commentText
.
src/components/NewCommentForm.tsx
Outdated
addComment(commentToAdd) | ||
.then(newComment => { | ||
setComments(currentComments => [...currentComments, newComment]); | ||
setComentText(''); |
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 variable comentText
is misspelled. It should be commentText
.
setComments(currentComments => [...currentComments, newComment]); | ||
setComentText(''); | ||
}) | ||
.catch(() => 'Something Wrong') |
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 error handling here is not effective. Consider setting an error state to display a message to the user when the comment addition fails.
src/components/PostDetails.tsx
Outdated
const handleDeleteComment = (id: Comment['id']) => { | ||
deleteComment(id).then(() => | ||
setComments(currentComments => | ||
currentComments.filter(comment => comment.id !== id), | ||
), | ||
); |
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 handleDeleteComment
function lacks error handling. Consider adding a catch
block to handle potential errors during the deletion process and inform the user appropriately.
src/utils/fetchClient.ts
Outdated
.then(response => response.json()); | ||
.then(response => { | ||
if (!response.ok) { | ||
throw new Error(); |
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 error handling here is minimal. Consider throwing an error with a more informative message, such as including the status code or response text, to aid in debugging.
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.
Thank you for your submission! Your code generally meets the task requirements, and I'm approving it to help you move forward with your learning. However, there are a few important areas for improvement:
-
PropTypes: Ensure that all components have
propTypes
defined for type checking. This is crucial for maintaining robust code. -
Error Handling: Consider using
try-catch
blocks for error handling instead of.catch()
. This aligns with best practices and provides clearer error management. -
Specific Error Messages: Avoid generic error messages like 'Something wrong'. Providing specific error details can greatly aid in debugging and user feedback.
-
Dropdown Toggle Logic: The
is-active
class is always applied to the dropdown. Make sure it is conditionally applied based on theisClicked
state. -
State Management: Ensure that state management logic is clear and correct, such as initializing the
error
state withnull
instead of an empty string. -
Complex Conditions: Review complex conditions to ensure they accurately reflect the intended logic, such as the condition for displaying the 'No posts yet' message.
-
Function Implementation: Verify that functions like
getPosts
correctly handle parameters likeuserId
to avoid unexpected behavior.
These improvements will enhance the functionality and maintainability of your code. Keep up the great work, and continue to learn from these experiences! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
const [selectedPost, setSelectedPost] = useState<Post | null>(null); | ||
|
||
const [isLoading, setIsLoading] = useState(false); | ||
const [error, setError] = useState(''); |
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.
Consider initializing the error
state with null
instead of an empty string to better represent the absence of an error. This will also help in conditionally rendering error messages more clearly.
setError(''); | ||
setIsLoading(true); | ||
|
||
getPosts(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.
Ensure that the getPosts
function is correctly implemented to accept a userId
and return a promise. If getPosts
does not handle the userId
correctly, it might lead to unexpected behavior.
</div> | ||
)} | ||
|
||
{userSelected && !error && posts.length === 0 && !isLoading && ( |
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 condition userSelected && !error && posts.length === 0 && !isLoading
might be complex. Ensure that each part of this condition is necessary and correctly reflects the intended logic for displaying the 'No posts yet' message.
|
||
const handleResetForm = () => { | ||
setAuthorName(''); | ||
setAuthorEmailError(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.
This line mistakenly sets setAuthorEmailError(false)
twice. The first occurrence should be setAuthorNameError(false)
to reset the name error state.
setComments(currentComments => [...currentComments, newComment]); | ||
setCommentText(''); | ||
}) | ||
.catch(() => 'Something Wrong') |
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 catch block should handle errors more explicitly. Consider displaying an error message to the user or logging the error for debugging purposes.
currentComments.filter(comment => comment.id !== id), | ||
), | ||
) | ||
.catch(() => 'Unable to delete post'); |
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 catch block for deleteComment
should handle errors more explicitly. Consider displaying an error message to the user or logging the error for debugging purposes.
|
||
getComments(selectedPost.id) | ||
.then(setComments) | ||
.catch(() => setError('Something went wrong')) |
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 error message in the catch block for getComments
is generic. Consider providing more specific error information to help users understand what went wrong.
|
||
export const UserSelector: React.FC = () => { | ||
return ( | ||
<div data-cy="UserSelector" className="dropdown is-active"> |
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 class is-active
is always applied to the dropdown, which might not be the intended behavior. Consider conditionally applying this class based on the isClicked
state to properly toggle the dropdown visibility.
.then(response => response.json()); | ||
.then(response => { | ||
if (!response.ok) { | ||
throw new Error('Something wrong'); |
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 error message 'Something wrong' is too generic. Consider providing more specific error information, such as the status code or response text, to help with debugging and user feedback.
demo - https://yuliiashatkovska.github.io/react_dynamic-list-of-posts/
не підтягуються стилі з бульми для цих елементів ▿
та не проходить тест, хоча наче має працювати все коректно should not request posts from API:
AssertionError: Timed out retrying after 4000ms: expected posts to not have been called at least once
at Context.eval (webpack://react_dynamic-list-of-posts/./cypress/integration/page.spec.js:236:25)
на qna ментор сказав запушити так