Navigating files in merge request diff

added ~874211 ~19492 labels

@dimitrieh : FYI since you were looking at octotree stuff earlier.

changed milestone to %Backlog

@victorwu I agree this is painful. Which diff view do you use? I use side-by-side view, so I don't exactly have a lot of horizontal space spare on my MBP screen

@smcgivern : Good point. Although I don't want to to presume that's something that UX cannot solve. So at least we agree there's a problem.

added diff ~13921 labels

@victorwu I do have faith, it's just that my screen (now that the top bar is no longer fixed) is pretty much completely showing code when I'm reviewing an MR, and it's hard to make a case for something to intrude on that.

added ~897282 label

Easy fix would be to render just a list of files and let the user:

click on one to review that file
make files as reviewed
switch from file-to-file easily

How that would work well with the current implementation is the challenge here.

@tauriedavis : Any obvious / quick UX improvements you could think of here to help alleviate pain?

I can't provide any quick fixes but I can provide some UX insight, into why reviewing merge requests with lots of changes, is a challenge, not only in GitLab, but with others as well.

Switching to a tree is a good start, but as you'll soon learn, it's far from the final answer, as navigation can still be cumbersome. And that's because the tree by itself, doesn't address all of the key points, for being able to consume large amounts of information efficiently.

When dealing with lots of information, you have to take the following into consideration:

Orientation
Filtration
Navigation

The tree will help with points 3 and depending on the number of changes, can help with points 1. But for the most part, if there are lots of changes, it can only really help with navigation.

Orientation

With orientation, your focus is to provide users with information, that can help them find their bearings. And what to provide, will depend solely on what you can provide. There is a reason why I developed an indexing engine, and that's because I knew, the "more indexed data", the "more analytics/orientation information". And now that I'm indexing merge requests, I can provide both high level and low level analytics, that can help reviewers:

Better understand how complex the code review will be.
Better understand where to focus their attention.

In a nutshell, provide reviewers with analytics/orientation information, that can help them divide and conquer large/complex diffs.

In the screen shot below, you can see an example, of high level orientation information:

With this high level view, reviewers can:

Quickly track related merge requests, all from a single location. For Enterprise this is important, since it's not uncommon to have cross repository dependencies. So by grouping logical reviews, you add orientation information by association.
Quickly see how many files were changed and how many have been reviewed.
Quickly gauge review complexity.
Quickly identify potential file change conflicts with other active/recently merged merge requests. To better understand why this would be important, take a look at https://gitlab.com/gitlab-org/gitlab-ee/issues/1543, which goes over a use case.

Something that I'm not doing, but should, would be to implement something like https://gitlab.com/gitlab-org/gitlab-ce/issues/29796 The dependency tree is something GitLab should probably look at, as I think it has the potential to be a foundation for so much more. For example it can be used to help create better notification rules and help develop smarter searches.

So that's an example, of high level orientation information. Below is an example of low level orientation information.

In the screenshot above, you can see the merge request changes, that is referenced in this issue. And as the analytics shows, this merge request, should have been easy to review.

The churn metrics shows 30 of the 35 changes, had 0-25 lines of code churn.
The vast majority of files in this merge request, were below 200 lines of code. The bigger the file => the more grokking => the more complex.
Only 4 files were edited more than 5 times, only 3 files were edited more than 20 times and only 2 files were edited more than 50 times. You basically only had to review 2 files on a continuous basis.

For this merge request, switching to a tree, would make it easier to navigate, but without any indicators (change frequency, what has been reviewed, etc.), you would still have a hard time determining what needs to be reviewed again. And you'll probably waste a bunch of time reviewing the same thing, over and over again. This merge request is a good example of how using a tree is a good thing, but is far from the final solution.

Filtration

Filtration is all about reducing noise and if you had a proper filtration system in place, you would have been able to prune the above merge request tree, so it looked like this:

Which as you can see, would have been trivial to navigate/continuously review.

The main take away, when thinking about filtration is, you can only filter what you know. The more information you can capture, the more filtering options you can provide.

Navigation

For navigation, there are a lot of things to consider, but since this post has gone on longer than I would have liked, I'll just reiterate that a tree is a good start, but you'll also need to focus on providing auxiliary information, to help with the navigation process. The merge request that is referenced in this issue should have been trivial to review/navigate and would be a good baseline for testing future enhancements against.

mentioned in issue #25426 (closed)

I don't have any quick magic fixes- this will require some UX exploration. Job points out some quick fixes but I imagine people will surely be upset if they have to click each file to review.

cc @hazelyang since you did some great work with adding the filetree here https://gitlab.com/gitlab-org/gitlab-ce/issues/13723#note_16567419

assigned to @hazelyang

mentioned in issue #29268 (closed)

mentioned in issue #26560 (closed)

mentioned in issue #32023 (moved)

I'd go for an extra sidebar (on the left) showing all the files (preferably with a tree-view-like folder structure) and for great ux have an :active following the file in view (would tie in with the code handling #32023 (moved))

Considering that files tree would take a lot of space on the screen and probably make the screen look messy (if we have the review mode feature in the future on EE gitlab-ee#1984), especially in small size screens, maybe we can utilize the elements that we already had to solve the problem.

Currently, the user can link to different files through the dropdown of Showing X changed files with Y additions.... However, if the user scrolls down the page, the dropdown will be useless.

Proposed design

The Showing X changed files with Y additions... (Screen 01) will be fixed on the top of the screen like tabs bar (Screen 02) after scrolling down.

Screen 01

X additions and Y deletions will be shown as +X -Y OOO0.(Looks like the image below)

+X -Y OOO0 is graphic. It would be easier and faster for the user to understand than the text.

Screen 02

After clicking X changed files, a dropdown will appear, and then the user can link to different files.

Users can also search the file in the dropdown.

Screen 03

Seems like a nice improvement to me @hazelyang!

I would still allow that dropdown to be fixed to the left side (in a compacter way) if the screen is wide enough; and give it a more tree-view like structure so there's better oversight of what changed (and less repetition of folder names, which saves screen-width)

p.s. I'd be fine with EE only option for this

@hazelyang . That's a great improvement.

What's the benefit of replacing with a graphic vs using just regular text? We do have space horizontally right? And what do the circles represent?

@hazelyang @smcgivern : Do you think it would be good to indicate in the dropdown how many lines were changed in a given file? The use case I'm thinking is that you arrive at this merge request diff, and you may want to look at the file with most changes first or figure out where is the most impactful change. Just looking at the filename is not enough right? (Probably should be a separate issue. But just to keep the design discussion going.)

@victorwu yeah, that's an interesting idea. I think that would make sense, if it's not too heavyweight.

@hazelyang one concern I do have about the design, actually, is that highlighting an added file in green for the entire row seems to give it too much prominence. (We already do this, you didn't add that.) For instance, in this screenshot:

My eye is drawn to the two added files, app/assets/javascripts/integrations/index.js and app/assets/javascripts/integrations/integration_settings_form.js. But there are more changes in app/assets/javascripts/flash.js than in the first of those, so maybe making that more muted would make sense, until we can add some indication of lines changed by file?

mentioned in issue #30091 (closed)

changed milestone to %Next 2-3 months

@markpundsack : Did you intend to punt this to %Next 2-3 months ? I'll move it back to Backlog, since I want to do all the Backlog grooming (i.e. pull out and put them in the right milestones) for Discussion issues all at once sometime in the next few days.

changed milestone to %Backlog

@victorwu Hmm... not sure. I was trying to add that milestone to all issues with the ~"coming soon" label, but I don't see that label on here, so not sure why that would have happened. I know nothing about this issue, so certainly wasn't consciously scheduling it. Sorry!

@markpundsack the coming soon label is actually on here...added 2 months ago by @victorwu

changed milestone to %Next 2-3 months

What's the benefit of replacing with a graphic vs using just regular text? We do have space horizontally right? And what do the circles represent?

@victorwu I think people can figure out the information with a graphic more quickly. Another reason is that there are already a lot of text on the screen, using a graphic would make the information obvious.

The circles give a quick view that which the "additions" or the "deletions" is more.

one concern I do have about the design, actually, is that highlighting an added file in green for the entire row seems to give it too much prominence. (We already do this, you didn't add that.)

@smcgivern Agree. We could only color the "Plus" icon with green.

Updated mockup:

Thanks @hazelyang . That looks great. For the changed files, will there be a - symbol on the left if an entire file is deleted? The +/- look okay. But I don't like that circle/disk thing, unless it represents something specific. But we can worry about that in future issues and you are redesigning a bunch of icons anyways right?

As for the red and green and grey circles on the right: That looks strange to me. I totally agree that a graphic gives nice summarizing info. But in this case, you already have +112 and -82. That represents changes in lines right? And so the circles represents what? Changes in file counts? And the number of circles seems arbitrary. So:

What are you trying to communicate? Relative proportions of files changes?
Why use circles? A simple progress bar style would be obvious to me. If I see circles, I start counting them and try to do math in my brain. I don't think of relative proportions. If the goal is relative proportions, there's better ways to communicate that right?

@victorwu Thanks for the feedback.

For the changed files, will there be a - symbol on the left if an entire file is deleted?

Yes, I think - makes sense.

The +/- look okay. But I don't like that circle/disk thing, unless it represents something specific. But we can worry about that in future issues and you are redesigning a bunch of icons anyways right?

I'll change the icon. Thanks for this feedback. In fact, I don't really understand what the circle/disk icon means. I guess it means a number of additions is equal to a number of deletions in the file, correct?

Why use circles? A simple progress bar style would be obvious to me. If I see circles, I start counting them and try to do math in my brain. I don't think of relative proportions. If the goal is relative proportions, there's better ways to communicate that right?

I think your opinion makes sense. I also do the same thing while seeing circles(or squares) Could you explain how the progress bar will indicate the relative proportions?

I imagine it will do in this way:

I don't do development a lot, so I'm not sure how useful this information would be. @smcgivern : Do you think the following is useful?

The +/- to indicate brand new files and totally removed files seems obvious.
Some type of "in-between" icon to indicate a file was changed. Looking forward to your icon.
The +112 -82 I assume is useful to see at a glance how many lines changed.
@hazelyang I see the circles/progress bar you got from GitHub's design. I don't think we need to copy it if you don't think it's useful at all. @smcgivern : Do you find that useful? I think we can leave this out and consider it only if people think it's useful. I think it's not useful at all.

This is what GitHub has. What is the purpose of the squares?

mentioned in issue #33596 (closed)

I pretty much only care about the file name (not even whether it was deleted, edited, or added), so I'm all for removing information unless we're sure that we need it

@hazelyang : Can we remove the circles/progress bar for this issue then? We can consider it for a future issue but not add to the scope here since these are new elements. I think everything else is great (since these are existing UI components, but just re-styled). If you could update the description I think we are good.

changed the description

@victorwu Sure, I removed it. Updated the description.

added UX ready and removed UX labels

mentioned in commit gitlab-design@b1ae2e7a

Thanks @hazelyang

changed milestone to %9.5

I asked @victorwu and @smcgivern whether the path/filename middle truncation shown in this mockup is done in the backend or in the frontend. The answer was frontend. This can be very helpful for responsive designs in other situations. However, I have another question: is this type of truncation possible outside of the file path case? Like a sentence?

@pedroms to be clear, we don't do it right now, but I think it's more of a frontend concern. I think we can do this anywhere we can mark up a file path as a separate element.

@smcgivern thanks for clearing that up. That was my thought as well.

added Deliverable label

added frontend label

mentioned in issue #33258 (closed)

mentioned in issue #15399 (moved)

mentioned in commit 32fee4f9

mentioned in merge request !13128 (merged)

assigned to @iamphill

mentioned in issue #35846 (moved)

marked this issue as related to #35846 (moved)

mentioned in commit 85582b05

closed via merge request !13128 (merged)

marked this issue as related to #32023 (moved)

marked this issue as related to #36687

marked this issue as related to #36688 (closed)

mentioned in issue #36687

Navigating files in merge request diff

Problem

Proposed solution

Design

Designs

Child items ...

Activity

Proposed design

Admin message

Admin message

Navigating files in merge request diff

Problem

Proposed solution

Design

Relates to

Activity

Proposed design