View and add comments to file in merge request diff

@smcgivern @awhildy @jschatz1 : This is the issue per our discussion.

@smcgivern : Can you comment on the feasibility and adjust scope if necessary, and if 9.0-ify if appropriate?

@dimitrieh FYI: We are trying to move in this direction. This being the absolute MVP according to @smcgivern

added ~874211 ~13921 labels

As these comments won't have a position, I think this is the same implementation as https://gitlab.com/gitlab-org/gitlab-ce/issues/20390!

@cperessini is that issue still accurate?

mentioned in issue gitlab-foss#2641 (closed)

As these comments won't have a position, I think this is the same implementation as gitlab-foss#20390 (closed)!

yep looks like we can cover both issues by allowing comment on file (both text and image)

@cperessini + @dimitrieh : Could you confirm the design of this? Would we simply have it below the file (text and image) as in https://gitlab.com/gitlab-org/gitlab-ce/issues/20390?

@smcgivern @dzaporozhets : I'll make commenting on text files as a stretch goal for this issue. So whoever works on it can focus on image files first, and if it makes sense, include text files too.

changed title from View and add comments to image in merge request diff to View and add comments to file in merge request diff

added diff in GitLab FOSS label

@victorwu @dimitrieh I think the same design will work. The comments will appear at the top, between the header and the image.

@smcgivern : Making this 9.0. Change the milestone and/or label if you disagree.

changed milestone to %9.0 in gitlab-foss

added Deliverable in GitLab FOSS label

@victorwu I think that this is Stretch in GitLab FOSS at best, I'm afraid.

added gitlab-foss~~992792 and removed gitlab-foss~~992791 labels

I think the same design will work. The comments will appear at the top, between the header and the image.

@cperessini @victorwu I think not for the following reasons:

Code-files/diffs can become extraordinarily long, normal files do not (except images with an extreme aspect ratio, but i believe those are scaled to some max as well)
Diffs can hold text content within the text content, while normal files do not. We have to see for example the image in order to know what to comment on it, otherwise it would be the same as commenting above a line of code instead of under it.
Image files, have a diff filtering mechanism UI at the bottom with: "onion skin, side-by-side etc. These controls should be near the discussion UI imo

Do you agree? If so I will update the description to reflect it :)

@victorwu on reflection, I don't think we'll get to this in 9.0 if it's Stretch in GitLab FOSS, so let's make it 9.1 instead. Sorry about that!

removed Stretch in GitLab FOSS label

changed milestone to %9.1 in gitlab-foss

@dimitrieh I originally placed the file-level comments at the bottom, but there was some discussion on gitlab-foss#20390 (closed) and we changed it to the top. Could you read https://gitlab.com/gitlab-org/gitlab-ce/issues/20390#note_14394891 and the follow-up comments to see if you agree? Thanks!

@cperessini I see.. i still think its a little weird, although I am probably okey with it.

My main problem with it (and there is not a really good solution to it) is that both design directions have a different assumption:

file comments at top: It assumes there already is conversation on the file.
file comments at bottom: It assumes there is no conversation on the file yet.

Why do i think it makes these assumptions: because there are various comments, like: if the comments are at the top, the user can then decide if they want to view the file contents yes or no. While if there are no comments yet and the user will want to comment on the file, they will first need to view the entire file (for example in the case its an image) in order to see if its worth their while in most cases.

So as there probably is no perfect solution, we have to see which one holds the most pro's vs the cons..

I think that having the comments at the top will have the most pro's:

less scrolling needed in general as comments are at top.
Works well with code diffs, as they can become really long. And have text as content..so you would have to filter visually when text stops vs when file comments begin, while the other way around you would know instantly)
Less viewing of file needed if comments make it not needed

So yes, let's still go with comments on top :) and keep the image diffing options at the bottom, as for those you will always first need to see the image.

Thanks @dimitrieh @cperessini . @smcgivern said this won't make it into 9.0. Let's not even assume it will be 9.1 either. We will make that decision when we are closer.

changed milestone to %Backlog in gitlab-foss

mentioned in issue gitlab-foss#20390 (closed)

changed milestone to %9.1 in gitlab-foss

added Deliverable in GitLab FOSS label

@dimitrieh @cperessini : Chatted with @smcgivern . This is going in for 9.1.

What do you think is the final design? Keep comments at the top of the file, for both image and text files?

I think that commenting at the top is weird for images, and commenting at the bottom is weird for text files (because you're more likely to be commenting on the file name).

I don't have a strong opinion on this one. I'll let you folks battle it out.

added ~19492 label

removed ~19492 label

@cperessini There is a question as to whether commenting will be on top/bottom for text vs images. It looks like you and @dimitrieh agreed on top for both.

I see an argument for keeping both file types consistent and keeping the comment functionality on the top. This allows users to know where to find this feature for all file types and it doesn't jump around on them.

I also see the argument that it is strange to see comments above the images for image files.

I think we should default to consistency and keep comments in the same place, regardless of file type. If we get feedback that it is strange for images, we can readdress later.

I believe then that the description here is up to date and ready for implementation!

Perhaps a bit late to the game, but any chance that resolvable discussions could also be added at the file level, similar to gitlab-foss#24378 (closed)?

@joshuabaergen Yep! Comments at the file level will also be resolvable :)

@tauriedavis that sounds good. I think another argument for placing the comments at the top is that you access the comment button by hovering over the file header.

Thanks @cperessini + @tauriedavis : Updated the description to be clear that the new comments are resolve-able.

added frontend in GitLab FOSS label

added gitlab-foss~~992792 and removed gitlab-foss~~992791 labels

Sorry folks for the last minute change. We are making this a Stretch for 9.1, as we didn't account for resources. My mistake.

Nothing is changed with scope and requirements. Sorry @tauriedavis @cperessini for the disruption. Hopefully it's minimal since most the primary UX is essentially done.

@smcgivern @jschatz1 : Go ahead and do your resource planning accordingly. Thanks!

We are making this a Stretch for 9.1, as we didn't account for resources. My mistake.

You're too kind, it was definitely a joint mistake

@smcgivern @victorwu - who is the best person to discuss backend changes for this? I had a quick look through existing notes, and based on some curling you can't link a note to a file without Line numbers. I can make some changes, but would be good to validate early

changed milestone to %9.2 in gitlab-foss

added gitlab-foss~~992791 and removed gitlab-foss~~992792 labels

@psimyn you can't! This has been moved to 9.2, at which point someone from the backend team will be available to help you. Is it possible to pick up another issue instead?