implement functionality for dynamic list of posts #921

voievodik · 2023-09-24T16:35:25Z

BudnikOleksii

Good work 👍
Let's improve your solution slightly

BudnikOleksii · 2023-09-25T10:02:45Z

src/App.tsx

+                    {user && posts.length === 0 ? (
+                      <div
+                        className="notification is-warning"
+                        data-cy="NoPostsYet"
+                      >
+                        No posts yet
+                      </div>
+                    ) : null}


Suggested change

{user && posts.length === 0 ? (

<div

className="notification is-warning"

data-cy="NoPostsYet"

>

No posts yet

</div>

) : null}

{user && posts.length === 0 && (

<div

className="notification is-warning"

data-cy="NoPostsYet"

>

No posts yet

</div>

)}

You can use just and operator in this case

BudnikOleksii · 2023-09-25T10:02:56Z

src/App.tsx

+                    {posts.length > 0 && user ? (
+                      <PostsList />
+                    ) : null}


BudnikOleksii · 2023-09-25T10:04:50Z

src/api/comments.ts

+// export const createComment = (data: Comment) => {
+//   return client.post<Comment>('/comments', data);
+// };
+
+export const createComment = ({
+  name, email, body, postId,
+}: Omit<Comment, 'id'>) => {
+  return client.post<Comment>('/comments', {
+    name, email, body, postId,
+  });
+};


Suggested change

// export const createComment = (data: Comment) => {

// return client.post<Comment>('/comments', data);

// };

export const createComment = ({

name, email, body, postId,

}: Omit<Comment, 'id'>) => {

return client.post<Comment>('/comments', {

name, email, body, postId,

});

};

export const createComment = (comment: Omit<Comment, 'id'>) => {

return client.post<Comment>('/comments', comment);

};

Don't leave comments, and you don't need destructing in this case

If I use this shortcut, the comment will not be created on the server and I get an error

IMO - You are not sure what data would actually be passed to this function - so explicitly sending only the required data is OK

BudnikOleksii · 2023-09-25T10:06:16Z

src/components/NewCommentForm.tsx

+  const handleChangeForm = (
+    event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>,
+  ) => {
+    event.preventDefault();
+
+    setFormData(prev => ({ ...prev, [event.target.name]: event.target.value }));
+  };


Suggested change

const handleChangeForm = (

event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>,

) => {

event.preventDefault();

setFormData(prev => ({ ...prev, [event.target.name]: event.target.value }));

};

const handleChangeForm = (

event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>,

) => {

const { name, value } = event.target;

setFormData(prev => ({ ...prev, [name]: value }));

};

Why do you do event.preventDefault();?

BudnikOleksii · 2023-09-25T10:07:17Z

src/components/NewCommentForm.tsx

            Add
          </button>
        </div>
+        {/* is-loading */}


Don't leave comments

BudnikOleksii · 2023-09-25T10:08:33Z

src/components/PostDetails.tsx


 export const PostDetails: React.FC = () => {
+  const { post } = useContext(PostsContext);
+  const { comments, setComments } = useContext(CommentsContext);


You can create a hook for your contexts and in this case, you don't need to pass context every time:

const useComments = () => useContext(CommentsContext);

BudnikOleksii · 2023-09-25T10:09:15Z

src/components/PostDetails.tsx

+      .then(() => {
+        setComments(prevComments => {
+          return prevComments.filter(({ id }) => id !== commentId);
+        });
+      });


Add catch block

BudnikOleksii · 2023-09-25T10:10:39Z

src/components/PostDetails.tsx

+              <article className="message is-small" data-cy="Comment" key={id}>
+                <div className="message-header">
+                  <a href={`mailto:${email}`} data-cy="CommentAuthor">
+                    {name}
+                  </a>
+                  <button
+                    data-cy="CommentDelete"
+                    type="button"
+                    className="delete is-small"
+                    aria-label="delete"
+                    onClick={() => handleDeleteComment(id)}
+                  >
+                    delete button
+                  </button>
+                </div>


Create a Comment component

BudnikOleksii · 2023-09-25T10:14:36Z

src/components/PostsList.tsx

-          </td>
-        </tr>
+            return (
+              <tr data-cy="Post" key={id}>


Create Post component

DenisChakhalian

good job!

DenisChakhalian · 2023-09-28T13:26:28Z

src/components/CurrentPost.tsx

+          data-cy="PostButton"
+          className={classnames('button is-link', {
+            'is-light': post?.id !== id,
+          })}


that is better

Suggested change

data-cy="PostButton"

className={classnames('button is-link', {

'is-light': post?.id !== id,

})}

data-cy="PostButton"

className={classnames('button', 'is-link', {

'is-light': post?.id !== id,

})}

Good point.

DenisChakhalian · 2023-09-28T13:28:06Z

src/components/NewCommentForm.tsx

+          <button type="submit" className="button is-link ">
            Add
          </button>


Suggested change

<button type="submit" className="button is-link ">

Add

</button>

<button type="submit" className="button is-link">

Add

</button>

Good point.

DenisChakhalian · 2023-09-28T13:32:22Z

src/index.tsx

+  <UsersProvider>
+    <PostsProvider>
+      <CommentsProvider>
+        <ErrorProvider>
+          <App />
+        </ErrorProvider>
+      </CommentsProvider>
+    </PostsProvider>
+  </UsersProvider>,


you can create a component that will wrap the App with all providers

That makes sense - you should do it next time

roman-mirzoian

Good job, but let's polish your code.

roman-mirzoian · 2023-09-28T21:45:31Z

src/components/CurrentPost.tsx

+          data-cy="PostButton"
+          className={classnames('button is-link', {
+            'is-light': post?.id !== id,
+          })}


Good point.

roman-mirzoian · 2023-09-28T21:46:22Z

src/components/NewCommentForm.tsx

+          <button type="submit" className="button is-link ">
            Add
          </button>


Good point.

src/components/PostDetails.tsx

Co-authored-by: Roman Mirzoian <[email protected]>

…list-of-posts into develop

alexander-ignatow

Overall - good job! 👍

You are good at writing components - but there are some issues with overall architecture and components responsibility

Let's move on with this solution, but here are the items to keep in mind for the future:

Mind the naming - all component and function names should be as descriptive as possible. For instance CurrentPost suggests that the component renders some specific post that the user is working on. But in your app, it is just an element of the list, so Post or even PostItem fits better. Also - mind the grammar ;)
Try organizing components in the folders: for instance, you can create a subfolder 'components/PostList' and move all post-related components there (PostList, CurrentPost, CurrentComment, etc.)
Don't overuse contexts - while this is a powerful tool, it has "dark sides". One of them is making the code more "coupled". Try making components as "dumb" and isolated as possible - it improves testability, makes supporting the code easier, etc. For instance - I would remove the PostsContext call from the PostList and simply pass posts as an external property.

alexander-ignatow · 2023-09-29T09:27:33Z

src/context/UsersContext.tsx

+  user: null,
+  setUser: () => {},


calling this currentUser or selectedUser would be a bit more readable

alexander-ignatow · 2023-09-29T09:28:16Z

src/context/PostsContext.tsx

+  post: null,
+  setPost: () => {},


Same as with user context - better call it selectedPost, etc

alexander-ignatow · 2023-09-29T09:29:06Z

src/context/ErrorContext.tsx

+
+interface ErrorContextType {
+  isErrorHappen: boolean;
+  setIsErrorHappen: React.Dispatch<React.SetStateAction<boolean>>;


Grammar issue :)

Better hasErrorHappened or just hasError, etc

alexander-ignatow · 2023-09-29T09:34:02Z

src/context/CommentsContext.tsx

From how I see your scenario - it makes sense to merge CommentsContext and PostsContext

The thing is, that you have currentPost in the PostsContext but comments do not make any sense without the Post.

The main idea is minimizing the number of contexts, as managing large number of contexts is not the best experience

alexander-ignatow · 2023-09-29T09:38:58Z

src/App.tsx

+                    {isPostsExist && (
+                      <div
+                        className="notification is-warning"
+                        data-cy="NoPostsYet"
+                      >
+                        No posts yet
+                      </div>
+                    )}

-                <div
-                  className="notification is-danger"
-                  data-cy="PostsLoadingError"
-                >
-                  Something went wrong!
-                </div>
+                    {isPostListShown && (
+                      <PostsList />
+                    )}
+                  </>


You are reading the posts in the App only to show this section

Possibly it makes sense to move the block "No posts yet" to the PostsList

alexander-ignatow · 2023-09-29T09:40:04Z

src/components/CurrentComment.tsx

Naming issue: just Comment looks better

alexander-ignatow · 2023-09-29T09:40:22Z

src/components/CurrentPost.tsx

Naming issue: just Post or PostItem looks better

implement functionality for dynamic list of posts

480bcdb

BudnikOleksii suggested changes Sep 25, 2023

View reviewed changes

provided fixed according to review

5183411

voievodik requested a review from BudnikOleksii September 25, 2023 19:13

created hook for contexts

5de30e5

DenisChakhalian approved these changes Sep 28, 2023

View reviewed changes

roman-mirzoian suggested changes Sep 28, 2023

View reviewed changes

voievodik and others added 6 commits September 29, 2023 09:46

update the condition

94f5ee1

Co-authored-by: Roman Mirzoian <[email protected]>

update the condition

b1d8b2e

Co-authored-by: Roman Mirzoian <[email protected]>

provided fixes according to review

fa6477a

added changes

f09ba9a

update the condition

b3c6163

Co-authored-by: Roman Mirzoian <[email protected]>

Merge branch 'develop' of https://github.com/voievodik/react_dynamic-…

ad4f041

…list-of-posts into develop

voievodik requested a review from roman-mirzoian September 29, 2023 07:28

alexander-ignatow approved these changes Sep 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement functionality for dynamic list of posts #921

implement functionality for dynamic list of posts #921

voievodik commented Sep 24, 2023 •

edited

Loading

BudnikOleksii left a comment

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

voievodik Sep 25, 2023

alexander-ignatow Sep 29, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

BudnikOleksii Sep 25, 2023

DenisChakhalian left a comment

DenisChakhalian Sep 28, 2023

roman-mirzoian Sep 28, 2023

DenisChakhalian Sep 28, 2023

roman-mirzoian Sep 28, 2023

DenisChakhalian Sep 28, 2023

alexander-ignatow Sep 29, 2023

roman-mirzoian left a comment

roman-mirzoian Sep 28, 2023

roman-mirzoian Sep 28, 2023

alexander-ignatow left a comment

alexander-ignatow Sep 29, 2023

alexander-ignatow Sep 29, 2023

alexander-ignatow Sep 29, 2023

alexander-ignatow Sep 29, 2023

alexander-ignatow Sep 29, 2023

alexander-ignatow Sep 29, 2023

alexander-ignatow Sep 29, 2023

implement functionality for dynamic list of posts #921

Are you sure you want to change the base?

implement functionality for dynamic list of posts #921

Conversation

voievodik commented Sep 24, 2023 • edited Loading

BudnikOleksii 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

DenisChakhalian 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

roman-mirzoian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-ignatow 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

voievodik commented Sep 24, 2023 •

edited

Loading