Merge branch 'merge-request-push-compare-ui' into 'master'

Frontend for Merge Request Diff

This merge request improves the UX for the merge request diff feature which was recently implemented here (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127). Specifically, it styles various parts of the diff feature to match the designs, it disables comment-related buttons in states where comments are disabled, and it adds a 'Show latest version' button for convenience.

## Are there points in the code the reviewer needs to double check?

I could use feedback on this MR's fidelity to the design.  

## Why was this MR needed?

Neccessary styling improvements for basic UX of this feature, and enabled comment buttons are not functional and thus need to be disabled in certain states.

## Screenshots (if relevant)

![57dd0755f0b14342305909](/uploads/318a44a3bc8b7fc5c9c6ef92ba92e511/57dd0755f0b14342305909.gif)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/21427
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6127

cc: @jschatz1 

See merge request !6343

6 files changed, 99 insertions, 48 deletions

diff --git a/CHANGELOG b/CHANGELOG
index e14b52f2b3c..3e2fd3ee15e 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -153,8 +153,10 @@ v 8.12.0 (unreleased)
   - Fix Gitlab::Popen.popen thread-safety issue
   - Add specs to removing project (Katarzyna Kobierska Ula Budziszewska)
   - Clean environment variables when running git hooks
+  - Add UX improvements for merge request version diffs
   - Fix Import/Export issues importing protected branches and some specific models
   - Fix non-master branch readme display in tree view
+  - Add UX improvements for merge request version diffs
 
 v 8.11.6
   - Fix unnecessary horizontal scroll area in pipeline visualizations. !6005
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 96c06086867..926247e5e87 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -373,11 +373,40 @@
 
 .mr-version-controls {
   background: $background-color;
-  padding: $gl-btn-padding;
-  color: $gl-placeholder-color;
+  border-bottom: 1px solid $border-color;
+  color: $gl-text-color;
+
+  .mr-version-menus-container {
+    display: -webkit-flex;
+    display: flex;
+    -webkit-align-items: center;
+    align-items: center;
+    padding: 16px;
+  }
+
+  .comments-disabled-notif {
+    padding: 10px 16px;
+    .btn {
+      margin-left: 5px;
+    }
+  }
+
+  .mr-version-dropdown,
+  .mr-version-compare-dropdown {
+    margin: 0 7px;
+  }
+
+  .comments-disabled-notif {
+    border-top: 1px solid $border-color;
+  }
+
+  .dropdown-title {
+    color: $gl-text-color;
+  }
 
-  a.btn-link {
-    color: $gl-dark-link-color;
+  .fa-info-circle {
+    color: $orange-normal;
+    padding-right: 5px;
   }
 }
 
diff --git a/app/views/discussions/_jump_to_next.html.haml b/app/views/discussions/_jump_to_next.html.haml
index 69bd416c4de..da970792b4d 100644
--- a/app/views/discussions/_jump_to_next.html.haml
+++ b/app/views/discussions/_jump_to_next.html.haml
@@ -1,3 +1,4 @@
+- diff_notes_disabled = (@merge_request_diff.latest? && !!@start_sha) if @merge_request_diff
 - discussion = local_assigns.fetch(:discussion, nil)
 - if current_user
   %jump-to-discussion{ "inline-template" => true, ":discussion-id" => "'#{discussion.try(:id)}'" }
@@ -5,5 +6,6 @@
       %button.btn.btn-default.discussion-next-btn.has-tooltip{ "@click" => "jumpToNextUnresolvedDiscussion",
         title: "Jump to next unresolved discussion",
         "aria-label" => "Jump to next unresolved discussion",
-        data: { container: "body" } }
+        data: { container: "body" },
+        disabled: diff_notes_disabled }
         = custom_icon("next_discussion")
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index ad2eb3e504f..1a51ccd4c7d 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -5,7 +5,7 @@
     - unless diff_file.submodule?
       .file-actions.hidden-xs
         - if blob_text_viewable?(blob)
-          = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip btn-file-option', title: "Toggle comments for this file" do
+          = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip btn-file-option', title: "Toggle comments for this files", disabled: @diff_notes_disabled do
             = icon('comment')
           \
 
diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml
index 00287f2d245..8f7b5d1543e 100644
--- a/app/views/projects/merge_requests/show/_versions.html.haml
+++ b/app/views/projects/merge_requests/show/_versions.html.haml
@@ -1,42 +1,23 @@
 - if @merge_request_diffs.size > 1
   .mr-version-controls
-    Changes between
-    %span.dropdown.inline.mr-version-dropdown
-      %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
-        %strong
-          - if @merge_request_diff.latest?
-            latest version
-          - else
-            version #{version_index(@merge_request_diff)}
-        %span.caret
-      %ul.dropdown-menu.dropdown-menu-selectable
-        - @merge_request_diffs.each do |merge_request_diff|
-          %li
-            = link_to merge_request_version_path(@project, @merge_request, merge_request_diff), class: ('is-active' if merge_request_diff == @merge_request_diff) do
-              %strong
-                - if merge_request_diff.latest?
-                  latest version
-                - else
-                  version #{version_index(merge_request_diff)}
-              .monospace #{short_sha(merge_request_diff.head_commit_sha)}
-              %small
-                #{number_with_delimiter(merge_request_diff.commits.count)} #{'commit'.pluralize(merge_request_diff.commits.count)},
-                = time_ago_with_tooltip(merge_request_diff.created_at)
-
-    - if @merge_request_diff.base_commit_sha
-      and
-      %span.dropdown.inline.mr-version-compare-dropdown
-        %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
-          %strong
-            - if @start_sha
-              version #{version_index(@start_version)}
+    %div.mr-version-menus-container.content-block
+      Changes between
+      %span.dropdown.inline.mr-version-dropdown
+        %a.dropdown-toggle.btn.btn-default{ data: {toggle: :dropdown} }
+          %span
+            - if @merge_request_diff.latest?
+              latest version
             - else
-              #{@merge_request.target_branch}
+              version #{version_index(@merge_request_diff)}
           %span.caret
         %ul.dropdown-menu.dropdown-menu-selectable
-          - @comparable_diffs.each do |merge_request_diff|
+          .dropdown-title
+            %span Version:
+            %button.dropdown-title-button.dropdown-menu-close
+              %i.fa.fa-times.dropdown-menu-close-icon
+          - @merge_request_diffs.each do |merge_request_diff|
             %li
-              = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff, merge_request_diff.head_commit_sha), class: ('is-active' if merge_request_diff == @start_version) do
+              = link_to merge_request_version_path(@project, @merge_request, merge_request_diff), class: ('is-active' if merge_request_diff == @merge_request_diff) do
                 %strong
                   - if merge_request_diff.latest?
                     latest version
@@ -44,17 +25,46 @@
                     version #{version_index(merge_request_diff)}
                 .monospace #{short_sha(merge_request_diff.head_commit_sha)}
                 %small
+                  #{number_with_delimiter(merge_request_diff.commits.count)} #{'commit'.pluralize(merge_request_diff.commits.count)},
                   = time_ago_with_tooltip(merge_request_diff.created_at)
-          %li
-            = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless @start_sha) do
-              %strong
-                #{@merge_request.target_branch} (base)
-              .monospace #{short_sha(@merge_request_diff.base_commit_sha)}
+
+      - if @merge_request_diff.base_commit_sha
+        and
+        %span.dropdown.inline.mr-version-compare-dropdown
+          %a.btn.btn-default.dropdown-toggle{ data: {toggle: :dropdown} }
+            %span
+              - if @start_sha
+                version #{version_index(@start_version)}
+              - else
+                #{@merge_request.target_branch}
+            %span.caret
+          %ul.dropdown-menu.dropdown-menu-selectable
+            .dropdown-title
+              %span Compared with:
+              %button.dropdown-title-button.dropdown-menu-close
+                %i.fa.fa-times.dropdown-menu-close-icon
+            - @comparable_diffs.each do |merge_request_diff|
+              %li
+                = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff, merge_request_diff.head_commit_sha), class: ('is-active' if merge_request_diff == @start_version) do
+                  %strong
+                    - if merge_request_diff.latest?
+                      latest version
+                    - else
+                      version #{version_index(merge_request_diff)}
+                  .monospace #{short_sha(merge_request_diff.head_commit_sha)}
+                  %small
+                    = time_ago_with_tooltip(merge_request_diff.created_at)
+            %li
+              = link_to merge_request_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless @start_sha) do
+                %strong
+                  #{@merge_request.target_branch} (base)
+                .monospace #{short_sha(@merge_request_diff.base_commit_sha)}
 
     - unless @merge_request_diff.latest? && !@start_sha
-      .prepend-top-10
+      .comments-disabled-notif.content-block
         = icon('info-circle')
         - if @start_sha
           Comments are disabled because you're comparing two versions of this merge request.
         - else
           Comments are disabled because you're viewing an old version of this merge request.
+        = link_to 'Show latest version', merge_request_version_path(@project, @merge_request, @merge_request_diff), class: 'btn btn-sm'
diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb
index 9e759de3752..22d9e42119d 100644
--- a/spec/features/merge_requests/merge_request_versions_spec.rb
+++ b/spec/features/merge_requests/merge_request_versions_spec.rb
@@ -21,7 +21,7 @@ feature 'Merge Request versions', js: true, feature: true do
   describe 'switch between versions' do
     before do
       page.within '.mr-version-dropdown' do
-        find('.btn-link').click
+        find('.btn-default').click
         click_link 'version 1'
       end
     end
@@ -42,12 +42,12 @@ feature 'Merge Request versions', js: true, feature: true do
   describe 'compare with older version' do
     before do
       page.within '.mr-version-compare-dropdown' do
-        find('.btn-link').click
+        find('.btn-default').click
         click_link 'version 1'
       end
     end
 
-    it 'should has correct value in the compare dropdown' do
+    it 'should have correct value in the compare dropdown' do
       page.within '.mr-version-compare-dropdown' do
         expect(page).to have_content 'version 1'
       end
@@ -64,5 +64,13 @@ feature 'Merge Request versions', js: true, feature: true do
     it 'show diff between new and old version' do
       expect(page).to have_content '4 changed files with 15 additions and 6 deletions'
     end
+
+    it 'should return to latest version when "Show latest version" button is clicked' do
+      click_link 'Show latest version'
+      page.within '.mr-version-dropdown' do
+        expect(page).to have_content 'latest version'
+      end
+      expect(page).to have_content '8 changed files'
+    end
   end
 end