From e93f6b030a1a4c71aaea9a64672369723dc845a0 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Tue, 24 Jun 2014 22:19:35 +0300 Subject: [PATCH] Improve discussions * check for outdated discussions by comparing diff * improve discussion UI Signed-off-by: Dmitriy Zaporozhets --- app/assets/stylesheets/sections/notes.scss | 46 +++++++------------ app/helpers/notes_helper.rb | 6 --- app/models/note.rb | 22 +++++++-- .../notes/_commit_discussion.html.haml | 0 .../notes/_diff_notes_with_reply.html.haml | 5 +- .../_diff_notes_with_reply_parallel.html.haml | 2 +- .../projects/notes/_discussion.html.haml | 21 +++++---- .../notes/_outdated_discussion.html.haml | 0 .../notes/discussions/_active.html.haml | 21 +++++++++ .../notes/discussions/_commit.html.haml | 28 +++++++++++ .../notes/discussions/_outdated.html.haml | 20 ++++++++ 11 files changed, 119 insertions(+), 52 deletions(-) create mode 100644 app/views/projects/notes/_commit_discussion.html.haml create mode 100644 app/views/projects/notes/_outdated_discussion.html.haml create mode 100644 app/views/projects/notes/discussions/_active.html.haml create mode 100644 app/views/projects/notes/discussions/_commit.html.haml create mode 100644 app/views/projects/notes/discussions/_outdated.html.haml diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss index e8d6ec3e29a..18db7abc64e 100644 --- a/app/assets/stylesheets/sections/notes.scss +++ b/app/assets/stylesheets/sections/notes.scss @@ -43,38 +43,14 @@ ul.notes { } .discussion { - padding: 8px 0; + padding: 10px 0; overflow: hidden; display: block; position:relative; + border-bottom: 1px solid #EEE; .discussion-body { margin-left: 50px; - - .diff-file, - .discussion-hidden, - .notes { - background-color: #F9F9F9; - } - .diff-file .notes { - /* reset */ - background: inherit; - border: none; - @include box-shadow(none); - - } - .discussion-hidden .note { - @extend .cgray; - padding: 8px; - text-align: center; - } - .notes .note { - border-color: #ddd; - padding: 8px; - } - .reply-btn { - margin-top: 8px; - } } } @@ -137,10 +113,6 @@ ul.notes { vertical-align: top; } } - - .reply-btn { - margin: 5px; - } } /** @@ -376,3 +348,17 @@ ul.notes { margin-top: 5px; margin-bottom: 5px; } + +.discussion-body, +.diff-file { + .notes .note { + border-color: #ddd; + padding: 10px 15px; + } + + .discussion-reply-holder { + background: #f9f9f9; + padding: 10px 15px; + border-top: 1px solid #DDD; + } +} diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index da1969b344d..53ac5febd61 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -15,12 +15,6 @@ module NotesHelper end end - def link_to_merge_request_diff_line_note(note) - if note.for_merge_request_diff_line? and note.diff - link_to "#{note.diff_file_name}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable, anchor: note.line_code) - end - end - def note_timestamp(note) # Shows the created at time and the updated at time if different ts = "#{time_ago_with_tooltip(note.created_at, 'bottom', 'note_created_ago')}" diff --git a/app/models/note.rb b/app/models/note.rb index 01026cd3994..590fd338fd9 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -179,10 +179,26 @@ class Note < ActiveRecord::Base @diff ||= Gitlab::Git::Diff.new(st_diff) if st_diff.respond_to?(:map) end + # Check if such line of code exists in merge request diff + # If exists - its active discussion + # If not - its outdated diff def active? - # TODO: determine if discussion is outdated - # according to recent MR diff or not - true + noteable.diffs.each do |mr_diff| + next unless mr_diff.new_path == self.diff.new_path + + Gitlab::DiffParser.new(mr_diff.diff.lines.to_a, mr_diff.new_path). + each do |full_line, type, line_code, line_new, line_old| + if full_line == diff_line + return true + end + end + end + + false + end + + def outdated? + !active? end def diff_file_index diff --git a/app/views/projects/notes/_commit_discussion.html.haml b/app/views/projects/notes/_commit_discussion.html.haml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/app/views/projects/notes/_diff_notes_with_reply.html.haml b/app/views/projects/notes/_diff_notes_with_reply.html.haml index 6ec9169d432..79a66eff129 100644 --- a/app/views/projects/notes/_diff_notes_with_reply.html.haml +++ b/app/views/projects/notes/_diff_notes_with_reply.html.haml @@ -1,7 +1,7 @@ - note = notes.first # example note -# Check if line want not changed since comment was left - if !defined?(line) || line == note.diff_line - %tr.notes_holder.js-toggle-content + %tr.notes_holder %td.notes_line{ colspan: 2 } %span.btn.disabled %i.icon-comment @@ -9,4 +9,5 @@ %td.notes_content %ul.notes{ rel: note.discussion_id } = render notes - = link_to_reply_diff(note) + .discussion-reply-holder + = link_to_reply_diff(note) diff --git a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml b/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml index 02f189f7c5a..279b04e5047 100644 --- a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml +++ b/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml @@ -2,7 +2,7 @@ - note2 = notes2.first # example note -# Check if line want not changed since comment was left /- if !defined?(line) || line == note.diff_line -%tr.notes_holder.js-toggle-content +%tr.notes_holder - if note1 %td.notes_line %span.btn.disabled diff --git a/app/views/projects/notes/_discussion.html.haml b/app/views/projects/notes/_discussion.html.haml index b4bbfdc3f44..73f1fe788ea 100644 --- a/app/views/projects/notes/_discussion.html.haml +++ b/app/views/projects/notes/_discussion.html.haml @@ -1,5 +1,13 @@ - note = discussion_notes.first -.discussion.js-toggle-container{ class: note.discussion_id } +- if note.for_merge_request? + - if note.outdated? + = render "projects/notes/discussions/outdated", discussion_notes: discussion_notes + - else + = render "projects/notes/discussions/active", discussion_notes: discussion_notes +- else + = render "projects/notes/discussions/commit", discussion_notes: discussion_notes + +-#.discussion.js-toggle-container{ class: note.discussion_id } .discussion-header .discussion-actions = link_to "#", class: "js-toggle-button" do @@ -10,7 +18,7 @@ = link_to_member(@project, note.author, avatar: false) - if note.for_merge_request? - if note.diff - started a discussion on this merge request diff + started a discussion on the diff = link_to_merge_request_diff_line_note(note) - else started @@ -32,14 +40,7 @@ #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')} .discussion-body.js-toggle-content - if note.for_diff_line? - - if note.active? - = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note - - else - = link_to 'show outdated discussion', '#', class: 'js-show-outdated-discussion' - %div.hide.outdated-discussion - .notes{ rel: discussion_notes.first.discussion_id } - = render discussion_notes - + = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note - else .notes{ rel: discussion_notes.first.discussion_id } = render discussion_notes diff --git a/app/views/projects/notes/_outdated_discussion.html.haml b/app/views/projects/notes/_outdated_discussion.html.haml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/app/views/projects/notes/discussions/_active.html.haml b/app/views/projects/notes/discussions/_active.html.haml new file mode 100644 index 00000000000..c242e68ee73 --- /dev/null +++ b/app/views/projects/notes/discussions/_active.html.haml @@ -0,0 +1,21 @@ +- note = discussion_notes.first +.discussion.js-toggle-container{ class: note.discussion_id } + .discussion-header + .discussion-actions + = link_to "#", class: "js-toggle-button" do + %i.icon-chevron-up + Show/hide discussion + = image_tag avatar_icon(note.author_email), class: "avatar s32" + %div + = link_to_member(@project, note.author, avatar: false) + started a discussion + = link_to diffs_project_merge_request_path(note.project, note.noteable, anchor: note.line_code) do + %strong on the diff + .last-update.hide.js-toggle-content + - last_note = discussion_notes.last + last updated by + = link_to_member(@project, last_note.author, avatar: false) + %span.discussion-last-update + #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')} + .discussion-body.js-toggle-content + = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note diff --git a/app/views/projects/notes/discussions/_commit.html.haml b/app/views/projects/notes/discussions/_commit.html.haml new file mode 100644 index 00000000000..f8ec4e972e0 --- /dev/null +++ b/app/views/projects/notes/discussions/_commit.html.haml @@ -0,0 +1,28 @@ +- note = discussion_notes.first +.discussion.js-toggle-container{ class: note.discussion_id } + .discussion-header + .discussion-actions + = link_to "#", class: "js-toggle-button" do + %i.icon-chevron-up + Show/hide discussion + = image_tag avatar_icon(note.author_email), class: "avatar s32" + %div + = link_to_member(@project, note.author, avatar: false) + started a discussion on commit + = link_to(note.noteable.short_id, project_commit_path(note.project, note.noteable), class: 'monospace') + .last-update.hide.js-toggle-content + - last_note = discussion_notes.last + last updated by + = link_to_member(@project, last_note.author, avatar: false) + %span.discussion-last-update + #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')} + .discussion-body.js-toggle-content + - if note.for_diff_line? + = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note + - else + .panel.panel-default + .notes{ rel: discussion_notes.first.discussion_id } + = render discussion_notes + .discussion-reply-holder + = link_to_reply_diff(discussion_notes.first) + diff --git a/app/views/projects/notes/discussions/_outdated.html.haml b/app/views/projects/notes/discussions/_outdated.html.haml new file mode 100644 index 00000000000..51ee0cc9d3f --- /dev/null +++ b/app/views/projects/notes/discussions/_outdated.html.haml @@ -0,0 +1,20 @@ +- note = discussion_notes.first +.discussion.js-toggle-container{ class: note.discussion_id } + .discussion-header + .discussion-actions + = link_to "#", class: "js-toggle-button" do + %i.icon-chevron-down + Show/hide discussion + = image_tag avatar_icon(note.author_email), class: "avatar s32" + %div + = link_to_member(@project, note.author, avatar: false) + started a discussion on the + %strong outdated diff + %div + - last_note = discussion_notes.last + last updated by + = link_to_member(@project, last_note.author, avatar: false) + %span.discussion-last-update + #{time_ago_with_tooltip(last_note.updated_at, 'bottom', 'discussion_updated_ago')} + .discussion-body.js-toggle-content.hide + = render "projects/notes/discussion_diff", discussion_notes: discussion_notes, note: note