-
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 #1196
base: master
Are you sure you want to change the base?
Solution #1196
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! Let's improve your code a bit
@@ -1,2 +1,3 @@ | |||
/build | |||
/node_modules | |||
/src |
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.
What's the purpose of it? It is better not to ignore eslint
const [isFormOpen, setFormOpen] = useState(false); | ||
|
||
useLayoutEffect(() => { | ||
fetchComments(); |
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.
{isLoading && <Loader />} | ||
|
||
{!isLoading && CommentsView} |
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.
Ternary operator can be used here
post: Post; | ||
} | ||
|
||
const PostListItem: FC<PostListItemProps> = memo(({ 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.
Let's create a separate directory for this component
|
||
<tbody> | ||
{posts.map(post => ( | ||
<tr data-cy="Post" key={post.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.
I guees this element should be inside PostItem component
src/context/PostContext.tsx
Outdated
const [selectedPost, setSelectedPost] = useState<Post | null>(null); | ||
const { posts, postsError, isLoadingPosts } = usePosts(selectedUser); | ||
|
||
const handleSelectUser = async (user: 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.
src/hooks/useComments.ts
Outdated
|
||
export const useComments = (postId: number) => { | ||
const [comments, setComments] = useState<Comment[]>([]); | ||
const [isLoading, 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.
const [isLoading, setLoading] = useState(false); | |
const [isLoading, setIsLoading] = useState(false); |
src/hooks/useComments.ts
Outdated
export const useComments = (postId: number) => { | ||
const [comments, setComments] = useState<Comment[]>([]); | ||
const [isLoading, setLoading] = useState(false); | ||
const [isError, 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.
const [isError, setError] = useState(''); | |
const [error, setError] = useState(''); |
is
prefix should be used only for boolean values
…have some problems with linter and without it I can't make a commit
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.
Let's fix a few comments before approving, overall it looks good to me
src/hooks/useCommentForm.ts
Outdated
const [name, setName] = useState(''); | ||
const [email, setEmail] = useState(''); | ||
const [body, setBody] = 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.
can we combine these three states into one, so we don't need three function for updates of name, email, body
src/hooks/useCommentForm.ts
Outdated
const handleChangeName = useCallback( | ||
(event: ChangeEvent<HTMLInputElement>) => { | ||
setName(event.target.value); | ||
|
||
if (nameError) { | ||
setNameError(''); | ||
} | ||
}, | ||
[nameError], | ||
); | ||
|
||
const handleChangeEmail = useCallback( | ||
(event: ChangeEvent<HTMLInputElement>) => { | ||
setEmail(event.target.value); | ||
|
||
if (emailError) { | ||
setEmailError(''); | ||
} | ||
}, | ||
[emailError], | ||
); | ||
|
||
const handleChangeBody = useCallback( | ||
(event: ChangeEvent<HTMLTextAreaElement>) => { | ||
setBody(event.target.value); | ||
|
||
if (bodyError) { | ||
setBodyError(''); | ||
} | ||
}, | ||
[bodyError], | ||
); |
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.
will be only one function after combine
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.
LGTM!
DEMO