task done #877

pos1bl · 2023-08-03T13:00:54Z

https://pos1bl.github.io/react_dynamic-list-of-posts/

IvanFesenko

Good job. Some improvements required

IvanFesenko · 2023-08-03T13:18:31Z

src/App.tsx

+    setComments(prevComments => prevComments
+      .filter(comment => comment.id !== id));
+    postService.deleteComment(id);


What happens when postService.deleteComment - will catch an error?

IvanFesenko · 2023-08-03T13:21:05Z

src/components/NewCommentForm.tsx

+  const [name, setName] = useState('');
+  const [email, setEmail] = useState('');
+  const [text, setText] = useState('');
+  const [nameError, setNameError] = useState(false);
+  const [emailError, setEmailError] = useState(false);
+  const [textError, setTextError] = useState(false);


My assumption it shoud be 2 states instead of 6

Suggested change

const [name, setName] = useState('');

const [email, setEmail] = useState('');

const [text, setText] = useState('');

const [nameError, setNameError] = useState(false);

const [emailError, setEmailError] = useState(false);

const [textError, setTextError] = useState(false);

const [formFieds, setFormFieds] = useState({

name: '',

email: '',

text: '',

});

const [errorMessage, setErrorMessage] = useState('');

its a bad idea to create errorMessage, because we would use it 3 times.

and about object, if i change only name, it will rerender all object. to my mind, its better to use 3 useStates.

and about object, if i change only name, it will rerender all object. to my mind, its better to use 3 useStates.

FYI: You probably meant that we will change the entire object stored in the state, but that's not a problem. we do not render an object. we render the component (jsx that is written below), and re-create every variable (including) functions during the render. Having different states would be useful if you use memorization hooks with different variables in the dependency arrays.

IvanFesenko · 2023-08-03T13:23:48Z

src/components/NewCommentForm.tsx

+    if (!name.trim()) {
+      setNameError(true);
+    }
+
+    if (!email.trim()) {
+      setEmailError(true);
+    }
+
+    if (!text.trim()) {
+      setTextError(true);
+    }
+
+    if (!name.trim() || !email.trim() || !text.trim()) {
+      return;
+    }


Create separate variables instead of 6 times calls trim()

IvanFesenko · 2023-08-03T13:25:41Z

src/components/NewCommentForm.tsx

@@ -14,24 +98,33 @@ export const NewCommentForm: React.FC = () => {
            name="name"
            id="comment-author-name"
            placeholder="Name Surname"
-            className="input is-danger"
+            value={name}
+            onChange={(event) => handleNameChange(event.target.value)}


Instead of additiona callback get target.value in handleNameChange
Fix all similar cases

Suggested change

onChange={(event) => handleNameChange(event.target.value)}

onChange={handleNameChange}

IvanFesenko · 2023-08-03T13:26:22Z

src/components/PostDetails.tsx

+                return (
+                  <article className="message is-small" data-cy="Comment">
+                    <div className="message-header">


Dont forget about key prop

IvanFesenko · 2023-08-03T13:26:49Z

src/components/PostsList.tsx

+            return (
+              <tr data-cy="Post">


IvanFesenko · 2023-08-03T13:27:09Z

src/components/UserSelector.tsx

+              <a
+                href={`#user-${user.id}`}
+                onClick={() => handleSelect(user)}
+                className={classNames(
+                  'dropdown-item',
+                  { 'is-active': selectedUser?.id === user.id },
+                )}


TarasHoliuk · 2023-08-03T17:04:14Z

src/App.tsx

+  const [users, setUsers] = useState<User[]>([]);
+  const [posts, setPosts] = useState<Post[]>([]);
+  const [comments, setComments] = useState<Comment[]>([]);
+  const [selectedUser, setSelectedUser] = useState<User | null>(null);
+  const [selectedPost, setSelectedPost] = useState<Post | null>(null);
+  const [isLoading, setIsLoading] = useState(false);
+  const [isError, setIsError] = useState(false);
+  const [isCmntsLoading, setIsCmntsLoading] = useState(false);
+  const [isCmntsError, setIsCmntsError] = useState(false);
+  const [isWriting, setIsWriting] = useState(false);


there are too many states and logic in App.tsx
try to move part of them to other components

TarasHoliuk · 2023-08-03T17:05:52Z

src/components/NewCommentForm.tsx

+  const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {
+    event.preventDefault();
+
+    const nameValidate = !name.trim;


Suggested change

const nameValidate = !name.trim;

const isNameValide = !!name.trim;

for boolean values, it's better to use such names

TarasHoliuk · 2023-08-03T17:11:04Z

src/components/NewCommentForm.tsx

+  const [name, setName] = useState('');
+  const [email, setEmail] = useState('');
+  const [text, setText] = useState('');
+  const [nameError, setNameError] = useState(false);
+  const [emailError, setEmailError] = useState(false);
+  const [textError, setTextError] = useState(false);


and about object, if i change only name, it will rerender all object. to my mind, its better to use 3 useStates.

FYI: You probably meant that we will change the entire object stored in the state, but that's not a problem. we do not render an object. we render the component (jsx that is written below), and re-create every variable (including) functions during the render. Having different states would be useful if you use memorization hooks with different variables in the dependency arrays.

… newCommentForm

volodymyr-soltys97

Let's improve your code

volodymyr-soltys97 · 2023-08-04T11:43:04Z

src/components/UserSelector.tsx

+            {users.map(user => (
+              <a


Use a destructuring for user

volodymyr-soltys97

Well done 🔥

task done

34127ba

IvanFesenko suggested changes Aug 3, 2023

View reviewed changes

fixed handleError, trim(), added key

2fa0152

pos1bl requested a review from IvanFesenko August 3, 2023 13:51

TarasHoliuk suggested changes Aug 3, 2023

View reviewed changes

moved some states and logic to other components and improve states in…

6c04df2

… newCommentForm

pos1bl requested a review from TarasHoliuk August 4, 2023 10:39

volodymyr-soltys97 suggested changes Aug 4, 2023

View reviewed changes

src/components/UserSelector.tsx Outdated

Comment on lines 53 to 54

{users.map(user => (

<a

Copy link

volodymyr-soltys97 Aug 4, 2023

Choose a reason for hiding this comment

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

Use a destructuring for user

destructured user

7a7e2b4

pos1bl requested a review from volodymyr-soltys97 August 5, 2023 10:42

volodymyr-soltys97 approved these changes Aug 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

task done #877

task done #877

pos1bl commented Aug 3, 2023

IvanFesenko left a comment

IvanFesenko Aug 3, 2023

IvanFesenko Aug 3, 2023

pos1bl Aug 3, 2023

pos1bl Aug 3, 2023

TarasHoliuk Aug 3, 2023

IvanFesenko Aug 3, 2023

IvanFesenko Aug 3, 2023

IvanFesenko Aug 3, 2023

IvanFesenko Aug 3, 2023

IvanFesenko Aug 3, 2023

TarasHoliuk Aug 3, 2023

TarasHoliuk Aug 3, 2023

TarasHoliuk Aug 3, 2023

volodymyr-soltys97 left a comment

volodymyr-soltys97 Aug 4, 2023

volodymyr-soltys97 left a comment

	onChange={(event) => handleNameChange(event.target.value)}
	onChange={handleNameChange}

	const nameValidate = !name.trim;
	const isNameValide = !!name.trim;

task done #877

Are you sure you want to change the base?

task done #877

Conversation

pos1bl commented Aug 3, 2023

IvanFesenko 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

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

volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyr-soltys97 left a comment

Choose a reason for hiding this comment