If there is an existing comment, you turn it into a thread by adding a second comment. In other words, there can never be any threads that only have only one comment.
I think this would break consistency with the diff notes, unless you consider the diff itself the first comment? I don't think that consistency is essential from a UX perspective, though.
If there is an existing comment, you turn it into a thread by adding a second comment. In other words, there can never be any threads that only have only one comment.
@victorwu I very much disagree with this. As a reviewer, I want to be the one to decide user whether a comment of mine is a discussion that needs to be resolved or just a passing comment. That's not something for the MR author or any other commenter to decide.
Our discussions are like Slack threads but more powerful, and we shouldn't reduce that power to be more like Slack.
I know you said:
Let's start with issue comments, commits, snippets, and not consider merge request resolvable discussions for now, since resolvability is more complexity. We can cover that in the next issue.
But we should strive for internal consistency between MR and issues more than GitLab/Slack consistency.
I think it's perfectly reasonable to allow new discussions to be created (as in https://gitlab.com/gitlab-org/gitlab-ce/issues/24378) and allow existing individual comments to be turned into discussions by replying to them.
I would really like this change! Whenever I find myself wanting to use the "start discussion" feature, it's usually in response to another comment, and it would be makes sense to be able to turn that comment into the root of the discussion thread.
Yup there is now a glaring need for this because the existing feature of start discussion begs for it .
@DouweM : Would it be a relatively small update to add this? Seems like a good one we can consider for 9.3. We'd need some UX. But I don't see the existing UI changing much to support it.
What happens when you have a thread and start deleting comments from it? Proposal is that if you keep deleting comments from it, and there is only one comment left, it is "demoted" into a regular non-thread comment.
I think it can stay discussion for a first iteration.. or otherwise we'd have to add a system note with deleted all subcomments demoting to normal comment ... or something like that, as there is a system note with "started a discussion" (like where is that discussion now?)
@DouweM : Would it be a relatively small update to add this? Seems like a good one we can consider for 9.3. We'd need some UX. But I don't see the existing UI changing much to support it.
@victorwu For the backend, it's a small change. For the frontend, potentially more involved, since clicking the "Reply in new discussion" (or w/e) button should display the note as if it were a discussion and focus on the text field, but when canceling, it should go back to being a normal note again. So after clicking "Reply in new discussion", we have to display it on the frontend as a discussion, but not actually turn it into a discussion on the backend yet.
@victorwu Check out this flow and let me know what you think. With abuse and threaded comments being added to the actions, I think we need to collapse some - I've added this in the mockups here. Let me know if you'd like to split this into separate issues.
I think "Reply" works, I updated the copy. Thanks @jk
@blackst0ne I'm not sure I understand the need for your proposal. It sounds like you want to pull out the dropdown button and make the comment box have four buttons: Comment, Start a discussion, Comment & close issue, and Discard draft. I am arguing that with this issue and the new design, a "Start a discussion" button in the comment form is not necessary. Each individual comment will have an icon that is used as a way to reply to that individual comment (ie. start a thread, like in Slack). This only requires two clicks (Click "Reply" and then click "Comment"). I don't feel this is too many clicks and follows the same user flow as replying to a thread in an MR diff.
I think making the distinction between comment and discussion is unnecessarily complex with this design.
I don't see any reason why to have menu on "Comment" button. You add comment, then later you hit reply to start a thread there. That's it. Anything else is redundant and pollutes UI.
hahah I haven't heard that called a meatball menu before @victorwu. We use "kebob" on the profile page:
They are both overflow menus. An ellipses is commonly known as meaning "there is more here," which is why I chose that version. I don't have a brilliant reason for one over the other, they both are just meant as a means of accessing overflow information. We can use the Kebob to be consistent with the profile.
I don't see any reason why to have menu on "Comment" button. You add comment, then later you hit reply to start a thread there. That's it. Anything else is redundant and pollutes UI.
I completely agree here. I don't understand the consistency comment. MR's create resolvable discussions, issues do not. They are inherently inconsistent.
@victorwu@tauriedavis In the current design, are we getting rid of the "Comment"/"Start discussion" toggle, or not?
As I've described earlier in https://gitlab.com/gitlab-org/gitlab-ce/issues/30299#note_26609931, I'm very much opposed to removing it, since it would cripple the "Start discussion" functionality, which was never intended to just add threads like in Slack, but explicitly designed as a code review feature: to leave feedback that the user needs to resolve, that is not connected to a specific piece of diff, but more generic like "Please add a changelog item" or "Did you check how this affects the API?". These need to start out as resolvable discussions to ensure they're not forgotten about by either the MR author or reviewer. See https://gitlab.com/gitlab-org/gitlab-ce/issues/24378 and https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7527.
On issues, deciding up front if something needs to be a discussion or a regular comment doesn't seem as useful, but on merge requests this is an absolute must. We could get rid of the toggle on the issues, snippets and commit pages, and only keep it on the MR page, but that breaks consistency.
The mockups in https://gitlab.com/gitlab-org/gitlab-ce/issues/30299#note_28612600 currently don't support such a discussion that doesn't yet have any replies, and the proposal in "What happens when you have a thread and start deleting comments from it? Proposal is that if you keep deleting comments from it, and there is only one comment left, it is "demoted" into a regular non-thread comment." is also incompatible with this.
I think this discussion is getting muddied by seeing discussions as the same thing as (Slack) threads, which they are not, especially on merge requests.
@DouweM Starting a discussion just to make a comment "resolvable" is a well hidden feature. The use-case makes sense, but implementation via starting a discussion feels more like an accident, than a feature.
I don't think starting a thread from an initial comment makes sense. I think creating a resolvable comment initially does make sense but I left that off per:
Let's start with issue comments, commits, snippets, and not consider merge request resolvable discussions for now, since resolvability is more complexity. We can cover that in the next issue.
I think this might start a larger conversation that might be better for a separate issue but it makes sense to me that there is only one type of comment and that comment can either be resolvable or not. I know we've discussed this before but I do not see the difference between a discussion, a resolvable discussion, and a comment. There may be a difference code-wise, but from a user perspective you have:
A comment
A comment that you can resolve
You can respond to a comment, and this creates a thread of comments. Any comment can be turned into a comment that can be marked as complete.
I agree with @jk about the current UI being hidden. I am not advocating for removing the feature, either.
I think each comment should have a way to make it resolvable. On create or edit, there can be a checkmark that allows you to make it resolvable and this would work for all areas that have comments.
Maybe a better wording on the checkbox: "This comment needs to be resolved." … With a small question mark which explains what happens next (like the one next to time tracking in the sidebar). The "resolvability" on its own is too passive — the label should suggest there will be another action later.
@DouweM Can you review these mockups and see if there is anything from our discussion that I missed?
Comments would work exactly the same when adding a comment to an issue or merge request discussion. You would be able to mark/unmark a comment as resolvable. You can resolve all comments, just as you can today. You can also collapse discussions (threads).
Comments on diffs are slightly different, in that they are always resolvable comments and there is something more specific you are commenting on than the issue/mr as a whole. This doesn't change much about the UI. I've included a design that allows you to still collapse each discussion. This mockup also includes adding multiple discussions to a single line.
@DouweM@victorwu I know this includes more than what is in this scope. I want to make sure we agree on this direction and then we can cut back to just the basics for a first iteration. Let me know your thoughts and if there is anything I missed.
Not sure if we discussed this explicitly. If we introduce resolvability for issue discussions, would we bring that functionality of skipping to the next unresolved discussion and seeing how many unresolved discussions to issues as well?
@victorwu Yeah, that makes sense. I wonder if it should be "unresolved comments" though, since not every comment in a discussion needs to be resolved - like with diffs.
1. Ability to mark and unmark a comment as resolvable
What do you think about something like "Needs to be resolved" with a checkmark instead of "Mark as resolveable"? Describing the goal instead of the technical detail.
If we stick with "Mark as resolveable", I think it should be "resolveable"!
2. Replying to a comment
Looks good to me!
3. Ability to resolve all comments that can be resolved
I think we should continue to decide on a per-discussion basis if it is resolvable, instead of per-comment. A resolvable discussion mentions a problem, the comments on that discussion try to find to a solution, and when it is found, the discussion is resolved. It doesn't make sense to me to have a discussion where only 1 comment is resolvable. If that were to happen, it would make more sense to create a new discussion that can be resolved as a whole. So I think the "This comment is resolvable" checkbox and "Resolve all comments" label don't make sense here.
Note that currently, we do allow individual comments to be marked resolved/unresolved, but this is really just because we may find 2 problems in the diff one line, and currently don't have a way to create individual discussion for each. If we did, we could/should let go of resolving individual comments, and only allow this on a per-discussion basis.
Note that when marking an individual root-level comment as resolvable, you are really marking that comment and any subsequent replies on that comment (also known as: the discussion) as resolvable.
We also need an entire discussion to be resolved or not resolved, instead of its individual comments, because we automatically collapse resolved discussions in the Discussion tab, so that people are not distracted by them.
It is currently, and ideally would still be, possible to see on a MR discussion (diff or non-diff) when it was last updated and/or resolved and by whom, even when it is collapsed. This way it’s relatively easy to see when scrolling through the page what changed about the discussions since the last time you viewed it, by looking at those timestamps.
4. Make a comment resolvable initially
Looks good to me, as long as that's the form to create a root comment, not a comment on a discussion. In that case the text should be "This discussion needs to be resolved." or similar, per thought 3.1.
5. Collapsed version
Looks good to me, as long as we address thought 3.2 by showing the last updated/resolved time and author.
6. Commenting on a diff
Looks good to me, but it may be worth showing "This discussion will need to be resolved" in gray on the right of the "Comment" button, just like we have "Please review the contribution guidelines for this project." on https://gitlab.com/gitlab-org/gitlab-ce/issues/new
7. Appears on discussion tab
Looks good to me, as long as we address thought 3.1. and change "Resolve all comments" to "Resolve discussion" as it is now
8. Collapsed version
Looks good to me, as long as we address thought 5.1.
9. Ability to start an additional discussion on the same line
I would add some padding between the discussion and the "Start a new discussion" button.
10. Two discussions on one line
What will it look like when one of those discussions is resolved and collapsed and the other is still relevant?
11. Both appear separately on the discussion tab and are collapsible.
1. Ability to mark and unmark a comment as resolvable
What do you think about something like "Needs to be resolved" with a checkmark instead of "Mark as resolveable"? Describing the goal instead of the technical detail.
I did initially think a checkmark would make sense here. We use this on other dropdowns to indicate an option has been marked. However, the rest of the dropdown are actions, rather than options. And I thought mixing the two would be weird.
If we stick with "Mark as resolveable", I think it should be "resolveable"!
Yes, definitely. This was a typo, that I always seem to make :)
3. Ability to resolve all comments that can be resolved
I think we should continue to decide on a per-discussion basis if it is resolvable, instead of per-comment. A resolvable discussion mentions a problem, the comments on that discussion try to find to a solution, and when it is found, the discussion is resolved. It doesn't make sense to me to have a discussion where only 1 comment is resolvable. If that were to happen, it would make more sense to create a new discussion that can be resolved as a whole. So I think the "This comment is resolvable" checkbox and "Resolve all comments" label don't make sense here.
I feel like this is a limiting use case. Not all comments need to be resolved. And replying to a comment does not necessarily mean that an action needs to be taken. It may be that comments on diffs all need to be resolved, but I'm not sure I buy that every comment within a thread needs to be resolved. This is why I'm trying to push towards not thinking of threads as discussions that need to be resolved as a whole.
Note that currently, we do allow individual comments to be marked resolved/unresolved, but this is really just because we may find 2 problems in the diff one line, and currently don't have a way to create individual discussion for each. If we did, we could/should let go of resolving individual comments, and only allow this on a per-discussion basis.
I would be okay with this for diffs, but I don't think it works for individual comments on issues/mrs that have replies.
Note that when marking an individual root-level comment as resolvable, you are really marking that comment and any subsequent replies on that comment (also known as: the discussion) as resolvable.
I disagree with this. I don't know why it has to be that black and white.
We also need an entire discussion to be resolved or not resolved, instead of its individual comments, because we automatically collapse resolved discussions in the Discussion tab, so that people are not distracted by them.
This makes sense for diff comments, but I'm not sure it makes sense for MR/Issue comments.
It is currently, and ideally would still be, possible to see on a MR discussion (diff or non-diff) when it was last updated and/or resolved and by whom, even when it is collapsed. This way it’s relatively easy to see when scrolling through the page what changed about the discussions since the last time you viewed it, by looking at those timestamps.
Sounds good, I will add that to the mockups.
4. Make a comment resolvable initially
Looks good to me, as long as that's the form to create a root comment, not a comment on a discussion. In that case the text should be "This discussion needs to be resolved." or similar, per thought 3.1.
I don't think it has to be, per 3.
5. Collapsed version
Looks good to me, as long as we address thought 3.2 by showing the last updated/resolved time and author.
Will add that
6. Commenting on a diff
Looks good to me, but it may be worth showing "This discussion will need to be resolved" in gray on the right of the "Comment" button, just like we have "Please review the contribution guidelines for this project." on https://gitlab.com/gitlab-org/gitlab-ce/issues/new
Sure thing, I don't see a problem stating that.
7. Appears on discussion tab
Looks good to me, as long as we address thought 3.1. and change "Resolve all comments" to "Resolve discussion" as it is now
Yeah, lets get to a conclusion on 3 in order to decide this.
8. Collapsed version
Looks good to me, as long as we address thought 5.1.
9. Ability to start an additional discussion on the same line
I would add some padding between the discussion and the "Start a new discussion" button.
Will double check padding.
10. Two discussions on one line
What will it look like when one of those discussions is resolved and collapsed and the other is still relevant?
Great question, will add that in the next iteration.
Biggest thing we need to decide on is individual comments as resolvable vs. entire discussions as resolvable. I'd like to get some more input from other uxers here. @pedroms can you take a look and weigh in on this?
1. Ability to mark and unmark a comment as resolvable
What do you think about something like "Needs to be resolved" with a checkmark instead of "Mark as resolveable"? Describing the goal instead of the technical detail.
I did initially think a checkmark would make sense here. We use this on other dropdowns to indicate an option has been marked. However, the rest of the dropdown are actions, rather than options. And I thought mixing the two would be weird.
@tauriedavis Fair enough. In any case, part of my concern was about the phrasing "Mark as resolveable". Even though "resolvable discussions" is what we call these internally, the intention behind that action is not so much to mark something as something, but to require that discussion/comment to be resolved in order to move forward with the MR. I don't think "mark as resolvable" is strong enough in indicating that. What do you think about "Needs to be resolved"/"Doesn't need to be resolved" or "Requires/Needs resolution"/"Does not require/need resolution" for the 2 different states?
3. Ability to resolve all comments that can be resolved
I think we should continue to decide on a per-discussion basis if it is resolvable, instead of per-comment. A resolvable discussion mentions a problem, the comments on that discussion try to find to a solution, and when it is found, the discussion is resolved. It doesn't make sense to me to have a discussion where only 1 comment is resolvable. If that were to happen, it would make more sense to create a new discussion that can be resolved as a whole. So I think the "This comment is resolvable" checkbox and "Resolve all comments" label don't make sense here.
I feel like this is a limiting use case. Not all comments need to be resolved. And replying to a comment does not necessarily mean that an action needs to be taken.
Right, I'm not saying that all discussions need to be resolved, I'm saying that a discussion as a whole can be resolvable or not resolvable, but not an individual comment inside a discussion.
So there would be a way to toggle the resolvability of a discussion (which may at that point only consistent of 1 comment), but not of an individual comment inside a discussion. Either all comments in a discussions are resolvable (i.e. the discussion is resolvable) or none of the comments in a discussion are resolvable (i.e. the discussion is not resolvable). A single root comment can be made resolvable, which technically makes it a 1-comment resolvable discussion. Any subsequent replies to that comment become part of that resolvable discussion.
I may not have been clear before, but with that in mind, please read my thought 3 again, and see if it makes more sense now :)
It may be that comments on diffs all need to be resolved, but I'm not sure I buy that every comment within a thread needs to be resolved. This is why I'm trying to push towards not thinking of threads as discussions that need to be resolved as a whole.
But does it make sense to have a thread/discussion consisting 4 comments, only 2 of which needs to be resolved? That implies to me that it's not a thread about 1 subject at all, but rather a thread about 2 subjects, that would have been better off as 2 separate threads. If it was 2 separate threads/discussions, the resolvability status could be a property of the entire discussion, and we could easily collapse that discussion if it is resolved, since we can now regard that discussion as no longer relevant.
If we agree that the intention is for each discussion/thread to represent a single sub-subject of the main issue/MR, whether that be a specific line of code or a part of a proposal someone isn't sure about, wouldn't it make sense to resolve the entire discussion about that subject at once?
Note that currently, we do allow individual comments to be marked resolved/unresolved, but this is really just because we may find 2 problems in the diff one line, and currently don't have a way to create individual discussion for each. If we did, we could/should let go of resolving individual comments, and only allow this on a per-discussion basis.
I would be okay with this for diffs, but I don't think it works for individual comments on issues/mrs that have replies.
Note that when marking an individual root-level comment as resolvable, you are really marking that comment and any subsequent replies on that comment (also known as: the discussion) as resolvable.
I disagree with this. I don't know why it has to be that black and white.
We also need an entire discussion to be resolved or not resolved, instead of its individual comments, because we automatically collapse resolved discussions in the Discussion tab, so that people are not distracted by them.
This makes sense for diff comments, but I'm not sure it makes sense for MR/Issue comments.
4. Make a comment resolvable initially
Looks good to me, as long as that's the form to create a root comment, not a comment on a discussion. In that case the text should be "This discussion needs to be resolved." or similar, per thought 3.1.
I don't think it has to be, per 3.
Biggest thing we need to decide on is individual comments as resolvable vs. entire discussions as resolvable. I'd like to get some more input from other uxers here. @pedroms can you take a look and weigh in on this?
I fail to see the use case for having a discussion consisting of 4 comments, that can all be individually marked as resolvable or not resolvable, and then individually marked as resolved or not resolved. A thread of 4 comments that has 2 non-resolvable comments, 1 resolved comment, and 1 unresolved comment feels to me like it should really have been 3 different discussions.
After reading through this long (but interesting) discussion and weighing in everyone's valid opinions, here are my thoughts:
The dropdown action as “Mark as resolvable” / “Unmark as resolvable” makes more sense as it starts with a verb and is consistent with the rest of the actions. If we were to rephrase the other actions to match “Needs to be resolved”, as an example, “Report abuse” would be “Has abusive content” or similar.
My mental model for this is, starting from the broadest level: Discussions > Discussion (aggregates comments and their replies) > Comment > Replies (in essence they're comments).
A Comment starts a Discussion if it has Replies, and the whole Discussion (read: the first Comment) is resolvable, not its individual Replies.
A Comment may be resolved on its own if it's marked as resolvable, even if it doesn't have replies.
I don't see the need to have “This discussion needs to be resolved” when replying to a comment. However, I do think it's helpful to inform the user of this when starting a new discussion.
The “Start a new discussion” button sounds good, but perhaps “New discussion at this line” is clearer?
Keep the “Reply” icon intact and don't impose the discussion-toggling behavior. Instead, rely only on the “X replies” toggle to open and close the discussion (of course it would need to be visually adapted when opened)
Indenting the replies is crucial for the understanding of discussions and resolvability. I like how @tauriedavis represented that for issue discussions, but that should also be applied to merge requests. Perhaps moving the first comment to the left, aligning it with the code line numbers.
Reducing the visual weight of the replies is also important, maybe with a smaller avatar.
Some different examples for issues and merge requests:
Thanks @pedroms - we came to the same conclusion about resolving the entire discussion yesterday in our meeting so its great you agree. I also think you have some nice ideas in terms of UI and will adapt them to the final designs.
@tauriedavis nice, thanks! Overall I think it solves the problem, but here are my two thoughts:
What does “Last updated by…” mean? If we're only talking about replies, maybe it could be clearer as “Last reply by…”?
If we only have one reply, maybe: “John Doe replied about 2 months ago”
If we have multiple replies from the same person, maybe “2 replies by John Doe” and under it “Last reply about 2 months ago”
I'm not sure about the icon design and placement of the collapse/expand toggle:
Icon design: it's not the most common design, maybe a simple chevron or triangle would suffice, also because the collapsing/expanding behavior has only one direction, it doesn't expand or collapse from the middle
Placement: when collapsed, the blue “X replies” link is more visible than the button on the far right side, which is a bit out of the reading flow. When expanded, I would look for the same location of the blue link to collapse the replies. It also looks like it relates to the “Reply…” input as it is inside the same container and very close to it, like a submit button.
@tauriedavis Instead of "2 replies[NEWLINE]Last updated by Tim Smith about 2 months ago", what do you think about 2 replies — last by Tim Smith about 2 months ago? This also addresses @pedroms's first point.
@fatihacet I will work on the backend, but we only finalized the UX on Friday. As you can see a little bit up there are still some questions, but nothing that should block development.
Let me know how you'd like to go about this, and if you think it's easiest for frontend or backend to start!
2+ replies from multiple people: Last reply by [Name] about [time period] ago2+ replies from the same person: by [Name] about [time period] ago1 reply: by [Name] about [time period] ago
@pedroms Was looking at another way to do expand/collapse and this is what I came up with:
Basically I just left the top summary there and you can expand/collapse from there. What are your thoughts?
@fatihacet Most of the UX is finished and I believe you (or whoever from frontend) can get started while we finalize how to expand/collapse.
@tauriedavis I agree with the suggested copy for varying number of replies and the way that is visually represented
I think the expand/collapse is much better like this because it's consistently in the same place. However, to reduce the visual clutter a bit, I would suggest:
Reduce the size of the avatars when the discussion is collapsed, which makes the whole toggle block a bit smaller. Since it already has a light gray background, this shouldn't impair its visual highlighting. Maybe size them like the assignee avatars in the collapsed issue sidebar:
This reminds me that we need to design a “maximum” state when we have lots of different people replying. Take a look at the multiple assignees issue, where the maximum is 3 avatars and then a +N.
Move the chevron icon from the right to the left, before the avatars
With the toggle block smaller and the chevron before the avatars, we can change the content of the toggle block when it's expanded, so we don't repeat things. Replace the avatars and reply information with “Hide replies” or similar.
Of course, feel free to disagree and improve upon this
Thanks @DouweM@tauriedavis . I should have looked at the other issue more carefully. I took the comments there and the design and stuck it in the description in this issue.
@JobV : During the 9.3 iteration, @fatihacet discovered that additional technical work is required to align the FE code with Vue in order to develop the feature here. That is being done here in 9.4.
Maybe I'm missing something, but why do comments need to be unresolvable ever? There may be nothing to resolve, but what's the harm in having the ability to mark it resolved anyway?
It seems to me the main issue is whether there's an indication of comments being unresolved as a potential blocker. So, I think "Needs resolution" is the checkbox we'd want. Either a comment/discussion is marked so that we notice that it needs resolution before we close the issue or merge the MR or we don't get that notice because it doesn't matter.
How about all comments and discussions can be marked resolved but only select ones are marked as "needs resolution" so that there's an expectation of those getting resolved?
@victorwu Unfortunatly . We will have to do this for 10.1. We still need to get the refactored finished and @filipa and @fatihacet will be out most of this month.
I'm a little late to this thread but +1 for this feature-request and I just wanted to ask if a simpler version of this implementation would just be an option in the edit dropdown of a comment to convert it into a discussion.
Mostly for me I find that team-members want to start a discussion but accidentally leave a comment.
@victorwu when could we get this in? This would help us have more efficient discussions by breaking them down into smaller parts that are addressed separately. Epics would most benefit from this as they're kind of “meta-issues” (these issues already have long discussions).
It would be awesome to have in release post MRs. We sometimes have comments that need to be tracked and managed, but we risk to lose them in the huge amount of other things since they don't count in the number of open discussions. Turning them into discussions could solve the problem.
@victorwu The most complicated thing here, by far, is the frontend. The backend will need some changes too, but besides some type checks, and hard-coded assumptions of the noteable type that need to change, no new logic needs to be written.
Anything blocking this from a Vue perspective @fatihacet?
@victorwu I don't think there is something blocking right now. We can start working on this for Issues and when we complete the MR discussions refactor and merge it into master, this will also be available for MR discussions too.
@stoivo : If you're interested, would you like to work on the feature of taking an existing non-discussion thread comment and turning it into a discussion thread? We already have the design ready (see mockups above). It'll be just clicking the reply button of such a comment to start the thread. (If you use Slack, the design is very similar.) We can separate some of the other ideas here to another issue.
@victorwu might there be a chance to move this to something sooner than the indicated planning date inside of the epic (8jan)? This is a much-requested feature and would especially be great to have to improve design discussions. cc: @sarrahvesselov
BTW lately I've been wishing more and more that this was a feature. I'd use it extensively! It seems the issue might be a bit out of sync since it's so old - should someone other than Douwe be assigned, since this is now marked ~Plan? (cc/ @victorwu)
From a backend perspective, I think this would come down to converting an existing Note to a DiscussionNote when a new comment is created referencing an existing Note in its reply_to_discussion_id.
That means, the discussion function fails 66% of cases.
Even if the statistical sample obviously is very small, it suggests that there is a problem with the discussion function.
I think this is a problem that needs a solution.
Unfortunately I can't think of a good idea. Maybe someone of you can come up with one ?
I think @annabeldunstone should pick up this issue from me, as she has been working on comments and this issue will need to be updated to match current designs.
@annabeldunstone@fatihacet : This is how the merge request discussion tab looks right now. Diff discussions and non-diff discussions look different. Not ideal. But I don't want that to slow us down. So I think that this issue here will not change anything with diff discussions. But non-diff discussions will change in that they can now be started from a standalone comment. That's the only change that this issue would introduce.
@annabeldunstone : I updated the description here per our latest styling, and indicated the logic for where the reply button should appear (basically in all comments), and what happens when you click on it. Feel free to change it if it's not what you hand in mind for the design.
I am frustrated by the implementation of the discussion function, because more than half of the time (more like 2/3), people don't respond within the discussion although they respond to a discussion comment. They just write in the standard reply box below. I gave 2 examples above.
IMO the current concept is broken. I have never witnessed this behaviour in other discussion forums, because their UI prevents that from happening in the first place.
I haven't encountered users not responding to threads myself so thank you for linking to those specific cases. The design for comments and discussions has recently changed. We may find that now that comments are more contained, users are able to more visually distinguish between the two.
because their UI prevents that from happening in the first place.
Can you provide an example of what you mean here?
@annabeldunstone I think its separate from this issue but consider a small study that ensures users understand how/when to reply to threads.
@tauriedavis I haven't encountered users not responding to threads myself
I assume that's because you mostly communicate with tech-savvy GitLab colleagues ?
My sample of correspondents mostly included users who are new to or inexperienced with GitLab. And honestly, in about 66% of cases (number increased since my last posting) the users just use the normal commentbox, because it's bigger, already open and they are familiar with it -- whereas the discussion commentbox is not expanded (just 1 line) and visually not very remarkable ... so
either the users don't SEE the discussion commentbox
OR they ignore it because they don't know that it has a different function
OR they don't care
OR they just use what they are most familiar with (the regular commentbox)
I have no idea. And even if I had, it wouldn't the change the fact.
I'm sorry to say that, but if that occurs, the UI design is broken.
The UI needs to be designed in a manner that doesn't depend on perfect or even correct user behaviour.
because their UI prevents that from happening in the first place.
Can you provide an example of what you mean here?
Let me put it this way: I have NEVER come across any site in my life, where I would have seen either replies on the wrong level or even the possibility to do that.
Forum conversations are most of the time either unthreaded or fully threaded. In the first case (e.g. Bugzilla), problems of hierarchy don't exist by definition, and in the second case (e.g. reddit etc) users click on a comment's reply button.
A mix of both is rather rare.
Here is an example of how it is handled by a newspaper. The commentbox is ABOVE the comments. If you want to reply to a comment, you click on "antworten". I have been reading this newspaper forum for a decade and never saw a single instance of users confusing "reply to comment" with "reply to article". Whereas over here on GitLab I have seen this problem occur 6 times out of the 9 times I started a discussion within a single week. So there you have it. I can link to further examples than the 2 already posted above, if you need.
One part of the problem is that the the commentbox is BELOW all comments and if the discussion was the last event to occur, then commentbox and discussionreplybox are directly adjacent. No wonder people don't see a big difference. It might even happen to people who would want to make a distinction, but just click in the wrong box by mistake.
Unfortunately I don't have a magic solution right of the top of my head, but the design is broken. I'm sorry, but there is simply no other way to put it.
I think adding reply buttons would slightly reduce the number of wrong-level-replies, but in itself that doesn't solve the problem entirely.
@chrizilla Thanks for all your feedback, we definitely understand the concern and the problem and will work to make sure users know when and how to use discussions!
I agree with @chrizilla - the majority (even the vast majority!) of the time I find my colleagues either don't ever start discussions or don't reply correctly - despite them all being fairly tech-savvy (some are devs) and having had multiple reminders from me. This is incredibly annoying, and I am frustrated that something as simple as converting a comment to a discussion has now been kicking around for a year and a half with no resolution.
Starting a discussion is hidden underneath a menu that is almost invisible. You can tell the UI sucks because even in this issue about how sucky it is there are barely any discussions - almost everyone has posted comments!
I know it is important to get it right, but please can we have some sort of quick-fix in the mean time? To be honest, it would be better if every comment was a discussion than the current state of a confusing mixture.
@elishahastings: I agree with @chrizilla - the majority (even the vast majority!) of the time I find my colleagues either don't ever start discussions or don't reply correctly - despite them all being fairly tech-savvy (some are devs) and having had multiple reminders from me. This is incredibly annoying
@tauriedavis: "@annabeldunstone I think its separate from this issue but consider a small study that ensures users understand how/when to reply to threads"
@tauriedavis: "Thanks for all your feedback, we definitely understand the concern and the problem and will work to make sure users know when and how to use discussions!"
@tauriedavis: I admit I'm reading between your lines, but to my ears they sound more like "let's educate users so that they understand how/when to reply to discussions" rather than "ok, our design is broken, we have to fix it".
Let me tell you one thing: The user is NOT the problem here!
Just yesterday I run into the same problem with a very intelligent and tech-savvy software developer. Even he failed to recognize the discussion. And someone of his stature shouldn't even be the standard. The standard should be first-time(!) GitLab users who file a bug report issue in a GitLab project. They should be able to correctly participate in the discussion without taking a how-to-use-Gitlab course or without you educating them. Everything else is a broken concept. Sorry to be blunt, I am afraid if I am not, this falls on deaf ears and won't get addressed.
Let me summarize her suggestions/observations, all of which I support whole-heartedly:
Starting a discussion is hidden underneath a menu that is almost invisible.
Even in this issue about how sucky it is there are barely any discussions - almost everyone has posted comments!
It would be better if every comment was a discussion than the current state of a confusing mixture.
I believe the description in this issue addresses much of your complaint here.
Currently, our threaded discussions are unintuitive to use. When posting a new note, you have to decide at the outset if you intend to allow direct, threaded replies by selecting "Start discussion" from a dropdown action:
This not ideal. Most of the time when I find a note I want to reply to directly, the original poster did not have the foresight to create it as a "discussion", so I have to post a new comment in the main thread in order to reply.
That is why we are changing the behavior to allow you to "reply" to any note, regardless of what option was selected when it was originally posted. This will make threaded discussions a lot more easy to use.
If we want to change around the design to make threaded discussions more discoverable (by, for instance, having the "Reply..." text field below every note), I believe that is a discussion for another follow-up issue. In any case, the change outlined in this issue description is a necessary first step and it will be a large improvement.
I admit I'm reading between your lines, but to my ears they sound more like "let's educate users so that they understand how/when to reply to discussions" rather than "ok, our design is broken, we have to fix it".
@mikegreiling: Most of the time when I find a note I want to reply to directly, the original poster did not have the foresight to create it as a "discussion", so I have to post a new comment in the main thread in order to reply.
no, at least not for the problem @elishahastings and myself were discussing and which you quoted (so I assume you are referring to it).
The function you mention lets you reply on level -1 everywhere.
But it won't change anything at all about users replying on level 0, when referring to a level -1 comment !
@mikegreiling: If we want to change around the design to make threaded discussions more discoverable, I believe that is a discussion for another follow-up issue.
@chrizilla: to my ears they sound more like "let's educate users so that they understand how/when to reply to discussions" rather than "ok, our design is broken, we have to fix it".
"consider a small study that ensures users understand how/when to reply to threads"
@tauriedavis: That happens to be precisely what I said. You don't need a study to make users understand or "ensure" that they understand. Again: THEY are not the problem. Your design is. You may need a study that ensures YOU understand why it is broken. That's a difference. The design needs to be functional in a way that there is no way to mishandle it - regardless of the user's level of understanding. Until you adopt this paradigm, you will only come up with small cosmetic retouches to somehow mitigate the problem without ever really solving it.
@tauriedavis: I will add that we will work to improve the design until we know that users understand the feature.
That's exactly what I meant.
It's the wrong paradigm to expect a certain level of understanding, absent which the GUI fails to function correctly. Offer something that doesn't depend on perfect or even correct user behaviour, but something that – quite to the contrary – would require some effort to MISUSE. I know, I am repeating myself, but that's because you haven't shifted to the right paradigm.
That happens to be precisely what I said. You don't need a study to make users understand or "ensure" that they understand. Again: THEY are not the problem. Your design is. You may need a study that ensures YOU understand why it is broken.
User research is always used to understand whether a design is working or not, and how we can improve it. Studies are never used to make users understand how to use something.
We appreciate your input and feedback and are doing everything we can to fix the problems with threaded discussions. We hear you. I ask that everyone here be respectful when addressing the designers and engineers whose everyday work is committed to listening to your feedback and improving GitLab for you, the user. Your tone and use of capitals, bolding is out of line and not productive to a positive outcome.
@mikegreiling: Most of the time when I find a note I want to reply to directly, the original poster did not have the foresight to create it as a "discussion", so I have to post a new comment in the main thread in order to reply.
no, at least not for the problem @elishahastings and myself were discussing and which you quoted (so I assume you are referring to it).
I believe by fixing a fundamental flaw in the creation of threaded notes which prevents people from using it in many (I would even argue "most") cases, users will end up with much more exposure to the feature and will be more likely to recognize and use it in the future.
If users rarely have the option to reply to a comment except in narrow cases where the note's author thought ahead to use "start discussion", they are far less likely to even remember that its an option when it is available.
What is being advocated here is pretty similar to the way threaded conversations work in Slack, and in my experience this works pretty well there:
Slack New Thread
Slack Existing Thread
The function you mention lets you reply on level -1 everywhere.
But it won't change anything at all about users replying on level 0, when referring to a level -1 comment !
There's no way to stop people from replying in the main thread vs a reply thread. The best we can do is sign-post the options clearly (which I think is what you're advocating). As @tauriedavis and @sarrahvesselov have indicated, this is something we will need to research. We're open to suggestions and appreciate your feedback.
Here are my own thoughts:
I would imagine placing a Reply... input underneath every top-level comment will add a lot of visual noise to the discussions so I don't like that idea. Perhaps de-emphasizing the comment box for the main thread when it is not in focus would be a better option. This would put it on the same perceived "importance" level as the threaded reply boxes and perhaps encourage more people to notice the other options.
We could also change placeholder text or the button text in the main thread comment box to something more explicit that would indicate that the user is about to create a new top-level comment as opposed to a threaded reply (further reenforcing that this is not the only option).
Another important change would be to modify the R key shortcut to reply in a new thread instead of quoting the highlighted text in a reply in the main thread (a vestigial behavior from before this feature was available).
When I look at this discussion, I see many posts without the "Reply..." row/input and some with the row. It makes no sense. Why I can reply to some and cannot to others? The reply input should be everywhere or nowhere.
I see two good aproaches to the discussions: single thread (e.g., Github), and one-level replies (e.g., Stack Overflow, or Facebook).
The single-threaded approach on Github makes sense for a simple discussions. You can quote when replying and you can open a new issue from a comment to discuss details on some sub-topic. The discussion is clean and it is easy to keep it on topic.
The one-level replies are nice on stack overflow, because the top-level comment is not a comment -- it is an answer to a problem and the replies are to improve the answer.
The question is, what Gitlab wants to do. To discuss an issue, the single-threaded approach is better and simpler. When doing code review, we need to discuss particular changes in a separate threads, because these threads are attached to the code, not to the issue.
Additional issue is with external events, like adding commits to a merge request while discussing the code in a thread. The commit is kind of reply and it won't get attached to the thread.
Sarrah Vesselov, I'm sorry you cannot take the criticism.
@mikegreiling: users will end up with much more exposure to the feature and will be more likely to recognize and use it in the future. If users rarely have the option […]
The GUI should NOT produce wrong results until such time as users have enough exposure and have familiarized themselves with the features, because you will ALWAYS have new users here.
The GUI should be hard to MISUSE, not hard to use.
The function you mention lets you reply on level -1 everywhere.
But it won't change anything at all about users replying on level 0, when referring to a level -1 comment !
There's no way to stop people from replying in the main thread vs a reply thread.
This is completely ridiculous.
Have you even read my comment above about the newspaper?
Honestly, I have seen many discussion forums across the internet in all my life and GitLab is the ONLY(!) one, where I have seen this problem occur.
Some of you seem caught in a state of denial. I'm wondering why the users without role badge consistently recognize and diagnose the problem so much more accurately (@elishahastings, @jk, etc).
So rather than saying "this cannot be done" when millions of sites can, I'd rather start looking at how other sites do it than trying to invent square wheels and claim that circular ones "cannot be done" because "there is now way".
from wiki which I strongly urge all GitLab designers and developers to read with a self-critical mind:
Reinventing the square wheel is the practice of unnecessarily engineering artifacts that provide functionality already provided by existing standard artifacts (reinventing the wheel) and ending up with a worse result than the standard (a square wheel). This is an anti-pattern which occurs when the engineer is unaware or contemptuous of the standard solution or does not understand the problem or the standard solution sufficiently to avoid problems overcome by the standard. It is mostly an affliction of inexperienced engineers, or the second-system effect.
Many problems contain subtleties which were resolved long ago in mainstream engineering (such as the importance of a wheel's rim being smooth). Anyone starting from scratch, ignoring the prior art, will naturally face these problems afresh, and to produce a satisfactory result they will have to spend time developing solutions for them (most likely the same solutions that are already well known). However, when reinventing the wheel is undertaken as a subtask of a bigger engineering project, rather than as a project in its own right hoping to produce a better wheel, the engineer often does not anticipate spending much time on it. The result is that an underdeveloped, poorly performing version of the wheel is used, when using a standard wheel would have been quicker and easier, and would have given better results.
@mikegreiling The best we can do is sign-post the options clearly (which I think is what you're advocating).
I'm not advocating anything. I'm just trying to make team members open their eyes, which so far has proven very difficult.
And no, I'm particularly NOT suggesting cosmetic retouches, such as styling changes as opposed to functional changes. I made that very clear above when talking about "cosmetic retouches".
We're open
I didn't get this impression during this discussion. When I first raised this issue, 4 developers/maintainers commented and none of them even acknowledged the problem. It took a second posting from me to even get this problem on the radar. (and then the same people wonder about boldening…)
Finding solutions depends on recognizing the problem in the first place, which I honestly don't think you have (fully). Failing to recognize the problem will just lead to cosmetic retouches that don't address/solve the underlying problem.
placing a Reply... input underneath every top-level comment will add a lot of visual noise to the discussions so I don't like that idea.
We could also change placeholder text or the button text in the main thread comment box to something more explicit that would indicate that the user is about to create a new top-level comment as opposed to a threaded reply (further reenforcing that this is not the only option).
@chrizilla great to see such passion for GitLab, but please keep the discussion civil.
Sarrah Vesselov, I'm sorry you cannot take the criticism.
This is completely ridiculous.
Are both examples of unnecessary, unfriendly engagements with people you're trying to work together with on making GitLab better. They don't deserve that, nor do other people (considering) engaging in this thread or any other issue.
GitLab is about making sure everyone can contribute. We want to create an environment where everyone feels free and welcome to do so. Being open and kind is fundamental to that.
I also want to remind people about the GitLab Code of Conduct in case people are not aware of it.
As @JobV noted, our goal is to foster an environment where both feedback and language are constructive. Please bear in mind that unfriendly comments not only affects people that the comments are meant for, but also other people trying to participate in the discussion.
Thank you for proving my point.
Rather than being thankful for criticism, you are offended by users pointing out flaws in your product.
I'm sorry you cannot take the criticism.
This is completely ridiculous.
Are both examples of unnecessary, unfriendly engagements
I very much stand by my remarks. This is my honest opinion. I don't apologize for honesty one bit.
You seem to confuse being "friendly" with being honest.
Being honest is friendly, being dishonest is unfriendly. Not the other way round.
Someone lying to your face with a polite, friendly smile is what you apparently seem to prefer.
being open is fundamental to that
than why do you fail to openly embrace the criticism ?
You have demonstrated the exact opposite of openness. Instead of acknowledging the problem and being thankful for honest feedback your taking honest criticism of the PRODUCT so personally has led to a huge distraction. Just think about how much progress might have been made, if instead of this faux outcry you had focused your energy on the problem, which you still don't fully understand and acknowledge rather than shooting the messenger. An age-old phenomenon.
Note that while I addressed the problem, you prefer to address ME – with ad-hominem arguments and attacks – instead of joining the effort. This just demonstrates the reluctance to take a deep self-critical and honest look at the problem at hand.
Maybe my comments were at least helpful in stirring things up a little. I just wished my original report as well as @elishahastings subsequent confirmation would have sufficed and allowed for a factual discussion. Maybe the right lessons are learned at a later stage, when the dust has settled a little. I believe there is a good chance you will look back at this discussion 1 year from now, when things have hopefully improved, and will see that this discussion was the first step of acknowledging and subsequently solving the problem. (Or it still persists at that point, depending on your readiness to acknowledge the underlying problem and depending on your change of attitude.)
Unless personally addressed, I don't intend to contribute to this discussion anymore, because the GitLab team replies have demonstrated that they don't recognize any value in it.
I know this issue is closed already, but the problem @chrizilla describes still persists. Maybe the issue #54238 (closed) can be reopened to further discuss this, anyway I will add my comments to this here:
You say GitLab is the only instance you know of with this problem
Might be true, I know at least one other one: https://www.discourse.org/ They have a similar approach of normal level replies and direct replies to comments. They even have a reply button for comments, hovever the direct replies appear as well as normal comments and as comments in a collapsed thread making their "thread" function even less likely to be discovered. I have experienced many instaces of users not replying directly to a comment but rather answering normal with the regular reply box.
You say the user is not the problem, but the design
This might be partly true, but I think users not paying alot attention to how things are supposed to work also play a role in this. I assume there are a ton of examples where even the best design failed for some users. A problem is just adding uncommon functionality and complexity leading to more ways for users to fail. I mean, someone even said he constantly reminded his colleagues how this all works and they kept failing.
However: I agree that a good design can help to reduce the amount of users failing, which seems really to be high in this case. So one thing would be to make discussions more distinguable like indenting all replies to a discussion a bit, using a slightly different background color for the thread highlighting the reply section a bit more.
So I am willing to further discuss on this, either here or in a seperate issue.
As a reminder: I have opened #54238 (closed) as focus point for this design flaw, if someone wants to face up to it (zero comments so far, quite a lot here, surely a coincidence only).
@chrizilla you have made your point repeatedly, at this point in a way and to an extent that is disruptive to having a productive discussion about this topic. Maintainers have responded and have committed to conducting further research. Please, refrain from repeating yourself so that it's easier for others to follow the progression of this issue.
@tauriedavis Can the "discussion from a non-discussion comment" be a modal view? It's already clustered with long discussions such as these, and it kind of hurt my eyes looking for ones that provide value and insight.
A modal would reduce such noise since the "discussion from a non-discussion comment" would be more of a specific discussion on the topic introduced by the comment.
The description says the reply icon should be shown on all replies of an existing discussion but the screenshot only shows it on the root comment. Which one should we implement?
Also, the icon you linked is a bit different (no lines within the comment box), is it okay to use this?
The description says the reply icon should be shown on all replies of an existing discussion but the screenshot only shows it on the root comment. Which one should we implement?
@engwan I think it should follow the screenshot, and just show the Reply button on the root comment of existing discussions. @victorwu what are your reasons to add the icon to every reply? I feel it might get cluttered and it seems unnecessary.
Also, the icon you linked is a bit different (no lines within the comment box), is it okay to use this?
Creating discussions the old way (clicking arrow beside comment button and selecting start discussion) created resolvable discussions. For this "reply" function, do we want the discussions to be resolvable too?
If we don't make it resolvable, this will be the first kind of discussion that's not resolvable (I may be wrong). But having it non-resolvable feels like a better approach, for me replying doesn't really imply that it needs to be resolved.
Nevermind this question. I found out that it's automatically resolvable in MRs and that there are no resolvable discussions in issues.
While working on the MR, I noticed that email replies to comment notifications were sending requests with a reply discussion_id. I had to change it so that it doesn't transform comments into discussions and keep current behavior.
I was just wondering if we want replies from emails to transform comments into discussions? We can handle this in a separate issue but personally I think they shouldn't so I wanted to ask you first.
@engwan Yep, resolvable discussions in issues will be coming in a future milestone!
Can we keep the current behavior for email replies? If I reply by email to a discussion, it's added a discussion comment on that current thread. Otherwise it should be a standalone comment.
it would be very nice, if every comment could be a starting point of a discussion. Currently I always discussion, so everybody is able to just jump in. Slack does it also, and I really love it
We may still be able to merge this in 11.8 since it's behind a feature flag but 11.9 is also fine.
@winh / @okoghenun I've created !24950 (merged) to fix polling and the "resolvable" issue. It has some frontend changes for the "resolvable" part. Please take a look when you have time.
So remaining issues are:
properly focusing the input field (being worked on)
properly revert state when clicking cancel
Is this needed before enabling feature flag? It doesn't break anything, just slightly bad UX
prevent transformed comment being marked as "Edited"
I had to touch the updated_at column of the transformed comment so that it is included in the polled response. This results in the comment being marked as "Edited".
Fix is to add a separate column edited_at so that polling can continue to use updated_at while the edited status can use edited_at. This could also be used in our resolve action. Currently, resolving a comment marks it as edited too which might not be what we want.
This is a fairly large change since it would probably require a background migration to update the notes table which is huge. I think we should not wait for this before enabling the feature flag. I will create a separate issue for this. #57330 (closed)
Unrelated issue: I can't toggle the resolve button multiple times. It only toggles once unless I unfocus the check button. I'll create a separate issue for this too. #57331 (moved) (Not sure if this is ~Plan or ~Create)
I'll also update the description here with our checklist of things to do.
We may still be able to merge this in 11.8 since it's behind a feature flag but 11.9 is also fine.
@engwan Given that it requires changes to code that is not behind a feature flag, I don't think we can pick it into %11.8 after the feature freeze though (unless we duplicate all affected components which would become a mess when removing the feature flag again). Maybe that means we need to improve our way we use feature flags in the frontend.
Given that it requires changes to code that is not behind a feature flag, I don't think we can pick it into %11.8 after the feature freeze though (unless we duplicate all affected components which would become a mess when removing the feature flag again). Maybe that means we need to improve our way we use feature flags in the frontend.
Ah, yeah. You're right. Considering a lot has changed in Vue, we can play it safe.
I think I know what you mean now. When we show individual comments on commits in the MR discussions, we group them into a discussion by commit.
So these show up as a discussion in MRs and when you reply to this discussion, the reply is posted on the commit as an individual comment. So they're just really individual unresolvable comments rendered as a single discussion in the MR.
Discussions of this type (grouped individual comments from commits) are not resolvable as of now. If this is what you meant, we can create a separate issue to discuss further.
To make resolvable discussions on the commit, you can create a discussion on the commit and these would be rendered as a separate discussion in the MR and these are resolvable.
Comments on the diff of a commit are also resolvable discussions.
we can create a separate issue to discuss further.
Could you do it please? As you know better how to formulate / describe / tag it.
Indeed, this was the case for a discussion, but I also explicitly started a discussion on a commit that's not resolvable neither on the commit, neither in the MR.