Commit 596c305e authored by Fatih Acet's avatar Fatih Acet

Merge branch 'less-intrusive-system-note' into 'master'

Less intrusive system note

## What does this MR do?
This MR is for making system notes less intrusive by implementing proposed design
 
## Are there points in the code the reviewer needs to double check?
System notes are reusing in issues, commits and MR discussions. May need to double check to make sure that the design is not breaking anywhere

## Why was this MR needed?
This merge request solve issue #19797 

## Screenshots (if relevant)
**Before:**
![merge-request-before](/uploads/91953598087a613078d80333a43815a7/merge-request-before.png)
**After**
![2016-11-15_17.41.46](/uploads/c809ba53cc34ee83bb439ed741dc39f3/2016-11-15_17.41.46.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)
- [ ] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] 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 it does - 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?
#19797, #23129

See merge request !6755
parents ffc5fc6a 58f78d05
...@@ -125,6 +125,11 @@ ...@@ -125,6 +125,11 @@
// Close any open tooltips // Close any open tooltips
$('.has-tooltip, [data-toggle="tooltip"]').tooltip('destroy'); $('.has-tooltip, [data-toggle="tooltip"]').tooltip('destroy');
}; };
gl.utils.isMetaKey = function(e) {
return e.metaKey || e.ctrlKey || e.altKey || e.shiftKey;
};
})(window); })(window);
}).call(this); }).call(this);
...@@ -12,7 +12,7 @@ ...@@ -12,7 +12,7 @@
var bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; }; var bind = function(fn, me){ return function(){ return fn.apply(me, arguments); }; };
this.Notes = (function() { this.Notes = (function() {
var isMetaKey; const MAX_VISIBLE_COMMIT_LIST_COUNT = 3;
Notes.interval = null; Notes.interval = null;
...@@ -33,6 +33,7 @@ ...@@ -33,6 +33,7 @@
this.resetMainTargetForm = bind(this.resetMainTargetForm, this); this.resetMainTargetForm = bind(this.resetMainTargetForm, this);
this.refresh = bind(this.refresh, this); this.refresh = bind(this.refresh, this);
this.keydownNoteText = bind(this.keydownNoteText, this); this.keydownNoteText = bind(this.keydownNoteText, this);
this.toggleCommitList = bind(this.toggleCommitList, this);
this.notes_url = notes_url; this.notes_url = notes_url;
this.note_ids = note_ids; this.note_ids = note_ids;
this.last_fetched_at = last_fetched_at; this.last_fetched_at = last_fetched_at;
...@@ -46,6 +47,7 @@ ...@@ -46,6 +47,7 @@
this.setPollingInterval(); this.setPollingInterval();
this.setupMainTargetNoteForm(); this.setupMainTargetNoteForm();
this.initTaskList(); this.initTaskList();
this.collapseLongCommitList();
} }
Notes.prototype.addBinding = function() { Notes.prototype.addBinding = function() {
...@@ -81,10 +83,13 @@ ...@@ -81,10 +83,13 @@
$(document).on("click", ".js-add-diff-note-button", this.addDiffNote); $(document).on("click", ".js-add-diff-note-button", this.addDiffNote);
// hide diff note form // hide diff note form
$(document).on("click", ".js-close-discussion-note-form", this.cancelDiscussionForm); $(document).on("click", ".js-close-discussion-note-form", this.cancelDiscussionForm);
// toggle commit list
$(document).on("click", '.system-note-commit-list-toggler', this.toggleCommitList);
// fetch notes when tab becomes visible // fetch notes when tab becomes visible
$(document).on("visibilitychange", this.visibilityChange); $(document).on("visibilitychange", this.visibilityChange);
// when issue status changes, we need to refresh data // when issue status changes, we need to refresh data
$(document).on("issuable:change", this.refresh); $(document).on("issuable:change", this.refresh);
// when a key is clicked on the notes // when a key is clicked on the notes
return $(document).on("keydown", ".js-note-text", this.keydownNoteText); return $(document).on("keydown", ".js-note-text", this.keydownNoteText);
}; };
...@@ -114,9 +119,10 @@ ...@@ -114,9 +119,10 @@
Notes.prototype.keydownNoteText = function(e) { Notes.prototype.keydownNoteText = function(e) {
var $textarea, discussionNoteForm, editNote, myLastNote, myLastNoteEditBtn, newText, originalText; var $textarea, discussionNoteForm, editNote, myLastNote, myLastNoteEditBtn, newText, originalText;
if (isMetaKey(e)) { if (gl.utils.isMetaKey(e)) {
return; return;
} }
$textarea = $(e.target); $textarea = $(e.target);
// Edit previous note when UP arrow is hit // Edit previous note when UP arrow is hit
switch (e.which) { switch (e.which) {
...@@ -156,10 +162,6 @@ ...@@ -156,10 +162,6 @@
} }
}; };
isMetaKey = function(e) {
return e.metaKey || e.ctrlKey || e.altKey || e.shiftKey;
};
Notes.prototype.initRefresh = function() { Notes.prototype.initRefresh = function() {
clearInterval(Notes.interval); clearInterval(Notes.interval);
return Notes.interval = setInterval((function(_this) { return Notes.interval = setInterval((function(_this) {
...@@ -263,6 +265,7 @@ ...@@ -263,6 +265,7 @@
$notesList.append(note.html).syntaxHighlight(); $notesList.append(note.html).syntaxHighlight();
// Update datetime format on the recent note // Update datetime format on the recent note
gl.utils.localTimeAgo($notesList.find("#note_" + note.id + " .js-timeago"), false); gl.utils.localTimeAgo($notesList.find("#note_" + note.id + " .js-timeago"), false);
this.collapseLongCommitList();
this.initTaskList(); this.initTaskList();
this.refresh(); this.refresh();
return this.updateNotesCount(1); return this.updateNotesCount(1);
...@@ -433,9 +436,9 @@ ...@@ -433,9 +436,9 @@
var $form = $(xhr.target); var $form = $(xhr.target);
if ($form.attr('data-resolve-all') != null) { if ($form.attr('data-resolve-all') != null) {
var projectPath = $form.data('project-path') var projectPath = $form.data('project-path');
discussionId = $form.data('discussion-id'), var discussionId = $form.data('discussion-id');
mergeRequestId = $form.data('noteable-iid'); var mergeRequestId = $form.data('noteable-iid');
if (ResolveService != null) { if (ResolveService != null) {
ResolveService.toggleResolveForDiscussion(projectPath, mergeRequestId, discussionId); ResolveService.toggleResolveForDiscussion(projectPath, mergeRequestId, discussionId);
...@@ -844,9 +847,9 @@ ...@@ -844,9 +847,9 @@
return this.notesCountBadge.text(parseInt(this.notesCountBadge.text()) + updateCount); return this.notesCountBadge.text(parseInt(this.notesCountBadge.text()) + updateCount);
}; };
Notes.prototype.resolveDiscussion = function () { Notes.prototype.resolveDiscussion = function() {
var $this = $(this), var $this = $(this);
discussionId = $this.attr('data-discussion-id'); var discussionId = $this.attr('data-discussion-id');
$this $this
.closest('form') .closest('form')
...@@ -855,6 +858,36 @@ ...@@ -855,6 +858,36 @@
.attr('data-project-path', $this.attr('data-project-path')); .attr('data-project-path', $this.attr('data-project-path'));
}; };
Notes.prototype.toggleCommitList = function(e) {
const $element = $(e.target);
const $closestSystemCommitList = $element.siblings('.system-note-commit-list');
$closestSystemCommitList.toggleClass('hide-shade');
};
/**
Scans system notes with `ul` elements in system note body
then collapse long commit list pushed by user to make it less
intrusive.
*/
Notes.prototype.collapseLongCommitList = function() {
const systemNotes = $('#notes-list').find('li.system-note').has('ul');
$.each(systemNotes, function(index, systemNote) {
const $systemNote = $(systemNote);
const headerMessage = $systemNote.find('.note-text').find('p:first').text().replace(':', '');
$systemNote.find('.note-header .system-note-message').html(headerMessage);
if ($systemNote.find('li').length > MAX_VISIBLE_COMMIT_LIST_COUNT) {
$systemNote.find('.note-text').addClass('system-note-commit-list');
$systemNote.find('.system-note-commit-list-toggler').show();
} else {
$systemNote.find('.note-text').addClass('system-note-commit-list hide-shade');
}
});
};
return Notes; return Notes;
})(); })();
......
...@@ -35,11 +35,83 @@ ul.notes { ...@@ -35,11 +35,83 @@ ul.notes {
.system-note { .system-note {
font-size: 14px; font-size: 14px;
padding-top: 10px; padding: 0;
padding-bottom: 10px; clear: both;
background: #fdfdfd;
&.timeline-entry::after {
clear: none;
}
.system-note-message {
text-transform: lowercase;
a {
color: $gl-link-color;
text-decoration: none;
}
}
.timeline-content {
padding: 14px 10px;
}
.note-body {
overflow: hidden;
.system-note-commit-list-toggler {
display: none;
padding: 10px 0 0;
cursor: pointer;
}
.note-text {
& p:first-child {
display: none;
}
&.system-note-commit-list {
max-height: 63px;
overflow: hidden;
display: block;
ul {
margin: 3px 0 3px 15px !important;
li {
font-family: $monospace_font;
font-size: 12px;
}
}
p:first-child {
display: none;
}
&::after {
content: '';
width: 100%;
height: 20px;
position: absolute;
left: 0;
bottom: 50px;
background: linear-gradient(rgba($gray-light, .3) 0, $white-light);
}
&.hide-shade {
max-height: 100%;
overflow: auto;
&::after {
background: transparent;
}
}
}
}
}
.timeline-icon { .timeline-icon {
display: none;
.avatar { .avatar {
visibility: hidden; visibility: hidden;
...@@ -65,6 +137,12 @@ ul.notes { ...@@ -65,6 +137,12 @@ ul.notes {
position: relative; position: relative;
border-bottom: 1px solid $table-border-gray; border-bottom: 1px solid $table-border-gray;
&.note-discussion {
&.timeline-entry {
padding: 14px 10px;
}
}
&.is-editting { &.is-editting {
.note-header, .note-header,
.note-text, .note-text,
...@@ -88,10 +166,8 @@ ul.notes { ...@@ -88,10 +166,8 @@ ul.notes {
overflow: auto; overflow: auto;
word-wrap: break-word; word-wrap: break-word;
@include md-typography; @include md-typography;
// Reset ul style types since we're nested inside a ul already // Reset ul style types since we're nested inside a ul already
@include bulleted-list; @include bulleted-list;
ul.task-list { ul.task-list {
ul:not(.task-list) { ul:not(.task-list) {
padding-left: 1.3em; padding-left: 1.3em;
...@@ -111,6 +187,11 @@ ul.notes { ...@@ -111,6 +187,11 @@ ul.notes {
padding-bottom: 3px; padding-bottom: 3px;
padding-right: 20px; padding-right: 20px;
p {
display: inline;
margin: 0;
}
@media (min-width: $screen-sm-min) { @media (min-width: $screen-sm-min) {
padding-right: 0; padding-right: 0;
} }
...@@ -238,6 +319,10 @@ ul.notes { ...@@ -238,6 +319,10 @@ ul.notes {
} }
} }
.discussion-header {
font-size: 14px;
}
.note-headline-light, .note-headline-light,
.discussion-headline-light { .discussion-headline-light {
color: $notes-light-color; color: $notes-light-color;
......
- expanded = discussion.expanded? - expanded = discussion.expanded?
%li.note.note-discussion.timeline-entry %li.note.note-discussion.timeline-entry
.timeline-entry-inner .timeline-entry-inner
.timeline-icon
= link_to user_path(discussion.author) do
= image_tag avatar_icon(discussion.author), class: "avatar s40"
.timeline-content .timeline-content
.discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } } .discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } }
.discussion-header .discussion-header
...@@ -13,9 +10,7 @@ ...@@ -13,9 +10,7 @@
= icon("chevron-up") = icon("chevron-up")
- else - else
= icon("chevron-down") = icon("chevron-down")
Toggle discussion Toggle discussion
= link_to_member(@project, discussion.author, avatar: false) = link_to_member(@project, discussion.author, avatar: false)
.inline.discussion-headline-light .inline.discussion-headline-light
...@@ -38,8 +33,6 @@ ...@@ -38,8 +33,6 @@
= time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago") = time_ago_with_tooltip(discussion.created_at, placement: "bottom", html_class: "note-created-ago")
= render "discussions/headline", discussion: discussion
.discussion-body.js-toggle-content{ class: ("hide" unless expanded) } .discussion-body.js-toggle-content{ class: ("hide" unless expanded) }
- if discussion.diff_discussion? && discussion.diff_file - if discussion.diff_discussion? && discussion.diff_file
= render "discussions/diff_with_notes", discussion: discussion = render "discussions/diff_with_notes", discussion: discussion
......
...@@ -14,6 +14,9 @@ ...@@ -14,6 +14,9 @@
= note.author.to_reference = note.author.to_reference
- unless note.system - unless note.system
commented commented
- if note.system
%span{class: 'system-note-message'}
= h(note.note_html.downcase.html_safe)
%a{ href: "##{dom_id(note)}" } %a{ href: "##{dom_id(note)}" }
= time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago') = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago')
- unless note.system? - unless note.system?
...@@ -67,7 +70,9 @@ ...@@ -67,7 +70,9 @@
= render 'projects/notes/edit_form', note: note = render 'projects/notes/edit_form', note: note
.note-awards .note-awards
= render 'award_emoji/awards_block', awardable: note, inline: false = render 'award_emoji/awards_block', awardable: note, inline: false
- if note.system
.system-note-commit-list-toggler
Toggle commit list
- if note.attachment.url - if note.attachment.url
.note-attachment .note-attachment
- if note.attachment.image? - if note.attachment.image?
......
---
title: Make system notes less intrusive
merge_request: 6755
author:
...@@ -201,7 +201,7 @@ feature 'Diff notes resolve', feature: true, js: true do ...@@ -201,7 +201,7 @@ feature 'Diff notes resolve', feature: true, js: true do
expect(first('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}") expect(first('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}")
end end
expect(page).to have_content('Last updated') expect(page).not_to have_content('Last updated')
page.within '.line-resolve-all-container' do page.within '.line-resolve-all-container' do
expect(page).to have_content('0/1 discussion resolved') expect(page).to have_content('0/1 discussion resolved')
......
...@@ -73,7 +73,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do ...@@ -73,7 +73,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
expect(page).to have_button "Merge When Build Succeeds" expect(page).to have_button "Merge When Build Succeeds"
visit_merge_request(merge_request) # refresh the page visit_merge_request(merge_request) # refresh the page
expect(page).to have_content "Canceled the automatic merge" expect(page).to have_content "canceled the automatic merge"
end end
it "allows the user to remove the source branch" do it "allows the user to remove the source branch" do
...@@ -101,7 +101,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do ...@@ -101,7 +101,7 @@ feature 'Merge When Build Succeeds', feature: true, js: true do
expect(page).not_to have_link "Merge When Build Succeeds" expect(page).not_to have_link "Merge When Build Succeeds"
end end
end end
def visit_merge_request(merge_request) def visit_merge_request(merge_request)
visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request) visit namespace_project_merge_request_path(merge_request.project.namespace, merge_request.project, merge_request)
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