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

Solution #854

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

Solution #854

wants to merge 3 commits into from

Conversation

VadymTsyndra
Copy link

@VadymTsyndra VadymTsyndra commented Jul 13, 2023

Copy link

@loralevitska loralevitska 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! There are things that should be fixed:

src/App.tsx Outdated
const [selectedUser, setSelectedUser] = useState<User | null>(null);
const [posts, setPosts] = useState<Post[] | null>(null);
const [currentPost, setCurrentPost] = useState<Post | null>(null);
const [loadingPosts, setLoadingPosts] = useState(false);

Choose a reason for hiding this comment

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

Fix it everywhere with tips on naming boolean variables

Suggested change
const [loadingPosts, setLoadingPosts] = useState(false);
const [isLoadingPosts, setIsLoadingPosts] = useState(false);

src/App.tsx Outdated
Comment on lines 31 to 35
async function fetchUsers() {
const data = await client.get<User[]>('/users');

setUsers(data);
}

Choose a reason for hiding this comment

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

Move this function out from useEffect

src/App.tsx Outdated
Comment on lines 78 to 80
const addComment = async ({
postId, name, email, body,
}: CommentData) => {

Choose a reason for hiding this comment

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

Suggested change
const addComment = async ({
postId, name, email, body,
}: CommentData) => {
const addComment = async ({
postId,
name,
email,
body,
}: CommentData) => {

src/App.tsx Outdated

return [response];
});
} catch (error: any) {

Choose a reason for hiding this comment

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

Don't use any for types. Add the correct type here
image

{!loadingComments && comments && comments.length > 0 && (
<p className="title is-4">Comments:</p>)}
{comments && comments.length > 0 && !loadingComments && comments.map(
comment => (

Choose a reason for hiding this comment

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

Destructure comment

voluptate et itaque vero tempora molestiae
</td>
<tbody>
{posts && posts.map(post => (

Choose a reason for hiding this comment

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

Destructure post

Choose a reason for hiding this comment

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

destructure post

Still not resolved

<a
href={`#user-${user.id}`}
key={user.id}
className={`dropdown-item ${selectedUser && selectedUser.id === user.id ? 'is-active' : ''}`}

Choose a reason for hiding this comment

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

Use classnames library to add classes conditionally

Copy link

@sTorba24 sTorba24 left a comment

Choose a reason for hiding this comment

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

Almost good!

wasn't able to test your work because of server error, could you rerequest review please when it'll be fixed?

src/App.tsx Outdated
<div className="notification is-warning" data-cy="NoPostsYet">
No posts yet
</div>
{posts && posts.length === 0 && (

Choose a reason for hiding this comment

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

Suggested change
{posts && posts.length === 0 && (
{posts && !posts.length && (

voluptate et itaque vero tempora molestiae
</td>
<tbody>
{posts && posts.map(post => (

Choose a reason for hiding this comment

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

destructure post

Still not resolved

Copy link

@VitaliyBodnar VitaliyBodnar 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!

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.

4 participants