sol #863
sol #863
Conversation
src/components/UserSelector.tsx
Outdated
| setOpen(false); | ||
| }; | ||
|
|
||
| document.addEventListener('click', (event) => { |
There was a problem hiding this comment.
Adding event listeners directly to the document can lead to memory leaks. Instead, use useEffect to handle the adding and removing of event listeners within a component lifecycle.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable @typescript-eslint/no-explicit-any */ | |||
There was a problem hiding this comment.
The 'any' type is being used, which is not recommended in TypeScript. It would be better to provide a specific type if possible to gain the benefits of using TypeScript.
There was a problem hiding this comment.
Any type is used only once, so we can send any data inside the body to server, in other functions generics are used.
|
|
||
| setIsLoading(true); | ||
|
|
||
| client.post<Comment>('/comments', { |
There was a problem hiding this comment.
You are not handling the error case when the api call fails. You should add a .catch block to handle any errors.
There was a problem hiding this comment.
The task to handle errors was optional, so I decided to skip it for simplicity)
(in fix added empty catch)
| @@ -1,8 +1,70 @@ | |||
| import React from 'react'; | |||
| import classNames from 'classnames'; | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
The import order should be organized. External libraries should be imported first, then internal files.
src/components/PostDetails.tsx
Outdated
|
|
||
| setComments(comments.filter((c) => c.id !== comment.id)); | ||
|
|
||
| client.delete(`/comments/${comment.id}`); |
There was a problem hiding this comment.
The delete operation should ideally be handled with error checks. It's good practice to handle .then() and .catch() for the promises.
There was a problem hiding this comment.
The task to handle errors was optional, so I decided to skip it for simplicity)
(in fix added simple catch)
| }) => { | ||
| const [comments, setComments] = useState<Comment[] | null>(null); | ||
| const [IsLoading, setIsLoading] = useState(false); | ||
| const [errorMsg, setErrorMsg] = useState(''); |
There was a problem hiding this comment.
The variable 'IsLoading' does not follow the camelCase naming convention. It should be renamed to 'isLoading'.
src/components/PostDetails.tsx
Outdated
| </div> | ||
| {errorMsg && !IsLoading && ( | ||
| <div className="notification is-danger" data-cy="CommentsError"> | ||
| Something went wrong |
There was a problem hiding this comment.
The error message is hardcoded which is not a good practice. Instead, display the error message returned from the API.
There was a problem hiding this comment.
Good comment, but, I think, in example all values are hard coded with Something went wrong. Changed to API message in fix.
| return () => { | ||
| document.removeEventListener('click', handleEvent); | ||
| }; |
DEMO LINK