Solution #889

Ssviatt · 2023-08-11T11:40:15Z

TarasHoliuk

GJ! Keep going! The main point: you should fix app behavior in some cases (read requirements, check tests)

src/components/UserSelector.tsx

TarasHoliuk · 2023-08-11T16:38:27Z

src/components/UserSelector.tsx

+import { getUsers } from '../api/ApiMethods';
+
+type Props = {
+  setSelectUser: (v: User) => void,


v is a bad argument name

TarasHoliuk · 2023-08-11T16:39:16Z

src/components/NewCommentForm.tsx

-            className="input is-danger"
+            className={classNames('input', { 'is-danger': checkEmailName })}
+            value={formInputs.email}
+            onChange={(e) => handleChange(e)}


Suggested change

onChange={(e) => handleChange(e)}

onChange={handleChange}

you don't need wrapper function in such cases

TarasHoliuk · 2023-08-11T16:39:45Z

src/components/NewCommentForm.tsx

-          <button type="submit" className="button is-link is-loading">
+          <button
+            type="submit"
+            onClick={() => setTuched(true)}


Suggested change

onClick={() => setTuched(true)}

onClick={() => setTouched(true)}

TarasHoliuk · 2023-08-11T16:40:23Z

src/components/NewCommentForm.tsx

+    }));
+  };
+
+  const handleClear = () => {


you should also clear errors (one of tests fails because of this):

TarasHoliuk · 2023-08-11T16:41:59Z

src/App.tsx

 export const App: React.FC = () => {
+  const [selectUser, setSelectUser] = useState<User | null>(null);


Suggested change

const [selectUser, setSelectUser] = useState<User | null>(null);

const [selectedUser, setSelectedUser] = useState<User | null>(null);

TarasHoliuk · 2023-08-11T16:42:37Z

src/App.tsx

+        .catch(() => setHasError(true))
+        .finally(() => {
+          setLoading(false);
+          setHasSelect(true);


you may just check selectedUser variable instead of creating an additional state

TarasHoliuk · 2023-08-11T16:43:19Z

src/App.tsx

+  const [hasSelect, setHasSelect] = useState(false);
+  const [hasError, setHasError] = useState(false);
+  const [openPost, setOpenPost] = useState<Post | null>(null);
+  const [openForm, setOpenForm] = useState(false);


Suggested change

const [openForm, setOpenForm] = useState(false);

const [isFormOpen, setIsFormOpen] = useState(false);

fix everywhere: https://dev.to/michi/tips-on-naming-boolean-variables-cleaner-code-35ig

TarasHoliuk · 2023-08-11T16:46:05Z

src/components/PostDetails.tsx

-              </button>
-            </div>
+      getComment(openPost.id)
+        .then(PostComments => setPostComments(PostComments))


Suggested change

.then(PostComments => setPostComments(PostComments))

.then(costComments => setPostComments(costComments))

camelCase for variable names (that's not a class, React component or type, etc)

TarasHoliuk · 2023-08-11T16:47:36Z

src/components/PostDetails.tsx

+                ) : (
+                  <>
+                    <p className="title is-4">Comments:</p>
+                    {postComments.map((com) => (


Suggested change

{postComments.map((com) => (

{postComments.map((comment) => (

do not use short forms for variable names

DarkMistyRoom

fix these tests, please

DarkMistyRoom · 2023-08-14T07:42:36Z

src/components/NewCommentForm.tsx

+    body: '',
+  });
+  const [loading, setLoading] = useState(false);
+  const [tuched, setTouched] = useState(false);


Suggested change

const [tuched, setTouched] = useState(false);

const [touched, setTouched] = useState(false);

src/components/PostDetails.tsx

etojeDenys · 2023-08-20T07:38:33Z

src/App.tsx

+      setHasError(false);
+
+      getPosts(selectedUser.id)
+        .then(posts => setUserPosts(posts))


Suggested change

.then(posts => setUserPosts(posts))

.then(setUserPosts)

we can simplify this logic

etojeDenys · 2023-08-20T07:41:39Z

src/components/PostDetails.tsx

 import { NewCommentForm } from './NewCommentForm';

-export const PostDetails: React.FC = () => {
+type Props = {
+  isPostOpen: Post | null,


Suggested change

isPostOpen: Post | null,

openedPost: Post | null,

we usually name var with is if it's boolean

how can it be null if we check if this prop exists

etojeDenys

looks strange with the name field

Screen.Recording.2023-08-21.at.21.21.22.mov

etojeDenys · 2023-08-21T18:18:52Z

src/components/UserSelector.tsx

        >
-          <span>Choose a user</span>
+          <span>{selectedUser ? selectedUser.name : 'Choose a user'}</span>


Suggested change

<span>{selectedUser ? selectedUser.name : 'Choose a user'}</span>

<span>{selectedUser?.name || 'Choose a user'}</span>

etojeDenys · 2023-08-21T18:19:12Z

src/components/UserSelector.tsx

+  };
+
+  useEffect(() => {
+    getUsers().then(users => setAllUsers(users));


Suggested change

getUsers().then(users => setAllUsers(users));

getUsers().then(setAllUsers);

we can simplify it

etojeDenys · 2023-08-21T18:20:13Z

src/components/UserSelector.tsx

+  selectedUser,
+  setOpenedPost,
+}) => {
+  const [allUsers, setAllUsers] = useState<null | User[]>(null);


Suggested change

const [allUsers, setAllUsers] = useState<null | User[]>(null);

const [allUsers, setAllUsers] = useState<User[]>([]);

we can just use it as empty array instead of the null

etojeDenys

lgtm

Solution

88b2fc6

TarasHoliuk suggested changes Aug 11, 2023

View reviewed changes

Ssviatt added 2 commits August 13, 2023 22:10

Solution

fb8a0f3

Solution

66dab50

Ssviatt requested a review from TarasHoliuk August 13, 2023 19:42

DarkMistyRoom suggested changes Aug 14, 2023

View reviewed changes

Solution

0ba3c86

Ssviatt requested a review from DarkMistyRoom August 19, 2023 12:13

etojeDenys suggested changes Aug 20, 2023

View reviewed changes

Solution

534feb8

Ssviatt requested a review from etojeDenys August 21, 2023 18:11

etojeDenys suggested changes Aug 21, 2023

View reviewed changes

Solution

8ac87bd

Ssviatt requested a review from etojeDenys August 21, 2023 20:44

etojeDenys approved these changes Aug 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solution #889

Solution #889

Ssviatt commented Aug 11, 2023

TarasHoliuk left a comment

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

TarasHoliuk Aug 11, 2023

DarkMistyRoom left a comment

DarkMistyRoom Aug 14, 2023

etojeDenys Aug 20, 2023

etojeDenys Aug 20, 2023

etojeDenys Aug 20, 2023

etojeDenys left a comment

etojeDenys Aug 21, 2023

etojeDenys Aug 21, 2023

etojeDenys Aug 21, 2023

etojeDenys left a comment

	onClick={() => setTuched(true)}
	onClick={() => setTouched(true)}

		export const App: React.FC = () => {
		const [selectUser, setSelectUser] = useState<User \| null>(null);

	const [selectUser, setSelectUser] = useState<User \| null>(null);
	const [selectedUser, setSelectedUser] = useState<User \| null>(null);

	const [openForm, setOpenForm] = useState(false);
	const [isFormOpen, setIsFormOpen] = useState(false);

	.then(PostComments => setPostComments(PostComments))
	.then(costComments => setPostComments(costComments))

	{postComments.map((com) => (
	{postComments.map((comment) => (

	const [tuched, setTouched] = useState(false);
	const [touched, setTouched] = useState(false);

	<span>{selectedUser ? selectedUser.name : 'Choose a user'}</span>
	<span>{selectedUser?.name \|\| 'Choose a user'}</span>

	getUsers().then(users => setAllUsers(users));
	getUsers().then(setAllUsers);

	const [allUsers, setAllUsers] = useState<null \| User[]>(null);
	const [allUsers, setAllUsers] = useState<User[]>([]);

Solution #889

Are you sure you want to change the base?

Solution #889

Conversation

Ssviatt commented Aug 11, 2023

TarasHoliuk 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

DarkMistyRoom 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

etojeDenys 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

etojeDenys left a comment

Choose a reason for hiding this comment