solution #901

vlkzmn · 2023-08-21T08:16:09Z

Anastasiia-Svintsova

Almost done!

Anastasiia-Svintsova · 2023-08-21T12:58:46Z

src/App.tsx

+        .catch(() => setErrorMessage(`Can't load posts from ${user.name}`))
+        .finally(() => setLoading(false));
+    }
+  }, [user]);


The errorMessage value is missing in the dependencies

Suggested change

}, [user]);

}, [user, errorMessage]);

Hi! don't understand why

Anastasiia-Svintsova · 2023-08-21T13:00:34Z

src/components/PostDetails.tsx

+      .then(setComments)
+      .catch(() => setCommentsLoadError("Can't load comments for current post"))
+      .finally(() => setLoading(false));
+  }, [post]);


Add correct dependencies here

don't understand why

and then you dont need extra dependencies here

Anastasiia-Svintsova · 2023-08-21T13:00:58Z

src/components/PostDetails.tsx

+                  >
+                    <div className="message-header">
+                      <a href={`mailto:${comment.email}`} data-cy="CommentAuthor">
+                        {comment.name}


Don't forget about the destructuring

I think that destructuring is not needed here, at the top of the code there is an object with which there will be a conflict

you dont need whole obj here, so you can easily to make it here

And what do you suggest to do?

Anastasiia-Svintsova · 2023-08-21T13:01:57Z

src/components/UserSelector.tsx

+  const [userName, setUserName] = useState('Choose a user');
+  const dropdownSelect = useRef<HTMLDivElement>(null);
+
+  function handleSelectUser(user: User) {


Use arrow functions for better readability

Anastasiia-Svintsova · 2023-08-21T13:02:40Z

src/components/UserSelector.tsx

+
+export const UserSelector: React.FC<Props> = ({ users, onSetUser }) => {
+  const [dropdown, setDropdown] = useState(false);
+  const [userName, setUserName] = useState('Choose a user');


The initial value should be null if there is no user. And add a conditional rendering instead

Suggested change

const [userName, setUserName] = useState('Choose a user');

const [userName, setUserName] = useState(null);

I have initial value user === null in app.tsx
Here initial value 'Choose a user', and I don't need conditional rendering
What is the meaning of this action?

just logically that user name can't be choose a user
Here can be name or nothing, in our case null

sTorba24 · 2023-08-21T15:22:19Z

src/components/UserSelector.tsx

+
+export const UserSelector: React.FC<Props> = ({ users, onSetUser }) => {
+  const [dropdown, setDropdown] = useState(false);
+  const [userName, setUserName] = useState('Choose a user');


just logically that user name can't be choose a user
Here can be name or nothing, in our case null

sTorba24 · 2023-08-21T15:23:48Z

src/components/PostDetails.tsx

+  function handleDeleteComment(commentId: number) {
+    client.delete(`/comments/${commentId}`)
+      .then(() => {
+        setComments(
+          current => current?.filter(item => item.id !== commentId) || null,
+        );
+        setCommentsDelError([]);
+      })
+      .catch(() => setCommentsDelError(current => [...current, commentId]));
+  }
+
+  const handleAddNewComment = (newComment: Comment) => {
+    setComments(current => (current ? [...current, newComment] : [newComment]));
+  };


Use a consistent approach.

Or arrow functions, or function declaration

#901 (comment)
В цьому компоненті ми робимо dropdown, тобто іметуємо select, а в select ми ж робимо перший option ось так,
<option value="0" disabled>Choose a user...</option>
це що теж не логічно, і тут таж історія. Є змінна userName, стартове значення 'Choose a user' яке логічно відносится до назви змінної, потім це значення змінюється на обране, та й ще умовний рендерінг не треба використовувати. Якщо це зауваження тільки про логічне що змінна не може мати таке значення, то я вважаю що це зайве.

maxim2310

Great job overall! Just couple tiny things needs to be fixed before approval)

maxim2310 · 2023-08-21T17:49:09Z

src/components/PostDetails.tsx

+    if (commentsLoadError) {
+      setCommentsLoadError('');
+    }
+
+    if (comments) {
+      setComments(null);
+    }
+
+    if (addNewComment) {
+      setAddNewComment(false);
+    }


why do you need to reset this value here, better to create a separate func for this and do it in add func after clicking on submit but not on re rendering component

maxim2310 · 2023-08-21T17:49:41Z

src/components/PostDetails.tsx

+      .then(setComments)
+      .catch(() => setCommentsLoadError("Can't load comments for current post"))
+      .finally(() => setLoading(false));
+  }, [post]);


and then you dont need extra dependencies here

maxim2310 · 2023-08-21T17:50:51Z

src/components/PostDetails.tsx

+                  >
+                    <div className="message-header">
+                      <a href={`mailto:${comment.email}`} data-cy="CommentAuthor">
+                        {comment.name}


you dont need whole obj here, so you can easily to make it here

vlkzmn · 2023-08-21T18:43:20Z

#901 (comment)
You suggest destructuring?

vlkzmn · 2023-08-21T18:47:33Z

#901 (comment)
#901 (comment)
Цей фрагмент коду завантажує список коментів при виборі чи зміні поста, він не має відношення до додавання нового поста, яке correct чи extra dependencies ви маєте на увазі?

anastasiia-tilikina

let's fix small issues in my comments.
and discuss other questions in the Slack chat. That would be faster and easier) 🙌 just tag mentors and ask your questions)

anastasiia-tilikina · 2023-08-22T07:12:48Z

src/components/PostDetails.tsx

          </p>
        </div>

-        <div className="block">
+        {loading && (


use ternary operator here for conditional rendering

anastasiia-tilikina · 2023-08-22T07:15:02Z

src/components/PostDetails.tsx


-          <p className="title is-4">Comments:</p>
+                {comments.map(comment => (


Suggested change

{comments.map(comment => (

{comments.map(({name, email, id, body})=> (

you can easily destructure your obj like this

solution

a033a79

Anastasiia-Svintsova suggested changes Aug 21, 2023

View reviewed changes

vlkzmn requested a review from Anastasiia-Svintsova August 21, 2023 15:03

sTorba24 suggested changes Aug 21, 2023

View reviewed changes

fixed

0c12094

vlkzmn requested a review from sTorba24 August 21, 2023 17:26

maxim2310 suggested changes Aug 21, 2023

View reviewed changes

vlkzmn mentioned this pull request Aug 21, 2023

you dont need whole obj here, so you can easily to make it here #902

Closed

vlkzmn requested a review from maxim2310 August 21, 2023 18:47

anastasiia-tilikina suggested changes Aug 22, 2023

View reviewed changes

fixed

4512c25

vlkzmn requested a review from anastasiia-tilikina August 23, 2023 09:57

vlkzmn added 3 commits August 23, 2023 13:12

fixed

2cef83f

optimization

a20221d

fixed

3e166ad

Anastasiia-Svintsova approved these changes Aug 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solution #901

solution #901

vlkzmn commented Aug 21, 2023 •

edited

Loading

Anastasiia-Svintsova left a comment

Anastasiia-Svintsova Aug 21, 2023

vlkzmn Aug 21, 2023 •

edited

Loading

Anastasiia-Svintsova Aug 21, 2023

vlkzmn Aug 21, 2023

maxim2310 Aug 21, 2023

Anastasiia-Svintsova Aug 21, 2023

vlkzmn Aug 21, 2023

maxim2310 Aug 21, 2023

vlkzmn Aug 21, 2023

Anastasiia-Svintsova Aug 21, 2023

Anastasiia-Svintsova Aug 21, 2023

vlkzmn Aug 21, 2023

sTorba24 Aug 21, 2023

sTorba24 Aug 21, 2023

sTorba24 Aug 21, 2023

vlkzmn Aug 21, 2023 •

edited

Loading

maxim2310 left a comment

maxim2310 Aug 21, 2023

maxim2310 Aug 21, 2023

maxim2310 Aug 21, 2023

vlkzmn commented Aug 21, 2023

vlkzmn commented Aug 21, 2023 •

edited

Loading

anastasiia-tilikina left a comment •

edited

Loading

anastasiia-tilikina Aug 22, 2023

anastasiia-tilikina Aug 22, 2023

	const [userName, setUserName] = useState('Choose a user');
	const [userName, setUserName] = useState(null);


		<p className="title is-4">Comments:</p>
		{comments.map(comment => (

	{comments.map(comment => (
	{comments.map(({name, email, id, body})=> (

solution #901

Are you sure you want to change the base?

solution #901

Conversation

vlkzmn commented Aug 21, 2023 • edited Loading

Anastasiia-Svintsova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlkzmn Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlkzmn Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

maxim2310 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlkzmn commented Aug 21, 2023

vlkzmn commented Aug 21, 2023 • edited Loading

anastasiia-tilikina left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlkzmn commented Aug 21, 2023 •

edited

Loading

vlkzmn Aug 21, 2023 •

edited

Loading

vlkzmn Aug 21, 2023 •

edited

Loading

vlkzmn commented Aug 21, 2023 •

edited

Loading

anastasiia-tilikina left a comment •

edited

Loading