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

Develop #919

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Develop #919

wants to merge 9 commits into from

Conversation

Wesses
Copy link

@Wesses Wesses commented Sep 21, 2023

DEMO LINK
Something wrong with tests, please proceed to review the demo link.

Copy link

@roman-mirzoian roman-mirzoian left a comment

Choose a reason for hiding this comment

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

Good job, but there is quite a bit of work to be done.

import React, { useState } from 'react';
import { Comment } from '../../types/Comment';

type CC = {

Choose a reason for hiding this comment

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

What does it mean - CC?
Do not abbreviate variable names to unreadable ones

Comment on lines 34 to 66
{modalUser ? (
<>
{isLoad ? <Loader /> : (
<>
{isError ? (
<div
className="notification is-danger"
data-cy="PostsLoadingError"
>
Something went wrong!
</div>
) : (
<>
{posts.length ? (
<PostsList posts={posts} />
) : (
<div
className="notification is-warning"
data-cy="NoPostsYet"
>
No posts yet
</div>
)}
</>
)}
</>
)}
</>
) : (
<p data-cy="NoSelectedUser">
No user selected
</p>
)}

Choose a reason for hiding this comment

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

Everything works fine now, but such a large nestedness is hard to read and maintain in the future.
Try to break it down into smaller components

import React, { useState } from 'react';
import { Post } from '../../types/Post';

type MPC = {

Choose a reason for hiding this comment

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

Do not abbreviate variable names to unreadable ones

import React, { useState } from 'react';
import { User } from '../../types/User';

type MUC = {

Choose a reason for hiding this comment

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

Do not abbreviate variable names to unreadable ones

const { comments, setComments } = useContext(CommentsContext);
const { modalPost } = useContext(ModalPostContext);

const errorCondition = (key: keyof CommentData) => {

Choose a reason for hiding this comment

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

Suggested change
const errorCondition = (key: keyof CommentData) => {
const checkForErrors = (key: keyof CommentData) => {

}
};

const handleReset = (event: React.FormEvent<HTMLFormElement>) => {

Choose a reason for hiding this comment

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

Handle reset of what?

setFormFields(DEFAULT_FIELDS);
};

const handleFieldChange = (

Choose a reason for hiding this comment

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

Suggested change
const handleFieldChange = (
const handleFormFieldChange = (
Suggested change
const handleFieldChange = (
const handleInputChange = (

Copy link

@roman-mirzoian roman-mirzoian left a comment

Choose a reason for hiding this comment

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

Almost done!

Comment on lines 14 to 17
const showLoad = isLoad && !isError && !posts.length;
const showError = !isLoad && isError && !posts.length;
const showPostList = !isLoad && !isError && !!posts.length;
const showNoPostsYet = !isLoad && !isError && !posts.length;

Choose a reason for hiding this comment

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

Functions are usually named from the verb, and you have variables.
In addition, you do similar checks in another place, so it can be transferred to a separate helper, where data can be passed as an argument, compared in the middle of the helper and return a Boolean result, and reused in two places.
image

Copy link

@BudnikOleksii BudnikOleksii left a comment

Choose a reason for hiding this comment

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

Nicely done 🚀

const handleDeleteComment = () => {
setComments(comments.filter(({ id: comId }) => comId !== id));

deleteCommentById(id);

Choose a reason for hiding this comment

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

In case of error, the user doesn't know anything about it and think that they successfully deleted the comment 🤷‍♂️

Comment on lines +23 to +37
setIsError(false);
setIsLoad(true);
if (modalUser) {
(async () => {
try {
const serverPosts = await getPostsByUserId(modalUser.id);

setPosts(serverPosts);
} catch {
setIsError(true);
} finally {
setIsLoad(false);
}
})();
}

Choose a reason for hiding this comment

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

Suggested change
setIsError(false);
setIsLoad(true);
if (modalUser) {
(async () => {
try {
const serverPosts = await getPostsByUserId(modalUser.id);
setPosts(serverPosts);
} catch {
setIsError(true);
} finally {
setIsLoad(false);
}
})();
}
if (modalUser) {
setIsError(false);
setIsLoad(true);
(async () => {
try {
const serverPosts = await getPostsByUserId(modalUser.id);
setPosts(serverPosts);
} catch {
setIsError(true);
} finally {
setIsLoad(false);
}
})();
}

If no modalUser you still set is load to true

'is-danger': checkForErrors('name'),
})}
value={formFields.name}
onChange={event => handleFormFieldChange(event, 'name')}

Choose a reason for hiding this comment

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

Hint, you can get the input name from event.target

const handleFormFieldChange = (
    event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>,
  ) => {
    const { name, value } = event.target;
    setFormFields(prevState => ({ ...prevState, [key]: value  }));
  };

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants