diff options
| -rw-r--r-- | app/assets/javascripts/diff.js | 5 | ||||
| -rw-r--r-- | app/assets/javascripts/diff_notes/components/diff_note_avatars.js | 4 | ||||
| -rw-r--r-- | app/assets/javascripts/files_comment_button.js | 193 | ||||
| -rw-r--r-- | app/assets/javascripts/notes.js | 9 | ||||
| -rw-r--r-- | app/assets/javascripts/single_file_diff.js | 4 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/diff.scss | 8 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/notes.scss | 10 | ||||
| -rw-r--r-- | app/helpers/notes_helper.rb | 12 | ||||
| -rw-r--r-- | app/views/projects/diffs/_line.html.haml | 3 | ||||
| -rw-r--r-- | app/views/projects/diffs/_parallel_view.html.haml | 7 | ||||
| -rw-r--r-- | features/steps/shared/diff_note.rb | 2 | ||||
| -rw-r--r-- | spec/features/expand_collapse_diffs_spec.rb | 2 |
12 files changed, 105 insertions, 154 deletions
diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 725ec7b9c70..1be9df19c81 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -1,6 +1,7 @@ /* eslint-disable class-methods-use-this */ import './lib/utils/url_utility'; +import FilesCommentButton from './files_comment_button'; const UNFOLD_COUNT = 20; let isBound = false; @@ -8,8 +9,10 @@ let isBound = false; class Diff { constructor() { const $diffFile = $('.files .diff-file'); + $diffFile.singleFileDiff(); - $diffFile.filesCommentButton(); + + FilesCommentButton.init($diffFile); $diffFile.each((index, file) => new gl.ImageFile(file)); diff --git a/app/assets/javascripts/diff_notes/components/diff_note_avatars.js b/app/assets/javascripts/diff_notes/components/diff_note_avatars.js index 517bdb6be09..c37249c060a 100644 --- a/app/assets/javascripts/diff_notes/components/diff_note_avatars.js +++ b/app/assets/javascripts/diff_notes/components/diff_note_avatars.js @@ -139,9 +139,9 @@ const DiffNoteAvatars = Vue.extend({ const notesCount = this.notesCount; $(this.$el).closest('.js-avatar-container') - .toggleClass('js-no-comment-btn', notesCount > 0) + .toggleClass('no-comment-btn', notesCount > 0) .nextUntil('.js-avatar-container') - .toggleClass('js-no-comment-btn', notesCount > 0); + .toggleClass('no-comment-btn', notesCount > 0); }, toggleDiscussionsToggleState() { const $notesHolders = $(this.$el).closest('.code').find('.notes_holder'); diff --git a/app/assets/javascripts/files_comment_button.js b/app/assets/javascripts/files_comment_button.js index 534e651b030..d02e4cd5876 100644 --- a/app/assets/javascripts/files_comment_button.js +++ b/app/assets/javascripts/files_comment_button.js @@ -1,150 +1,73 @@ /* eslint-disable func-names, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, max-len, one-var, one-var-declaration-per-line, quotes, prefer-template, newline-per-chained-call, comma-dangle, new-cap, no-else-return, consistent-return */ -/* global FilesCommentButton */ /* global notes */ -let $commentButtonTemplate; - -window.FilesCommentButton = (function() { - var COMMENT_BUTTON_CLASS, EMPTY_CELL_CLASS, LINE_COLUMN_CLASSES, LINE_CONTENT_CLASS, LINE_HOLDER_CLASS, LINE_NUMBER_CLASS, OLD_LINE_CLASS, TEXT_FILE_SELECTOR, UNFOLDABLE_LINE_CLASS; - - COMMENT_BUTTON_CLASS = '.add-diff-note'; - - LINE_HOLDER_CLASS = '.line_holder'; - - LINE_NUMBER_CLASS = 'diff-line-num'; - - LINE_CONTENT_CLASS = 'line_content'; - - UNFOLDABLE_LINE_CLASS = 'js-unfold'; - - EMPTY_CELL_CLASS = 'empty-cell'; - - OLD_LINE_CLASS = 'old_line'; - - LINE_COLUMN_CLASSES = "." + LINE_NUMBER_CLASS + ", .line_content"; - - TEXT_FILE_SELECTOR = '.text-file'; - - function FilesCommentButton(filesContainerElement) { - this.render = this.render.bind(this); - this.hideButton = this.hideButton.bind(this); - this.isParallelView = notes.isParallelView(); - filesContainerElement.on('mouseover', LINE_COLUMN_CLASSES, this.render) - .on('mouseleave', LINE_COLUMN_CLASSES, this.hideButton); - } - - FilesCommentButton.prototype.render = function(e) { - var $currentTarget, buttonParentElement, lineContentElement, textFileElement, $button; - $currentTarget = $(e.currentTarget); - - if ($currentTarget.hasClass('js-no-comment-btn')) return; - - lineContentElement = this.getLineContent($currentTarget); - buttonParentElement = this.getButtonParent($currentTarget); - - if (!this.validateButtonParent(buttonParentElement) || !this.validateLineContent(lineContentElement)) return; - - $button = $(COMMENT_BUTTON_CLASS, buttonParentElement); - buttonParentElement.addClass('is-over') - .nextUntil(`.${LINE_CONTENT_CLASS}`).addClass('is-over'); - - if ($button.length) { - return; +/* Developer beware! Do not add logic to showButton or hideButton + * that will force a reflow. Doing so will create a signficant performance + * bottleneck for pages with large diffs. For a comprehensive list of what + * causes reflows, visit https://gist.github.com/paulirish/5d52fb081b3570c81e3a + */ + +const LINE_NUMBER_CLASS = 'diff-line-num'; +const UNFOLDABLE_LINE_CLASS = 'js-unfold'; +const NO_COMMENT_CLASS = 'no-comment-btn'; +const EMPTY_CELL_CLASS = 'empty-cell'; +const OLD_LINE_CLASS = 'old_line'; +const LINE_COLUMN_CLASSES = `.${LINE_NUMBER_CLASS}, .line_content`; +const DIFF_CONTAINER_SELECTOR = '.files'; +const DIFF_EXPANDED_CLASS = 'diff-expanded'; + +export default { + init($diffFile) { + /* Caching is used only when the following members are *true*. This is because there are likely to be + * differently configured versions of diffs in the same session. However if these values are true, they + * will be true in all cases */ + + if (!this.userCanCreateNote) { + // data-can-create-note is an empty string when true, otherwise undefined + this.userCanCreateNote = $diffFile.closest(DIFF_CONTAINER_SELECTOR).data('can-create-note') === ''; } - textFileElement = this.getTextFileElement($currentTarget); - buttonParentElement.append(this.buildButton({ - discussionID: lineContentElement.attr('data-discussion-id'), - lineType: lineContentElement.attr('data-line-type'), - - noteableType: textFileElement.attr('data-noteable-type'), - noteableID: textFileElement.attr('data-noteable-id'), - commitID: textFileElement.attr('data-commit-id'), - noteType: lineContentElement.attr('data-note-type'), - - // LegacyDiffNote - lineCode: lineContentElement.attr('data-line-code'), - - // DiffNote - position: lineContentElement.attr('data-position') - })); - }; - - FilesCommentButton.prototype.hideButton = function(e) { - var $currentTarget = $(e.currentTarget); - var buttonParentElement = this.getButtonParent($currentTarget); - - buttonParentElement.removeClass('is-over') - .nextUntil(`.${LINE_CONTENT_CLASS}`).removeClass('is-over'); - }; - - FilesCommentButton.prototype.buildButton = function(buttonAttributes) { - return $commentButtonTemplate.clone().attr({ - 'data-discussion-id': buttonAttributes.discussionID, - 'data-line-type': buttonAttributes.lineType, - - 'data-noteable-type': buttonAttributes.noteableType, - 'data-noteable-id': buttonAttributes.noteableID, - 'data-commit-id': buttonAttributes.commitID, - 'data-note-type': buttonAttributes.noteType, - - // LegacyDiffNote - 'data-line-code': buttonAttributes.lineCode, - - // DiffNote - 'data-position': buttonAttributes.position - }); - }; - - FilesCommentButton.prototype.getTextFileElement = function(hoveredElement) { - return hoveredElement.closest(TEXT_FILE_SELECTOR); - }; - - FilesCommentButton.prototype.getLineContent = function(hoveredElement) { - if (hoveredElement.hasClass(LINE_CONTENT_CLASS)) { - return hoveredElement; - } - if (!this.isParallelView) { - return $(hoveredElement).closest(LINE_HOLDER_CLASS).find("." + LINE_CONTENT_CLASS); - } else { - return $(hoveredElement).next("." + LINE_CONTENT_CLASS); + if (typeof notes !== 'undefined' && !this.isParallelView) { + this.isParallelView = notes.isParallelView && notes.isParallelView(); } - }; - FilesCommentButton.prototype.getButtonParent = function(hoveredElement) { - if (!this.isParallelView) { - if (hoveredElement.hasClass(OLD_LINE_CLASS)) { - return hoveredElement; - } - return hoveredElement.parent().find("." + OLD_LINE_CLASS); - } else { - if (hoveredElement.hasClass(LINE_NUMBER_CLASS)) { - return hoveredElement; - } - return $(hoveredElement).prev("." + LINE_NUMBER_CLASS); + if (this.userCanCreateNote) { + $diffFile.on('mouseover', LINE_COLUMN_CLASSES, e => this.showButton(this.isParallelView, e)) + .on('mouseleave', LINE_COLUMN_CLASSES, e => this.hideButton(this.isParallelView, e)); } - }; + }, - FilesCommentButton.prototype.validateButtonParent = function(buttonParentElement) { - return !buttonParentElement.hasClass(EMPTY_CELL_CLASS) && !buttonParentElement.hasClass(UNFOLDABLE_LINE_CLASS); - }; + showButton(isParallelView, e) { + const buttonParentElement = this.getButtonParent(e.currentTarget, isParallelView); - FilesCommentButton.prototype.validateLineContent = function(lineContentElement) { - return lineContentElement.attr('data-note-type') && lineContentElement.attr('data-note-type') !== ''; - }; + if (!this.validateButtonParent(buttonParentElement)) return; - return FilesCommentButton; -})(); + buttonParentElement.classList.add('is-over'); + buttonParentElement.nextElementSibling.classList.add('is-over'); + }, -$.fn.filesCommentButton = function() { - $commentButtonTemplate = $('<button name="button" type="submit" class="add-diff-note js-add-diff-note-button" title="Add a comment to this line"><i class="fa fa-comment-o"></i></button>'); + hideButton(isParallelView, e) { + const buttonParentElement = this.getButtonParent(e.currentTarget, isParallelView); - if (!(this && (this.parent().data('can-create-note') != null))) { - return; - } - return this.each(function() { - if (!$.data(this, 'filesCommentButton')) { - return $.data(this, 'filesCommentButton', new FilesCommentButton($(this))); + buttonParentElement.classList.remove('is-over'); + buttonParentElement.nextElementSibling.classList.remove('is-over'); + }, + + getButtonParent(hoveredElement, isParallelView) { + if (isParallelView) { + if (!hoveredElement.classList.contains(LINE_NUMBER_CLASS)) { + return hoveredElement.previousElementSibling; + } + } else if (!hoveredElement.classList.contains(OLD_LINE_CLASS)) { + return hoveredElement.parentNode.querySelector(`.${OLD_LINE_CLASS}`); } - }); + return hoveredElement; + }, + + validateButtonParent(buttonParentElement) { + return !buttonParentElement.classList.contains(EMPTY_CELL_CLASS) && + !buttonParentElement.classList.contains(UNFOLDABLE_LINE_CLASS) && + !buttonParentElement.classList.contains(NO_COMMENT_CLASS) && + !buttonParentElement.parentNode.classList.contains(DIFF_EXPANDED_CLASS); + }, }; diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 34476f3303f..46d77b31ffd 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -829,6 +829,8 @@ export default class Notes { */ setupDiscussionNoteForm(dataHolder, form) { // setup note target + const diffFileData = dataHolder.closest('.text-file'); + var discussionID = dataHolder.data('discussionId'); if (discussionID) { @@ -839,9 +841,10 @@ export default class Notes { form.attr('data-line-code', dataHolder.data('lineCode')); form.find('#line_type').val(dataHolder.data('lineType')); - form.find('#note_noteable_type').val(dataHolder.data('noteableType')); - form.find('#note_noteable_id').val(dataHolder.data('noteableId')); - form.find('#note_commit_id').val(dataHolder.data('commitId')); + form.find('#note_noteable_type').val(diffFileData.data('noteableType')); + form.find('#note_noteable_id').val(diffFileData.data('noteableId')); + form.find('#note_commit_id').val(diffFileData.data('commitId')); + form.find('#note_type').val(dataHolder.data('noteType')); // LegacyDiffNote diff --git a/app/assets/javascripts/single_file_diff.js b/app/assets/javascripts/single_file_diff.js index c44892dae3d..9316a2af0b7 100644 --- a/app/assets/javascripts/single_file_diff.js +++ b/app/assets/javascripts/single_file_diff.js @@ -1,5 +1,7 @@ /* eslint-disable func-names, prefer-arrow-callback, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, one-var, one-var-declaration-per-line, consistent-return, no-param-reassign, max-len */ +import FilesCommentButton from './files_comment_button'; + (function() { window.SingleFileDiff = (function() { var COLLAPSED_HTML, ERROR_HTML, LOADING_HTML, WRAPPER; @@ -78,6 +80,8 @@ gl.diffNotesCompileComponents(); } + FilesCommentButton.init($(_this.file)); + if (cb) cb(); }; })(this)); diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index b58922626fa..631649b363f 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -476,6 +476,7 @@ height: 19px; width: 19px; margin-left: -15px; + z-index: 100; &:hover { .diff-comment-avatar, @@ -491,7 +492,7 @@ transform: translateX((($i * $x-pos) - $x-pos)); &:hover { - transform: translateX((($i * $x-pos) - $x-pos)) scale(1.2); + transform: translateX((($i * $x-pos) - $x-pos)); } } } @@ -542,6 +543,7 @@ height: 19px; padding: 0; transition: transform .1s ease-out; + z-index: 100; svg { position: absolute; @@ -555,10 +557,6 @@ fill: $white-light; } - &:hover { - transform: scale(1.2); - } - &:focus { outline: 0; } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 53d5cf2f7bc..303425041df 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -628,8 +628,14 @@ ul.notes { * Line note button on the side of diffs */ +.line_holder .is-over:not(.no-comment-btn) { + .add-diff-note { + opacity: 1; + } +} + .add-diff-note { - display: none; + opacity: 0; margin-top: -2px; border-radius: 50%; background: $white-light; @@ -642,13 +648,11 @@ ul.notes { width: 23px; height: 23px; border: 1px solid $blue-500; - transition: transform .1s ease-in-out; &:hover { background: $blue-500; border-color: $blue-600; color: $white-light; - transform: scale(1.15); } &:active { diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb index 64ad7b280cb..ecc6cd6c6c5 100644 --- a/app/helpers/notes_helper.rb +++ b/app/helpers/notes_helper.rb @@ -47,6 +47,18 @@ module NotesHelper data end + def add_diff_note_button(line_code, position, line_type) + return if @diff_notes_disabled + + button_tag '', + class: 'add-diff-note js-add-diff-note-button', + type: 'submit', name: 'button', + data: diff_view_line_data(line_code, position, line_type), + title: 'Add a comment to this line' do + icon('comment-o') + end + end + def link_to_reply_discussion(discussion, line_type = nil) return unless current_user diff --git a/app/views/projects/diffs/_line.html.haml b/app/views/projects/diffs/_line.html.haml index 43708d22a0c..cd0fb21f8a7 100644 --- a/app/views/projects/diffs/_line.html.haml +++ b/app/views/projects/diffs/_line.html.haml @@ -19,6 +19,7 @@ - if plain = link_text - else + = add_diff_note_button(line_code, diff_file.position(line), type) %a{ href: "##{line_code}", data: { linenumber: link_text } } - discussion = line_discussions.try(:first) - if discussion && discussion.resolvable? && !plain @@ -29,7 +30,7 @@ = link_text - else %a{ href: "##{line_code}", data: { linenumber: link_text } } - %td.line_content.noteable_line{ class: type, data: (diff_view_line_data(line_code, diff_file.position(line), type) unless plain) }< + %td.line_content.noteable_line{ class: type }< - if email %pre= line.text - else diff --git a/app/views/projects/diffs/_parallel_view.html.haml b/app/views/projects/diffs/_parallel_view.html.haml index 8e5f4d2573d..56d63250714 100644 --- a/app/views/projects/diffs/_parallel_view.html.haml +++ b/app/views/projects/diffs/_parallel_view.html.haml @@ -1,4 +1,5 @@ / Side-by-side diff view + .text-file.diff-wrap-lines.code.js-syntax-highlight{ data: diff_view_data } %table - diff_file.parallel_diff_lines.each do |line| @@ -18,11 +19,12 @@ - left_line_code = diff_file.line_code(left) - left_position = diff_file.position(left) %td.old_line.diff-line-num.js-avatar-container{ id: left_line_code, class: left.type, data: { linenumber: left.old_pos } } + = add_diff_note_button(left_line_code, left_position, 'old') %a{ href: "##{left_line_code}", data: { linenumber: left.old_pos } } - discussion_left = discussions_left.try(:first) - if discussion_left && discussion_left.resolvable? %diff-note-avatars{ "discussion-id" => discussion_left.id } - %td.line_content.parallel.noteable_line{ class: left.type, data: diff_view_line_data(left_line_code, left_position, 'old') }= diff_line_content(left.text) + %td.line_content.parallel.noteable_line{ class: left.type }= diff_line_content(left.text) - else %td.old_line.diff-line-num.empty-cell %td.line_content.parallel @@ -38,11 +40,12 @@ - right_line_code = diff_file.line_code(right) - right_position = diff_file.position(right) %td.new_line.diff-line-num.js-avatar-container{ id: right_line_code, class: right.type, data: { linenumber: right.new_pos } } + = add_diff_note_button(right_line_code, right_position, 'new') %a{ href: "##{right_line_code}", data: { linenumber: right.new_pos } } - discussion_right = discussions_right.try(:first) - if discussion_right && discussion_right.resolvable? %diff-note-avatars{ "discussion-id" => discussion_right.id } - %td.line_content.parallel.noteable_line{ class: right.type, data: diff_view_line_data(right_line_code, right_position, 'new') }= diff_line_content(right.text) + %td.line_content.parallel.noteable_line{ class: right.type }= diff_line_content(right.text) - else %td.old_line.diff-line-num.empty-cell %td.line_content.parallel diff --git a/features/steps/shared/diff_note.rb b/features/steps/shared/diff_note.rb index 36fc315599e..2c59ec5bb06 100644 --- a/features/steps/shared/diff_note.rb +++ b/features/steps/shared/diff_note.rb @@ -232,7 +232,7 @@ module SharedDiffNote end def click_parallel_diff_line(code, line_type) - find(".line_content.parallel.#{line_type}[data-line-code='#{code}']").trigger 'mouseover' + find(".line_holder.parallel .diff-line-num[id='#{code}']").trigger 'mouseover' find(".line_holder.parallel button[data-line-code='#{code}']").trigger 'click' end end diff --git a/spec/features/expand_collapse_diffs_spec.rb b/spec/features/expand_collapse_diffs_spec.rb index ea749528c11..d492a15ea17 100644 --- a/spec/features/expand_collapse_diffs_spec.rb +++ b/spec/features/expand_collapse_diffs_spec.rb @@ -129,7 +129,7 @@ feature 'Expand and collapse diffs', js: true, feature: true do before do large_diff.find('.diff-line-num', match: :prefer_exact).hover - large_diff.find('.add-diff-note').click + large_diff.find('.add-diff-note', match: :prefer_exact).click large_diff.find('.note-textarea').send_keys comment_text large_diff.find_button('Comment').click wait_for_requests |