Commit 5c2f6d7f050222d2601218a0bec1dadcee5fcfa0
1 parent
0b3df2f1
Exists in
master
and in
4 other branches
Update notes views to support discussions
Showing
14 changed files
with
270 additions
and
60 deletions
Show diff stats
app/assets/javascripts/notes.js
... | ... | @@ -20,7 +20,7 @@ var NoteList = { |
20 | 20 | // get initial set of notes |
21 | 21 | this.getContent(); |
22 | 22 | |
23 | - $("#notes-list, #new-notes-list").on("ajax:success", ".delete-note", function() { | |
23 | + $("#notes-list, #new-notes-list").on("ajax:success", ".js-note-delete", function() { | |
24 | 24 | $(this).closest('li').fadeOut(function() { |
25 | 25 | $(this).remove(); |
26 | 26 | NoteList.updateVotes(); |
... | ... | @@ -275,16 +275,23 @@ var NoteList = { |
275 | 275 | var PerLineNotes = { |
276 | 276 | init: |
277 | 277 | function() { |
278 | + $(".per_line_form .hide-button").on("click", function(){ | |
279 | + $(this).closest(".per_line_form").hide(); | |
280 | + return false; | |
281 | + }); | |
282 | + | |
278 | 283 | /** |
279 | 284 | * Called when clicking on the "add note" or "reply" button for a diff line. |
280 | 285 | * |
281 | 286 | * Shows the note form below the line. |
282 | 287 | * Sets some hidden fields in the form. |
283 | 288 | */ |
284 | - $(".diff_file_content").on("click", ".line_note_link, .line_note_reply_link", function(e) { | |
289 | + $(".diff_file_content").on("click", ".js-note-add-to-diff-line", function(e) { | |
285 | 290 | var form = $(".per_line_form"); |
286 | 291 | $(this).closest("tr").after(form); |
287 | 292 | form.find("#note_line_code").val($(this).data("lineCode")); |
293 | + form.find("#note_noteable_type").val($(this).data("noteableType")); | |
294 | + form.find("#note_noteable_id").val($(this).data("noteableId")); | |
288 | 295 | form.show(); |
289 | 296 | e.preventDefault(); |
290 | 297 | }); |
... | ... | @@ -297,7 +304,7 @@ var PerLineNotes = { |
297 | 304 | * Removes the actual note from view. |
298 | 305 | * Removes the reply button if the last note for that line has been removed. |
299 | 306 | */ |
300 | - $(".diff_file_content").on("ajax:success", ".delete-note", function() { | |
307 | + $(".diff_file_content").on("ajax:success", ".js-note-delete", function() { | |
301 | 308 | var trNote = $(this).closest("tr"); |
302 | 309 | trNote.fadeOut(function() { |
303 | 310 | $(this).remove(); | ... | ... |
app/assets/stylesheets/sections/notes.scss
1 | 1 | /** |
2 | 2 | * Notes |
3 | - * | |
4 | 3 | */ |
5 | 4 | #notes-list, |
6 | 5 | #new-notes-list { |
... | ... | @@ -8,6 +7,133 @@ |
8 | 7 | list-style: none; |
9 | 8 | margin: 0px; |
10 | 9 | padding: 0px; |
10 | + | |
11 | + .discussion-header, | |
12 | + .note-header { | |
13 | + @extend .cgray; | |
14 | + padding-top: 5px; | |
15 | + padding-bottom: 15px; | |
16 | + | |
17 | + .avatar { | |
18 | + float: left; | |
19 | + margin-right: 10px; | |
20 | + } | |
21 | + | |
22 | + .discussion-last-update, | |
23 | + .note-last-update { | |
24 | + font-style: italic; | |
25 | + } | |
26 | + .note-author { | |
27 | + color: $style_color; | |
28 | + font-weight: bold; | |
29 | + &:hover { | |
30 | + color: $primary_color; | |
31 | + } | |
32 | + } | |
33 | + } | |
34 | + | |
35 | + .discussion { | |
36 | + padding: 8px 0; | |
37 | + overflow: hidden; | |
38 | + display: block; | |
39 | + position:relative; | |
40 | + | |
41 | + .discussion-body { | |
42 | + margin-left: 50px; | |
43 | + | |
44 | + .diff_file, | |
45 | + .discussion-hidden, | |
46 | + .notes { | |
47 | + @extend .borders; | |
48 | + background-color: #F9F9F9; | |
49 | + } | |
50 | + .diff_file .note { | |
51 | + border-bottom: 0px; | |
52 | + padding: 0px; | |
53 | + } | |
54 | + .discussion-hidden .note { | |
55 | + @extend .cgray; | |
56 | + padding: 8px; | |
57 | + text-align: center; | |
58 | + } | |
59 | + .notes .note { | |
60 | + border-color: #ddd; | |
61 | + padding: 8px; | |
62 | + } | |
63 | + } | |
64 | + } | |
65 | + | |
66 | + .note { | |
67 | + padding: 8px 0; | |
68 | + overflow: hidden; | |
69 | + display: block; | |
70 | + position:relative; | |
71 | + p { color: $style_color; } | |
72 | + | |
73 | + .avatar { | |
74 | + margin-top:3px; | |
75 | + } | |
76 | + .note-body { | |
77 | + margin-left:45px; | |
78 | + padding-top: 5px; | |
79 | + } | |
80 | + .note-header { | |
81 | + padding-bottom: 5px; | |
82 | + } | |
83 | + } | |
84 | +} | |
85 | + | |
86 | +#notes-list:not(.reversed) .note, | |
87 | +#notes-list:not(.reversed) .discussion, | |
88 | +#new-notes-list:not(.reversed) .note, | |
89 | +#new-notes-list:not(.reversed) .discussion { | |
90 | + border-bottom: 1px solid #eee; | |
91 | +} | |
92 | +#notes-list.reversed .note, | |
93 | +#notes-list.reversed .discussion, | |
94 | +#new-notes-list.reversed .note, | |
95 | +#new-notes-list.reversed .discussion { | |
96 | + border-top: 1px solid #eee; | |
97 | +} | |
98 | + | |
99 | + | |
100 | +/** | |
101 | + * Discussion/Note Actions | |
102 | + */ | |
103 | +.discussion, | |
104 | +.note { | |
105 | + &.note:hover { | |
106 | + .note-actions { display: block; } | |
107 | + } | |
108 | + .discussion-header:hover { | |
109 | + .discussion-actions { display: block; } | |
110 | + } | |
111 | + | |
112 | + .discussion-actions, | |
113 | + .note-actions { | |
114 | + display: none; | |
115 | + float: right; | |
116 | + | |
117 | + [class^="icon-"], | |
118 | + [class*="icon-"] { | |
119 | + font-size: 16px; | |
120 | + line-height: 16px; | |
121 | + vertical-align: middle; | |
122 | + } | |
123 | + | |
124 | + a { | |
125 | + @extend .cgray; | |
126 | + | |
127 | + &:hover { | |
128 | + color: $primary_color; | |
129 | + &.danger { @extend .cred; } | |
130 | + } | |
131 | + } | |
132 | + } | |
133 | +} | |
134 | +.diff_file .note .note-actions { | |
135 | + right: 0; | |
136 | + top: 0; | |
11 | 137 | } |
12 | 138 | |
13 | 139 | .issue_notes, |
... | ... | @@ -18,13 +144,19 @@ |
18 | 144 | } |
19 | 145 | } |
20 | 146 | |
21 | -/* Note textare */ | |
22 | -#note_note { | |
23 | - height: 80px; | |
24 | - width: 99%; | |
25 | - font-size: 14px; | |
147 | +/* | |
148 | + * New Note Form | |
149 | + */ | |
150 | +.new_note { | |
151 | + /* Note textare */ | |
152 | + #note_note { | |
153 | + height:80px; | |
154 | + width:99%; | |
155 | + font-size:14px; | |
156 | + } | |
26 | 157 | } |
27 | 158 | |
159 | + | |
28 | 160 | #new_note { |
29 | 161 | .attach_holder { |
30 | 162 | display: none; | ... | ... |
app/controllers/notes_controller.rb
... | ... | @@ -6,13 +6,15 @@ class NotesController < ProjectResourceController |
6 | 6 | respond_to :js |
7 | 7 | |
8 | 8 | def index |
9 | + @target_note = Note.new(noteable_type: params[:target_type].camelize, | |
10 | + noteable_id: params[:target_id]) | |
11 | + @target = @target_note.noteable | |
9 | 12 | @notes = Notes::LoadContext.new(project, current_user, params).execute |
10 | 13 | |
11 | 14 | if params[:target_type] == "merge_request" |
12 | - @mixed_targets = true | |
13 | - @main_target_type = params[:target_type].camelize | |
14 | - @discussions = discussions_from_notes | |
15 | - @has_diff = true | |
15 | + @has_diff = true | |
16 | + @mixed_targets = true | |
17 | + @discussions = discussions_from_notes | |
16 | 18 | elsif params[:target_type] == "commit" |
17 | 19 | @has_diff = true |
18 | 20 | end |
... | ... | @@ -72,6 +74,6 @@ class NotesController < ProjectResourceController |
72 | 74 | |
73 | 75 | # Helps to distinguish e.g. commit notes in mr notes list |
74 | 76 | def for_main_target?(note) |
75 | - !@mixed_targets || (@main_target_type == note.noteable_type && !note.for_diff_line?) | |
77 | + !@mixed_targets || (@target.class.name == note.noteable_type && !note.for_diff_line?) | |
76 | 78 | end |
77 | 79 | end | ... | ... |
app/helpers/notes_helper.rb
... | ... | @@ -9,13 +9,18 @@ module NotesHelper |
9 | 9 | |
10 | 10 | # Helps to distinguish e.g. commit notes in mr notes list |
11 | 11 | def note_for_main_target?(note) |
12 | - !@mixed_targets || (@main_target_type == note.noteable_type && !note.for_diff_line?) | |
12 | + !@mixed_targets || (@target.class.name == note.noteable_type && !note.for_diff_line?) | |
13 | 13 | end |
14 | 14 | |
15 | 15 | def link_to_commit_diff_line_note(note) |
16 | 16 | if note.for_commit_diff_line? |
17 | 17 | link_to "#{note.diff_file_name}:L#{note.diff_new_line}", project_commit_path(@project, note.noteable, anchor: note.line_code) |
18 | 18 | end |
19 | + end | |
19 | 20 | |
21 | + def link_to_merge_request_diff_line_note(note) | |
22 | + if note.for_merge_request_diff_line? | |
23 | + link_to "#{note.diff_file_name}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code) | |
24 | + end | |
20 | 25 | end |
21 | 26 | end | ... | ... |
app/models/note.rb
... | ... | @@ -79,7 +79,7 @@ class Note < ActiveRecord::Base |
79 | 79 | end |
80 | 80 | |
81 | 81 | def discussion_id |
82 | - @discussion_id ||= [noteable_type, noteable_id, line_code].join.underscore.to_sym | |
82 | + @discussion_id ||= [:discussion, noteable_type.underscore, noteable_id, line_code].join("-").to_sym | |
83 | 83 | end |
84 | 84 | |
85 | 85 | # Returns true if this is a downvote note, | ... | ... |
app/views/merge_requests/diffs.html.haml
... | ... | @@ -0,0 +1,46 @@ |
1 | +- note = discussion_notes.first | |
2 | +.discussion.js-details-container.js-toggler-container.open{ class: note.discussion_id } | |
3 | + .discussion-header | |
4 | + .discussion-actions | |
5 | + = link_to "javascript:;", class: "js-details-target turn-on js-toggler-target" do | |
6 | + %i.icon-eye-close | |
7 | + Hide discussion | |
8 | + = link_to "javascript:;", class: "js-details-target turn-off js-toggler-target" do | |
9 | + %i.icon-eye-open | |
10 | + Show discussion | |
11 | + = image_tag gravatar_icon(note.author.email), class: "avatar s32" | |
12 | + %div | |
13 | + = link_to note.author_name, project_team_member_path(@project, @project.team_member_by_id(note.author)), class: "note-author" | |
14 | + - if note.for_merge_request? | |
15 | + started a discussion on this merge request diff | |
16 | + = link_to_merge_request_diff_line_note(note) | |
17 | + - elsif note.for_commit? | |
18 | + started a discussion on commit | |
19 | + #{link_to note.noteable.short_id, project_commit_path(@project, note.noteable)} | |
20 | + = link_to_commit_diff_line_note(note) if note.for_diff_line? | |
21 | + - else | |
22 | + %cite.cgray started a discussion | |
23 | + %div | |
24 | + - if discussion_notes.size > 1 | |
25 | + - last_note = discussion_notes.last | |
26 | + last updated by | |
27 | + = link_to last_note.author_name, project_team_member_path(@project, @project.team_member_by_id(last_note.author)), class: "note-author" | |
28 | + %span.discussion-last-update | |
29 | + = time_ago_in_words(last_note.updated_at) | |
30 | + ago | |
31 | + .discussion-body | |
32 | + - if note.for_diff_line? | |
33 | + .diff_file.content | |
34 | + = render "notes/discussion_diff", discussion_notes: discussion_notes, note: note | |
35 | + - else | |
36 | + .notes.content | |
37 | + = render discussion_notes | |
38 | + | |
39 | + -# will be shown when the other one is hidden | |
40 | + .discussion-hidden.content.hide | |
41 | + .note | |
42 | + %em Hidden discussion. | |
43 | + = link_to "javascript:;", class: "js-details-target js-toggler-target" do | |
44 | + %i.icon-eye-open | |
45 | + Show | |
46 | + | ... | ... |
... | ... | @@ -0,0 +1,24 @@ |
1 | +- diff = note.diff | |
2 | +.diff_file_header | |
3 | + %i.icon-file | |
4 | + - if diff.deleted_file | |
5 | + %span{id: "#{diff.a_path}"}= diff.a_path | |
6 | + - else | |
7 | + %span{id: "#{diff.b_path}"}= diff.b_path | |
8 | + %br/ | |
9 | +.diff_file_content | |
10 | + %table | |
11 | + - each_diff_line(diff.diff.lines.to_a, note.diff_file_index) do |line, type, line_code, line_new, line_old| | |
12 | + %tr.line_holder{ id: line_code } | |
13 | + - if type == "match" | |
14 | + %td.old_line= "..." | |
15 | + %td.new_line= "..." | |
16 | + %td.line_content.matched= line | |
17 | + - else | |
18 | + %td.old_line= raw(type == "new" ? " " : line_old) | |
19 | + %td.new_line= raw(type == "old" ? " " : line_new) | |
20 | + %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw "#{line} " | |
21 | + | |
22 | + - if line_code == note.line_code | |
23 | + = render "notes/per_line_notes_with_reply", notes: discussion_notes | |
24 | + - break # cut off diff after notes | ... | ... |
app/views/notes/_note.html.haml
1 | -%li{id: dom_id(note), class: "note"} | |
2 | - = image_tag gravatar_icon(note.author.email), class: "avatar s32" | |
3 | - %div.note-author | |
4 | - %strong= note.author_name | |
5 | - = link_to "##{dom_id(note)}", name: dom_id(note) do | |
6 | - %cite.cgray | |
7 | - = time_ago_in_words(note.updated_at) | |
8 | - ago | |
9 | - | |
10 | - - unless note_for_main_target?(note) | |
11 | - - if note.for_commit? | |
12 | - %span.cgray | |
13 | - on #{link_to note.noteable.short_id, project_commit_path(@project, note.noteable)} | |
14 | - = link_to_commit_diff_line_note(note) if note.for_diff_line? | |
1 | +%li{ id: dom_id(note), class: dom_class(note), data: { discussion: note.discussion_id } } | |
2 | + .note-header | |
3 | + .note-actions | |
4 | + = link_to "##{dom_id(note)}", name: dom_id(note) do | |
5 | + %i.icon-link | |
6 | + Link here | |
7 | + | |
8 | + - if(note.author_id == current_user.id) || can?(current_user, :admin_note, @project) | |
9 | + = link_to project_note_path(@project, note), method: :delete, confirm: 'Are you sure?', remote: true, class: "danger js-note-delete" do | |
10 | + %i.icon-remove-circle | |
11 | + = image_tag gravatar_icon(note.author.email), class: "avatar s32" | |
12 | + = link_to note.author_name, project_team_member_path(@project, @project.team_member_by_id(note.author)), class: "note-author" | |
13 | + %span.note-last-update | |
14 | + = time_ago_in_words(note.updated_at) | |
15 | + ago | |
15 | 16 | |
16 | 17 | -# only show vote if it's a note for the main target |
17 | 18 | - if note_for_main_target?(note) |
... | ... | @@ -24,13 +25,8 @@ |
24 | 25 | %i.icon-thumbs-down |
25 | 26 | \-1 |
26 | 27 | |
27 | - -# remove button | |
28 | - - if(note.author_id == current_user.id) || can?(current_user, :admin_note, @project) | |
29 | - = link_to [@project, note], confirm: 'Are you sure?', method: :delete, remote: true, class: "cred delete-note btn very_small" do | |
30 | - %i.icon-trash | |
31 | - Remove | |
32 | 28 | |
33 | - %div.note-title | |
29 | + .note-body | |
34 | 30 | = preserve do |
35 | 31 | = markdown(note.note) |
36 | 32 | - if note.attachment.url | ... | ... |
app/views/notes/_notes.html.haml
1 | -- @notes.each do |note| | |
2 | - - next unless note.author | |
3 | - = render "note", note: note | |
4 | - | |
1 | +- if @discussions.present? | |
2 | + - @discussions.each do |discussion_notes| | |
3 | + - note = discussion_notes.first | |
4 | + - if note_for_main_target?(note) | |
5 | + = render discussion_notes | |
6 | + - else | |
7 | + = render 'discussion', discussion_notes: discussion_notes | |
8 | +- else | |
9 | + - @notes.each do |note| | |
10 | + - next unless note.author | |
11 | + = render 'note', note: note | ... | ... |
app/views/notes/_per_line_form.html.haml
... | ... | @@ -17,7 +17,7 @@ |
17 | 17 | .note_actions |
18 | 18 | .buttons |
19 | 19 | = f.submit 'Add Comment', class: "btn save-btn submit_note submit_inline_note", id: "submit_note" |
20 | - = link_to "Cancel", "#", class: "btn hide-button" | |
20 | + = link_to "Cancel", "javascript:;", class: "btn hide-button" | |
21 | 21 | .options |
22 | 22 | %h6.left Notify via email: |
23 | 23 | .labels |
... | ... | @@ -29,11 +29,3 @@ |
29 | 29 | = label_tag :notify_author do |
30 | 30 | = check_box_tag :notify_author, 1 , @note.noteable_type == "Commit" |
31 | 31 | %span Commit author |
32 | - | |
33 | -:javascript | |
34 | - $(function(){ | |
35 | - $(".per_line_form .hide-button").bind("click", function(){ | |
36 | - $('.per_line_form').hide(); | |
37 | - return false; | |
38 | - }); | |
39 | - }); | ... | ... |
app/views/notes/_per_line_note_link.html.haml
1 | 1 | = link_to "", |
2 | 2 | "#", |
3 | - id: "line-note-#{line_code}", | |
4 | - class: "line_note_link", | |
3 | + id: "add-diff-line-note-#{line_code}", | |
4 | + class: "line_note_link js-note-add-to-diff-line", | |
5 | 5 | data: @comments_target.merge({ line_code: line_code }), |
6 | - title: "Add comment on line #{line_code[/[0-9]+$/]}" | |
6 | + title: "Add a comment to this line" | ... | ... |
app/views/notes/_per_line_reply_button.html.haml
1 | 1 | %tr.line_notes_row.reply |
2 | 2 | %td{colspan: 3} |
3 | - = link_to "#", | |
4 | - class: "line_note_reply_link", | |
3 | + = link_to "javascript:;", | |
4 | + class: "line_note_reply_link js-note-add-to-diff-line", | |
5 | 5 | data: { line_code: note.line_code, |
6 | 6 | noteable_type: note.noteable_type, |
7 | 7 | noteable_id: note.noteable_id }, |
8 | - title: "Add note for this line" do | |
8 | + title: "Add a comment to this line" do | |
9 | 9 | %i.icon-comment |
10 | 10 | Reply | ... | ... |