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 #1022

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

solution #1022

wants to merge 10 commits into from

Conversation

oleksiy-uchaev
Copy link

@oleksiy-uchaev oleksiy-uchaev commented Jan 19, 2024

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! Please check your DEMO link, it's link to api partly(but it's not place where you should change it and it's not link to send to review)

README.md Outdated
@@ -10,7 +10,7 @@ form to add new comments.
1. Initially the `App` shows the `UserSelector` and a paragraph `No user selected` in the main content block.
- load users from the API on page load;
- implement the `UserSelector` as a dropdown using the given markup;
1. When a user is selected load the user's posts form [the API](https://mate-academy.github.io/fe-students-api/) and show them using a table in the main content clock;
1. When a user is selected load the user's posts form [the API](https://oleksiy-uchaev.github.io/fe-students-api/) and show them using a table in the main content clock;

Choose a reason for hiding this comment

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

you shouldn't change this link, it's mate-academy api

Comment on lines 41 to 45
{ 'is-active': comments.length === 0 },
{
'is-hidden': comments.length !== 0
|| loadType === Load.Comments,
})}

Choose a reason for hiding this comment

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

Suggested change
{ 'is-active': comments.length === 0 },
{
'is-hidden': comments.length !== 0
|| loadType === Load.Comments,
})}
{ 'is-active': !comments.length },
{
'is-hidden': comments.length
|| loadType === Load.Comments,
})}

Copy link

@volodymyr-soltys97 volodymyr-soltys97 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 👍
Let's improve your code

href={`mailto:${comment.email}`}
data-cy="CommentAuthor"
>
{comment.name}

Choose a reason for hiding this comment

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

Use the destruction for comment


return (
<>
{comments.length > 0 && <p className="title is-4">Comments:</p>}

Choose a reason for hiding this comment

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

Suggested change
{comments.length > 0 && <p className="title is-4">Comments:</p>}
{!!comments.length && <p className="title is-4">Comments:</p>}

<button
type="reset"
className="button is-link is-light"
onClick={() => handleClearForm()}

Choose a reason for hiding this comment

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

Suggested change
onClick={() => handleClearForm()}
onClick={handleClearForm}

Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 left a comment

Choose a reason for hiding this comment

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

Well done.

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