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

add task solution #925

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

Conversation

AnnaViolin23
Copy link

Copy link

@TarasHoliuk TarasHoliuk left a comment

Choose a reason for hiding this comment

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

GJ 👍
One thing should be fixed (cannot add comment), also, added a few comments to improve code :)

src/App.tsx Outdated
useEffect(() => {
getAllUsers()
.then(usersFromServer => {
if (usersFromServer) {

Choose a reason for hiding this comment

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

I guess this check is redundant here
If the request is successful we will get an array (at least empty)

but not critical

setPosts(postsFromServer);
}
})
.finally(() => setIsLoading(false));

Choose a reason for hiding this comment

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

would be nice to add .catch too here

src/App.tsx Outdated
Comment on lines 94 to 99
{(!posts.length
&& selectedUser
&& !errorMessage
&& !isLoading
&& !isDropdownActive
) && (

Choose a reason for hiding this comment

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

I'd move it into a variable above. Do you need isDropdownActive here?

setSelectedPost,
setIsCommentButtonClicked,
}) => {
const selectedPostHandler = (post: Post) => {

Choose a reason for hiding this comment

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

create this function in the App.tsx and pass as prop instead of setSelectedPost, and setIsCommentButtonClicked,

const dropdown = useRef<HTMLDivElement>(null);

useEffect(() => {
const closeDropdown = (e: MouseEvent) => {

Choose a reason for hiding this comment

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

define function outside the useEffect hook

.finally(() => setIsCommentsLoading(false));
}, [selectedPost]);

const deleteComment = (id: number) => {

Choose a reason for hiding this comment

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

I'd move all such functions into a separate file

Choose a reason for hiding this comment

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

Still not fixed from the previous review

return client.delete(`/comments/${id}`);
};

const handleDeletingComment = (id: number) => {

Choose a reason for hiding this comment

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

Suggested change
const handleDeletingComment = (id: number) => {
const handleDeleteComment = (id: number) => {

Comment on lines 32 to 35
const handleNameChange = (event: React.ChangeEvent<HTMLInputElement>) => {
setCommentName(event.target.value);
setFormError({ ...formError, name: '' });
};

Choose a reason for hiding this comment

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

you may save all values in state as object (the same as errors), and create 1 handler. Use event.target.name as key for object:

setFormValue(prevValues => ({ ...prevValues, [name]: value }))

const addComment = (event: React.FormEvent<HTMLFormElement>) => {
event.preventDefault();

if (!commentName) {

Choose a reason for hiding this comment

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

it's better to trim value here - count spaces only as an empty string

setFormError(rest => ({ ...rest, name: 'Enter some text' }));
}

if (selectedPost?.id && !isEmptyField) {

Choose a reason for hiding this comment

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

in case selectedPost?.id is 0 number it will not work

generally, it doesn't work, also, there is a strange error for the name field:
Uploading Screencast from 28-09-23 19:49:44.webm…

Choose a reason for hiding this comment

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

Not fixed

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 work 👍
Let's improve your code

src/App.tsx Outdated
Comment on lines 30 to 32
const getAllUsers = () => {
return client.get<User[]>('/users');
};

Choose a reason for hiding this comment

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

Still not fixed from the previous review, will be better if you create a separate file for all helpers functions

src/App.tsx Outdated

export const App: React.FC = () => {
const [users, setUsers] = useState<User[]>([]);
const [posts, setPosts] = useState<Post[]>([]);
const [errorMessage, setErrorMessage] = useState('');

Choose a reason for hiding this comment

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

Add a generic types everywher

Suggested change
const [errorMessage, setErrorMessage] = useState('');
const [errorMessage, setErrorMessage] = useState<string>('');

src/App.tsx Outdated
<div className="notification is-warning" data-cy="NoPostsYet">
No posts yet
</div>
{(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.length > 0
{(!!posts.length

src/App.tsx Outdated
<PostDetails />
</div>

{selectedPost !== null && (

Choose a reason for hiding this comment

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

Suggested change
{selectedPost !== null && (
{selectedPost && (

src/App.tsx Outdated
@@ -50,12 +132,19 @@ export const App: React.FC = () => {
'is-parent',
'is-8-desktop',
'Sidebar',
'Sidebar--open',
{ 'Sidebar--open': selectedPost !== null },

Choose a reason for hiding this comment

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

Suggested change
{ 'Sidebar--open': selectedPost !== null },
{ 'Sidebar--open': selectedPost },

.finally(() => setIsCommentsLoading(false));
}, [selectedPost]);

const deleteComment = (id: number) => {

Choose a reason for hiding this comment

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

Still not fixed from the previous review

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work! Almost done

setFormError(rest => ({ ...rest, name: 'Enter some text' }));
}

if (selectedPost?.id && !isEmptyField) {

Choose a reason for hiding this comment

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

Not fixed

}) => {
const dropdownHandler = () => setIsDropdownActive(!isDropdownActive);

const dropdown = useRef<HTMLDivElement>(null);

Choose a reason for hiding this comment

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

Not fixed

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work!

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work!

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