Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add a close button to comment input #3897

Merged
merged 19 commits into from
Dec 6, 2024

Conversation

tu2463
Copy link
Contributor

@tu2463 tu2463 commented Dec 2, 2024

Changes

This PR resolves #1606 dailydotdev/daily#1606

  • Added a CloseButton component to the MarkdownInput component, so that we can see a "X" on the top-right corner of the comment box.
image
  • Let CommentInputOrModal passes onClose to CommentMarkdownInput as a prop "onRequestClose". This function is eventually passed to MarkdownInput, so that clicking the close button can cancel the comment.

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

  1. Go to a post
  2. Click on comment button
  3. Should see the comment box with a close button. Clicking the close button will cancel the comment.

-->

Copy link

vercel bot commented Dec 2, 2024

@tu2463 is attempting to deploy a commit to the Daily Dev Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Great work! Thank you for your effort 🙏

After this, there should be two components we'll need to update to make this work on desktop. The MainComment and SubComment components contain a conditional render on the input field we display, we should pass the onRequestClose value to be a function that clears the state that manages the input itself.

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2024

CLA assistant check
All committers have signed the CLA.

@jullia02
Copy link

jullia02 commented Dec 4, 2024

After this, there should be two components we'll need to update to make this work on desktop. The MainComment and SubComment components contain a conditional render on the input field we display, we should pass the onRequestClose value to be a function that clears the state that manages the input itself.

Thank you for your feedback! We've updated both components to pass the onRequestClose prop, so it clears the input state as requested. We've also reviewed your inline comments and incorporated the changes.

Please let us know if there's anything else you'd like adjusted. 😄

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Amazing work by both you 👏 👏 👏

Just the suggested commits, then we should be good to go! 🚀 :shipit:

Co-authored-by: Lee Hansel Solevilla <[email protected]>
Copy link

vercel bot commented Dec 4, 2024

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

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Dec 6, 2024 5:29am

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Just to avoid any conflict. I shall test this thoroughly tomorrow and see if we can merge 🥳

@sshanzel sshanzel merged commit f8c7648 into dailydotdev:main Dec 6, 2024
8 of 9 checks passed
@sshanzel
Copy link
Member

sshanzel commented Dec 6, 2024

@tu2463 @jullia02 thank you both for your efforts. We've now merged your work, and should be seen within the app in a few minutes 🥳 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants