Add Edit and Reply UI to DiscussionsCard #2375

HS-90 · 2022-09-30T20:37:49Z

Description: relates to #2252

This PR adds the Edit and Reply UI for the DiscussionsCard. Please see screenshots below:

User clicks edit:

User clicks reply:

User clicks both reply and edit:

Test component here: https://deploy-preview-2375--youthful-thompson-448332.netlify.app/?path=/story/components-discussionscard--discussions-card-main

vercel · 2022-09-30T20:37:53Z

@HS-90 is attempting to deploy a commit to the c0d3-prod Team on Vercel.

A member of the Team first needs to authorize it.

vercel · 2022-09-30T20:44:51Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
c0d3-app	✅ Ready (Inspect)	Visit Preview	Sep 30, 2022 at 8:49PM (UTC)

codecov · 2022-09-30T20:46:07Z

Codecov Report

Merging #2375 (342b3f3) into master (14e6751) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2375   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          184       184           
  Lines         3270      3294   +24     
  Branches       861       868    +7     
=========================================
+ Hits          3270      3294   +24

Impacted Files	Coverage Δ
components/DiscussionsCard/DiscussionsCard.tsx	`100.00% <100.00%> (ø)`

bryanjenningz · 2022-10-01T03:56:50Z

components/DiscussionsCard/DiscussionsCard.tsx

+                <CommentIcon /> Reply
+              </div>
+              <div className={styles.edit_delete}>
+                <div onClick={editClick} data-testid="pencil">


You might want to use a button instead of a div for clickable elements.

bryanjenningz · 2022-10-01T03:57:24Z

components/DiscussionsCard/DiscussionsCard.tsx

+          <div>
+            <img src={userPic} className={styles.user_pic} />
+            <span className={styles.username}>{username}</span>
+            <span> </span>


What's this span for?

I used it to add a little space between elements.

If you are adding span for the effect setting a specific amount of space between elements, then you need to do that with CSS instead instead of using span elements with whitespace values. It's more appropriate to do it with CSS since CSS is meant to be used for styling. Using CSS will make the styling choice more explicit as well, thus making the code more simple and understandable. From my side, I was confused about what this span was being used for because it wasn't very clear.

i ended up getting rid of it in my local, and didn't notice any difference visually actually. So it's removed now.

bryanjenningz · 2022-10-01T03:58:56Z

components/DiscussionsCard/DiscussionsCard.tsx

+          }
+        >
+          <div>
+            <img src={userPic} className={styles.user_pic} />


You might want to add alt="" here. (https://davidwalsh.name/accessibility-tip-empty-alt-attributes)

We should use the <Image> component by next.js

We should use the <Image> component by next.js

When I use the Image component from next, for some reason it did not incorporate any of the styling, so I kept it as <img> for now.

We should use the <Image> component by next.js

When I use the Image component from next, for some reason it did not incorporate any of the styling, so I kept it as <img> for now.

Does these help?

https://nextjs.org/docs/api-reference/next/image#style

https://www.peterlunch.com/snippets/next-image-styling

bryanjenningz · 2022-10-01T04:11:18Z

components/DiscussionsCard/DiscussionsCard.tsx

+  content: string
+  likes?: number
+  dislikes?: number
+  replies?: Array<DiscussionReply>


Why are the likes, dislikes, and replies optional?

We decided we aren't implementing likes in this version, but i just kept it in there as optional for now in case we change our minds or decide to put them in our backend data.

For replies, not every post will have replies, so I thought optional was the better way.

It's not a good idea to add these like/dislike props now.

This PR is about adding edit/reply, not about adding likes/dislikes -- adding likes/dislikes seems specific
enough of a task that it should be its own PR. Being more organized with PRs will also make it easier for issue triaging when needed.

I also have concerns about not fully completing the likes/dislikes feature in this component because I don't see a fleshed out plan or POC for the feature. Since its not completed in this PR(and shouldn't) and there's no fleshed out plan or POC, it makes me concerned that these props won't even be used -- when fully implementing the likes/dislikes feature, it could be decided that the like/dislike props should instead be replaced with prop X, which would mean the like/dislike props are potential tech debt

Similarly, why is the replies prop added? It doesn't seem to be used yet either

Please first finalize on the design and architecture of how to display replies to replies(discussion replies) before adding all this code related to it. If this architecture isn't finalized, then it doesn't make sense to add all this logic relating to it because it'll just be potential tech debt

flacial · 2022-10-01T06:29:52Z

components/DiscussionsCard/discussionsCard.module.scss

+  margin-right: 5px;
+  font-size: 14px;
+  border: 0px;
+  padding: 5px 6px 5px 6px;


A shorthand for this is padding: 5px 6px

flacial · 2022-10-01T06:31:08Z

components/DiscussionsCard/discussionsCard.module.scss

+  border: 0px;
+  padding: 5px 6px 5px 6px;
+  background-color: transparent;
+  &:hover,


It's nice to have a transition.

transition: background-color 0.3s ease

flacial · 2022-10-01T06:33:03Z

components/DiscussionsCard/discussionsCard.test.js

+
+    await userEvent.click(getByTestId('pencil'))
+
+    waitFor(() => {


I don't think you would need waitFor if you used findBy* instead of getBy*.

flacial · 2022-10-01T06:39:18Z

Margin between mdInput and DiscussionCard would be nice.

anthonykhoa · 2022-10-01T21:06:51Z

components/DiscussionsCard/DiscussionsCard.tsx

        </div>
      </div>
+      {replyMode ? replyBox : <></>}


We can simplify this to {replyMode && replyBox}

anthonykhoa · 2022-10-01T21:07:50Z

components/DiscussionsCard/DiscussionsCard.tsx

+        )}
+
+        <div className={styles.card_buttons}>
+          {editMode ? (


{editMode && (
...
)}

anthonykhoa · 2022-10-01T21:15:18Z

components/DiscussionsCard/DiscussionsCard.tsx

+          <div className={styles.card_content}>{content}</div>
+        )}
+
+        <div className={styles.card_buttons}>


Does this div do anything if editMode is false?

If it doesnt do anything, we can simplify logic by removing an instance of checking for editMode

{editMode ? editBox : ( <> <div className={styles.card_content}>{content}</div> <div className={styles.card_buttons}>...</div> </> )}

anthonykhoa · 2022-10-01T21:26:34Z

components/DiscussionsCard/DiscussionsCard.tsx

+  const [replyMode, setReplyMode] = useState(false)
+  const [editMode, setEditMode] = useState(false)
+  const [replyText, setReplyText] = useState('')
+  const [editText, setEditText] = useState('')


Whats the difference between replyText and editText? Why are both kinds of states needed?

replyText keeps track of the user inputting the reply, and editText keep track of inputs while a post is being edited by user. Here's a screenshot of both of them being edited at the same time:

anthonykhoa · 2022-10-02T02:58:49Z

Can we focus this PR on just implementing either the editing feature or replying feature? You finished half of the editing feature(edit/save), and half of the replying feature(replying and displays replies) in the same PR. The edit is useless without the save working, likewise the replying is useless without the ability to see the replies. Its disorganized that you're implementing a bit of each feature in this PR, just focus this PR on one feature

HS-90 · 2022-10-02T14:00:43Z

Can we focus this PR on just implementing either the editing feature or replying feature? You finished half of the editing feature(edit/save), and half of the replying feature(replying and displays replies) in the same PR. The edit is useless without the save working, likewise the replying is useless without the ability to see the replies. Its disorganized that you're implementing a bit of each feature in this PR, just focus this PR on one feature

So should I prioritize some of the backend things first? That would be required for the save and reply feature to work as intended in production. Or should I create dummy objects for now to just save things on the frontend so we at least know what they look like once a user clicks 'save'(edit) or 'send'(replies).

anthonykhoa · 2022-10-02T17:08:50Z

So should I prioritize some of the backend things first? That would be required for the save and reply feature to work as intended in production. Or should I create dummy objects for now to just save things on the frontend so we at least know what they look like once a user clicks 'save'(edit) or 'send'(replies).

Working on back-end or front-end first is fine, at work it happens in parallel sometimes -- just be sure there is coordination between front-end changes and back-end changes. Frontend and backend should be following the same e2e architecture.

Also, is there some design doc or POC in place? If there is none, then I have concerns that you are breaking up this feature of editing&saving into multiple PRs without a finalized architecture in place. If the architecture isn't finalized, then all the code in your PR is just going to be potential tech debt since everything will be subject to change.

HS-90 · 2022-10-03T14:53:34Z

Also, is there some design doc or POC in place? If there is none, then I have concerns that you are breaking up this feature of editing&saving into multiple PRs without a finalized architecture in place. If the architecture isn't finalized, then all the code in your PR is just going to be potential tech debt since everything will be subject to change.

There is none right now. I can get one started though so we can get a better idea of the architecture.

anthonykhoa · 2022-10-03T15:23:46Z

Also, is there some design doc or POC in place? If there is none, then I have concerns that you are breaking up this feature of editing&saving into multiple PRs without a finalized architecture in place. If the architecture isn't finalized, then all the code in your PR is just going to be potential tech debt since everything will be subject to change.

There is none right now. I can get one started though so we can get a better idea of the architecture.

If the feature isn't complex enough you don't have to make a design doc, but just make sure you're organized in your PRs. Instead of implementing a bit of both edit and reply feature, it would be better to focus this PR on just one thing -- for example if you wanted to focus on just editing, then you should have made code changes in this PR to make save work instead of some code for replying. And then you could have another PR that implements backend changes needed. If the changes needed aren't complex enough, you can have the backend PR and front-end PR up at the same time with no need for design doc. If the feature requires complex changes though, then yeah you need to make a design doc

HS-90 · 2022-10-03T17:20:16Z

Also, is there some design doc or POC in place? If there is none, then I have concerns that you are breaking up this feature of editing&saving into multiple PRs without a finalized architecture in place. If the architecture isn't finalized, then all the code in your PR is just going to be potential tech debt since everything will be subject to change.

There is none right now. I can get one started though so we can get a better idea of the architecture.

If the feature isn't complex enough you don't have to make a design doc, but just make sure you're organized in your PRs. Instead of implementing a bit of both edit and reply feature, it would be better to focus this PR on just one thing -- for example if you wanted to focus on just editing, then you should have made code changes in this PR to make save work instead of some code for replying. And then you could have another PR that implements backend changes needed. If the changes needed aren't complex enough, you can have the backend PR and front-end PR up at the same time with no need for design doc. If the feature requires complex changes though, then yeah you need to make a design doc

After reading some of your comments, I think it would be a good idea to create a design doc for this project. I think having one will provide a better sense of direction for myself and the team while working on this.

HS-90 · 2022-10-04T19:19:57Z

#2400

Here is an initial draft of the design doc. Welcome to any comments or suggestions for improvement!

flacial · 2023-04-04T12:45:00Z

Closing due to inactivity.

''

bfcf7e9

''

d446cdd

vercel bot deployed to Preview September 30, 2022 20:49 View deployment

bryanjenningz reviewed Oct 1, 2022

View reviewed changes

flacial reviewed Oct 1, 2022

View reviewed changes

anthonykhoa reviewed Oct 1, 2022

View reviewed changes

components/DiscussionsCard/DiscussionsCard.tsx

)}

<div className={styles.card_buttons}>

{editMode ? (

Copy link

Collaborator

anthonykhoa Oct 1, 2022

Choose a reason for hiding this comment

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

{editMode && (
...
)}

anthonykhoa reviewed Oct 1, 2022

View reviewed changes

HS-90 added 2 commits October 1, 2022 18:14

Merge branch 'garageScript:master' into exerciseList

9fba755

Merge branch 'master' into exerciseList

342b3f3

HS-90 added the dont-merge Not ready for merge label Oct 6, 2022

flacial marked this pull request as draft December 16, 2022 11:53

flacial closed this Apr 4, 2023

Add Edit and Reply UI to DiscussionsCard #2375

Add Edit and Reply UI to DiscussionsCard #2375

Conversation

HS-90 commented Sep 30, 2022 • edited Loading

vercel bot commented Sep 30, 2022

vercel bot commented Sep 30, 2022 • edited Loading

codecov bot commented Sep 30, 2022 • edited Loading

Codecov Report

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

HS-90 Oct 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HS-90 Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

anthonykhoa Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa Oct 1, 2022 • 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

flacial commented Oct 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa commented Oct 2, 2022 • edited Loading

HS-90 commented Oct 2, 2022 • edited Loading

anthonykhoa commented Oct 2, 2022 • edited Loading

HS-90 commented Oct 3, 2022

anthonykhoa commented Oct 3, 2022

HS-90 commented Oct 3, 2022 • edited Loading

HS-90 commented Oct 4, 2022

flacial commented Apr 4, 2023

HS-90 commented Sep 30, 2022 •

edited

Loading

vercel bot commented Sep 30, 2022 •

edited

Loading

codecov bot commented Sep 30, 2022 •

edited

Loading

HS-90 Oct 2, 2022 •

edited

Loading

HS-90 Oct 1, 2022 •

edited

Loading

anthonykhoa Oct 1, 2022 •

edited

Loading

anthonykhoa Oct 1, 2022 •

edited

Loading

anthonykhoa Oct 1, 2022 •

edited

Loading

anthonykhoa commented Oct 2, 2022 •

edited

Loading

HS-90 commented Oct 2, 2022 •

edited

Loading

anthonykhoa commented Oct 2, 2022 •

edited

Loading

HS-90 commented Oct 3, 2022 •

edited

Loading