Commit ab3efb66 authored by Jacob Schatz's avatar Jacob Schatz

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
parents 74725da1 2872c752
...@@ -161,8 +161,10 @@ v 8.12.0 (unreleased) ...@@ -161,8 +161,10 @@ v 8.12.0 (unreleased)
- Fix Gitlab::Popen.popen thread-safety issue - Fix Gitlab::Popen.popen thread-safety issue
- Add specs to removing project (Katarzyna Kobierska Ula Budziszewska) - Add specs to removing project (Katarzyna Kobierska Ula Budziszewska)
- Clean environment variables when running git hooks - 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 Import/Export issues importing protected branches and some specific models
- Fix non-master branch readme display in tree view - Fix non-master branch readme display in tree view
- Add UX improvements for merge request version diffs
v 8.11.6 v 8.11.6
- Fix unnecessary horizontal scroll area in pipeline visualizations. !6005 - Fix unnecessary horizontal scroll area in pipeline visualizations. !6005
......
...@@ -373,11 +373,40 @@ ...@@ -373,11 +373,40 @@
.mr-version-controls { .mr-version-controls {
background: $background-color; background: $background-color;
padding: $gl-btn-padding; border-bottom: 1px solid $border-color;
color: $gl-placeholder-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 { .fa-info-circle {
color: $gl-dark-link-color; color: $orange-normal;
padding-right: 5px;
} }
} }
......
- diff_notes_disabled = (@merge_request_diff.latest? && !!@start_sha) if @merge_request_diff
- discussion = local_assigns.fetch(:discussion, nil) - discussion = local_assigns.fetch(:discussion, nil)
- if current_user - if current_user
%jump-to-discussion{ "inline-template" => true, ":discussion-id" => "'#{discussion.try(:id)}'" } %jump-to-discussion{ "inline-template" => true, ":discussion-id" => "'#{discussion.try(:id)}'" }
...@@ -5,5 +6,6 @@ ...@@ -5,5 +6,6 @@
%button.btn.btn-default.discussion-next-btn.has-tooltip{ "@click" => "jumpToNextUnresolvedDiscussion", %button.btn.btn-default.discussion-next-btn.has-tooltip{ "@click" => "jumpToNextUnresolvedDiscussion",
title: "Jump to next unresolved discussion", title: "Jump to next unresolved discussion",
"aria-label" => "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") = custom_icon("next_discussion")
...@@ -5,7 +5,7 @@ ...@@ -5,7 +5,7 @@
- unless diff_file.submodule? - unless diff_file.submodule?
.file-actions.hidden-xs .file-actions.hidden-xs
- if blob_text_viewable?(blob) - 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') = icon('comment')
\ \
......
- if @merge_request_diffs.size > 1 - if @merge_request_diffs.size > 1
.mr-version-controls .mr-version-controls
Changes between %div.mr-version-menus-container.content-block
%span.dropdown.inline.mr-version-dropdown Changes between
%a.btn-link.dropdown-toggle{ data: {toggle: :dropdown} } %span.dropdown.inline.mr-version-dropdown
%strong %a.dropdown-toggle.btn.btn-default{ data: {toggle: :dropdown} }
- if @merge_request_diff.latest? %span
latest version - if @merge_request_diff.latest?
- else latest version
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)}
- else - else
#{@merge_request.target_branch} version #{version_index(@merge_request_diff)}
%span.caret %span.caret
%ul.dropdown-menu.dropdown-menu-selectable %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 %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 %strong
- if merge_request_diff.latest? - if merge_request_diff.latest?
latest version latest version
...@@ -44,17 +25,46 @@ ...@@ -44,17 +25,46 @@
version #{version_index(merge_request_diff)} version #{version_index(merge_request_diff)}
.monospace #{short_sha(merge_request_diff.head_commit_sha)} .monospace #{short_sha(merge_request_diff.head_commit_sha)}
%small %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) = 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 - if @merge_request_diff.base_commit_sha
%strong and
#{@merge_request.target_branch} (base) %span.dropdown.inline.mr-version-compare-dropdown
.monospace #{short_sha(@merge_request_diff.base_commit_sha)} %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 - unless @merge_request_diff.latest? && !@start_sha
.prepend-top-10 .comments-disabled-notif.content-block
= icon('info-circle') = icon('info-circle')
- if @start_sha - if @start_sha
Comments are disabled because you're comparing two versions of this merge request. Comments are disabled because you're comparing two versions of this merge request.
- else - else
Comments are disabled because you're viewing an old version of this merge request. 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'
...@@ -21,7 +21,7 @@ feature 'Merge Request versions', js: true, feature: true do ...@@ -21,7 +21,7 @@ feature 'Merge Request versions', js: true, feature: true do
describe 'switch between versions' do describe 'switch between versions' do
before do before do
page.within '.mr-version-dropdown' do page.within '.mr-version-dropdown' do
find('.btn-link').click find('.btn-default').click
click_link 'version 1' click_link 'version 1'
end end
end end
...@@ -42,12 +42,12 @@ feature 'Merge Request versions', js: true, feature: true do ...@@ -42,12 +42,12 @@ feature 'Merge Request versions', js: true, feature: true do
describe 'compare with older version' do describe 'compare with older version' do
before do before do
page.within '.mr-version-compare-dropdown' do page.within '.mr-version-compare-dropdown' do
find('.btn-link').click find('.btn-default').click
click_link 'version 1' click_link 'version 1'
end end
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 page.within '.mr-version-compare-dropdown' do
expect(page).to have_content 'version 1' expect(page).to have_content 'version 1'
end end
...@@ -64,5 +64,13 @@ feature 'Merge Request versions', js: true, feature: true do ...@@ -64,5 +64,13 @@ feature 'Merge Request versions', js: true, feature: true do
it 'show diff between new and old version' do it 'show diff between new and old version' do
expect(page).to have_content '4 changed files with 15 additions and 6 deletions' expect(page).to have_content '4 changed files with 15 additions and 6 deletions'
end 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
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment