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

feat(retro): adds the possibility to add coments #122

Closed
wants to merge 8 commits into from

Conversation

sillydomnom
Copy link
Contributor

@sillydomnom sillydomnom commented Nov 23, 2021

Adding comments to cards would be possible with these changes

Solves: #60

Functionality

  • After clicking on the Comment Button, Comments will be accessible in a dialog.
  • On the first part you can see the current card on the second part you can write and send your comments and on the last part you can see all comments
  • Every comment can be edited and delted by the moderator and by the creator
  • Changed the Discussed Icon to be more clear about what what is

Boundaries

  • The check if the user is able to edit a comment is made client side, if the user really want to edit a comment from another user it is possible

Possibilities

  • In the Future the Process of creating Cards and Comments could be made easier when letting the User choose a name when they is joining, so there would be more space when editing cards

Current View

image

Copy link
Collaborator

@yduman yduman left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your PR! I noticed some things during testing that I would like to share with you.

When you open the comments dialog:

  • The first card that you display, doesn't handle line breaks when having for example a card with multi-line content. In my opinion it's not necessary to display the card in the dialog again. I understand your intention behind it and it makes sense when having short and concise cards, but I saw how teams are sometimes writing very long text inside it which would bloat the dialog. So maybe we should leave it out there.
  • Speaking of multi-line, the text field where the user can enter a comment cannot handle multi-line as well. The resulting comment is displayed without line breaks.
  • I was thinking a bit more about how we handle author names currently. We can keep the current behavior as it is, since it's the same when creating a card, but I thought that it might make sense to have a new feature, where every user provides a name before entering the session. Kind of like it is handled currently in the planning poker section. That way we could remove the need to always pre-fill or even have the ability to enter the author name for everything.

frontend/src/context/DialogContext.tsx Outdated Show resolved Hide resolved
* fixes(retro): card is now not shown in
comment dialog

* fix(retro): code cleanup
Copy link
Collaborator

@yduman yduman left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looking good so far! There are just some small UI/UX things that would be nice.

  1. If it's possible, remove the cancel button on the bottom of the dialog and instead provide an X icon button on the top right in order to close the dialog.
  2. On the top of the dialog where the user can enter a comment, extend the text fields to full width and move the send button after the content text field aligned to the end of the row. Similar to MS Teams for example. You could maybe use a button with icon and label. The label can have the value "Send". More here: https://mui.com/components/buttons/#buttons-with-icons-and-label
  3. I think it's better if we move the edit and delete button on the same row as the name of the author and align it to the end of the row. Same for the "save" and "close" buttons when you editing a comment
  4. Lastly, you could also apply the logic for having clickable URLs in the content section, since most of the time participants are referring to some content via URLs.

If we have these, we can do the merge. Very excited to release this feature. Thanks a lot!

@NearW
Copy link
Collaborator

NearW commented Mar 16, 2023

Closing this one since we can't apply this PR to the new codebase. The issue will be evaluated and prioritized in the future.

@NearW NearW closed this Mar 16, 2023
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.

3 participants