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

Enable context menu for thread with replies #1523

Merged
merged 17 commits into from
Mar 3, 2022

Conversation

Komediruzecki
Copy link
Contributor

@Komediruzecki Komediruzecki commented Feb 24, 2022

Change behaviour of delete when thread has replies

Add deleted initial comment handling

Showcase:

ContextMenuForThread_2022-02-24.17-19-08.mp4

TODO:
Best to merge this after thread updates are fixed and reactions are merged

Also features a fix for comment sorting so its easier to track the this feature (change will be nulled when that PR is merged)

src/cloud/components/Comments/ThreadItem.tsx Outdated Show resolved Hide resolved
src/cloud/components/Comments/ThreadItem.tsx Outdated Show resolved Hide resolved
src/cloud/components/Comments/ThreadItem.tsx Outdated Show resolved Hide resolved
src/cloud/components/Comments/ThreadItem.tsx Outdated Show resolved Hide resolved
src/cloud/components/Comments/ThreadItem.tsx Outdated Show resolved Hide resolved
Change behaviour of delete when thread has replies
@Komediruzecki Komediruzecki force-pushed the feature/enable-context-menu-on-thread-with-replies branch from 94c52e9 to 4b03ea7 Compare March 1, 2022 20:05
@Rokt33r Rokt33r force-pushed the feature/enable-context-menu-on-thread-with-replies branch from 4b03ea7 to 9539b92 Compare March 2, 2022 07:08
<div className={className}>
<CommentListContainer className={className}>
{initialComment == null && (
<div className={'deleted_initial_comment'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

lets not abuse brackets for strings

<div className='deleted_initial_comment'>

className feels a bit overkill to me

comment--deleted would be simpler and maybe more reusable.


return threadComments[0].user
}, [threadComments, user])

const submitComment = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

the naming irks me a bit. We are not submitting a new comment
we are submitting an update to the thread's initial/first comment.
updateThreadOpener or something would be more intuitive

return
}

if (thread.initialComment == null || replyCount === 0) {
Copy link
Contributor

@Davy-c Davy-c Mar 2, 2022

Choose a reason for hiding this comment

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

I am a bit confused by this logic here.
so a user will still be to completely delete threads that have replies?
They just need to remove their initial comment and then delete again for the thread itself? If we want to mimic slack it is not correct.

It should be back end logic.

in the front end: always send deleteComment
in the backend: delete comment, if thread affiliated has no more comments, remove thread.

Copy link
Contributor Author

@Komediruzecki Komediruzecki Mar 2, 2022

Choose a reason for hiding this comment

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

The onThreadDelete is fired when user can click on trash button, but the button itself is shown only if there are no replies (or initial comment was not yet deleted - in this case it will be), so no, user cannot do this afaik.

If the initial comment was deleted, you can see the trash icon only if number of replies is zero.
User needs to have 0 replies nevertheless of inital comment being removed, if it is not, it will remove the initial comment, not the thread

threadOpenedUser != null && user != null && threadOpenedUser.id == user.id
if (thread.initialComment == null) {
return (
<div className={'comment__meta__actions'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary brackets

)}
</div>
) : (
<div className={'thread__comment__line'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary brackets

</div>
) : (
<div className={'thread__comment__line'}>
<span>{thread.contributors[0].displayName}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

comments have set null on user, so contributors could be an empty array.

}) => {
const { translate } = useI18n()
return (
<div className={'thread__comment__line_more_replies_container'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary brackets,
className should be simplified
thread__comment__replies

@Rokt33r Rokt33r merged commit 30e0815 into master Mar 3, 2022
@Rokt33r Rokt33r deleted the feature/enable-context-menu-on-thread-with-replies branch March 3, 2022 06:26
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.

None yet

4 participants