Improve merge request version switch/compare dropdown

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/assets/stylesheets/pages/merge_requests.scss

@@ -375,7 +375,7 @@
   }
 }
 
-.mr-version-switch {
+.mr-version-controls {
   background: $background-color;
   padding: $gl-btn-padding;
   color: $gl-placeholder-color;
@@ -383,6 +383,10 @@
   a.btn-link {
     color: $gl-dark-link-color;
   }
+
+  .compare-dots {
+    margin: 0 $btn-side-margin;
+  }
 }
 
 .merge-request-details {

app/helpers/git_helper.rb

@@ -2,4 +2,8 @@ module GitHelper
   def strip_gpg_signature(text)
     text.gsub(/-----BEGIN PGP SIGNATURE-----(.*)-----END PGP SIGNATURE-----/m, "")
   end
+
+  def short_sha(text)
+    text[0...8]
+  end
 end

app/views/projects/merge_requests/show/_versions.html.haml

 - merge_request_diffs = @merge_request.merge_request_diffs.select_without_diff
+- compareable_diffs = merge_request_diffs.select { |diff| diff.id < @merge_request_diff.id }
 
 - if merge_request_diffs.size > 1
-  .mr-version-switch
-    Version:
-    %span.dropdown.inline
+  .mr-version-controls
+    Version
+    %span.dropdown.inline.mr-version-dropdown
       %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
         %strong.monospace<
           - if @merge_request_diff.latest?
-            #{"latest"}
+            Latest: #{short_sha(@merge_request_diff.head_commit_sha)}
           - else
-            #{@merge_request_diff.head_commit.short_id}
+            #{short_sha(@merge_request_diff.head_commit_sha)}
         %span.caret
       %ul.dropdown-menu.dropdown-menu-selectable
-        - merge_request_diffs.each do |merge_request_diff|
+        - merge_request_diffs.each_with_index do |merge_request_diff, i|
           %li
             = link_to diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request, diff_id: merge_request_diff.id), class: ('is-active' if merge_request_diff == @merge_request_diff) do
               %strong.monospace
+                - if i.zero?
+                  Latest:
+                - else
+                  #{merge_request_diffs.size - i}.
                 #{merge_request_diff.head_commit.short_id}
               %br
               %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)
 
-    %span.prepend-left-default
-      Compared with:
-      %span.dropdown.inline
-        %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
-          %strong.monospace<
-            - if params[:start_sha].present?
-              #{params[:start_sha][0...8]}
-            - else
-              #{"base"}
-          %span.caret
-        %ul.dropdown-menu.dropdown-menu-selectable
-          - merge_request_diffs.each do |merge_request_diff|
-            - next if merge_request_diff.id >= @merge_request_diff.id
-            %li
-              = link_to mr_version_path(@project, @merge_request, @merge_request_diff, merge_request_diff.head_commit_sha), class: ('is-active' if merge_request_diff.head_commit_sha == params[:start_sha]) do
-                %strong.monospace
-                  #{merge_request_diff.head_commit.short_id}
-                %br
-                %small
-                  = time_ago_with_tooltip(merge_request_diff.created_at)
+    %span.compare-dots ...
+
+    Compared with
+    %span.dropdown.inline.mr-version-compare-dropdown
+      %a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} }
+        %strong.monospace<
+          - if params[:start_sha].present?
+            #{short_sha(params[:start_sha])}
+          - else
+            Base: #{short_sha(@merge_request_diff.base_commit_sha)}
+        %span.caret
+      %ul.dropdown-menu.dropdown-menu-selectable
+        - compareable_diffs.each_with_index do |merge_request_diff, i|
           %li
-            = link_to mr_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless params[:start_sha].present?) do
+            = link_to mr_version_path(@project, @merge_request, @merge_request_diff, merge_request_diff.head_commit_sha), class: ('is-active' if merge_request_diff.head_commit_sha == params[:start_sha]) do
               %strong.monospace
-                base
-
+                #{compareable_diffs.size - i}. #{short_sha(merge_request_diff.head_commit_sha)}
+              %br
+              %small
+                = time_ago_with_tooltip(merge_request_diff.created_at)
+        %li
+          = link_to mr_version_path(@project, @merge_request, @merge_request_diff), class: ('is-active' unless params[:start_sha].present?) do
+            %strong.monospace
+              Base: #{short_sha(@merge_request_diff.base_commit_sha)}
 
     - unless @merge_request_diff.latest? && params[:start_sha].blank?
-      .pull-right
+      .prepend-top-10
         = icon('info-circle')
-        - if params[:start_sha].present?
-          Comments are disabled when compare with version different from base
-        - else
-          This version is not the latest one. Comments are disabled
+        Comments are disabled while viewing outdated merge versions or comparing to versions other than base.

spec/features/merge_requests/merge_request_versions_spec.rb

@@ -11,7 +11,7 @@ feature 'Merge Request versions', js: true, feature: true do
   end
 
   it 'show the latest version of the diff' do
-    page.within '.mr-version-switch' do
+    page.within '.mr-version-dropdown' do
       expect(page).to have_content 'Version: latest'
     end
 
@@ -20,15 +20,32 @@ feature 'Merge Request versions', js: true, feature: true do
 
   describe 'switch between versions' do
     before do
-      page.within '.mr-version-switch' do
+      page.within '.mr-version-dropdown' do
         find('.btn-link').click
         click_link '6f6d7e7e'
       end
     end
 
     it 'should show older version' do
-      page.within '.mr-version-switch' do
-        expect(page).to have_content 'Version: 6f6d7e7e'
+      page.within '.mr-version-dropdown' do
+        expect(page).to have_content '6f6d7e7e'
+      end
+
+      expect(page).to have_content '5 changed files'
+    end
+  end
+
+  describe 'compare with older version' do
+    before do
+      page.within '.mr-version-compare-dropdown' do
+        find('.btn-link').click
+        click_link '6f6d7e7e'
+      end
+    end
+
+    it 'should show older version' do
+      page.within '.mr-version-compare-dropdown' do
+        expect(page).to have_content '6f6d7e7e'
       end
 
       expect(page).to have_content '5 changed files'