From b54203f0ada8f7ad6d24437b6f5f46ebf43f8695 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Sat, 7 Oct 2017 04:25:17 +0000 Subject: [PATCH] Commenting on image diffs --- app/assets/images/icon_image_comment.svg | 1 + app/assets/images/icon_image_comment@2x.svg | 1 + app/assets/javascripts/commit.js | 12 - app/assets/javascripts/commit/file.js | 14 - app/assets/javascripts/commit/image_file.js | 13 +- app/assets/javascripts/diff.js | 5 +- .../components/jump_to_discussion.js | 9 +- app/assets/javascripts/dispatcher.js | 2 - .../image_diff/helpers/badge_helper.js | 38 ++ .../helpers/comment_indicator_helper.js | 58 +++ .../image_diff/helpers/dom_helper.js | 44 +++ .../javascripts/image_diff/helpers/index.js | 25 ++ .../image_diff/helpers/utils_helper.js | 95 +++++ .../javascripts/image_diff/image_badge.js | 23 ++ .../javascripts/image_diff/image_diff.js | 143 +++++++ .../image_diff/init_discussion_tab.js | 12 + .../image_diff/replaced_image_diff.js | 92 +++++ .../javascripts/image_diff/view_types.js | 9 + .../javascripts/lib/utils/image_utility.js | 5 + app/assets/javascripts/main.js | 3 - app/assets/javascripts/merge_request_tabs.js | 4 + app/assets/javascripts/notes.js | 128 ++++++- app/assets/javascripts/single_file_diff.js | 7 +- app/assets/stylesheets/framework/buttons.scss | 22 ++ .../stylesheets/framework/timeline.scss | 6 +- .../stylesheets/framework/variables.scss | 7 + app/assets/stylesheets/pages/diff.scss | 170 ++++++++- app/assets/stylesheets/pages/note_form.scss | 5 +- app/assets/stylesheets/pages/notes.scss | 19 +- app/controllers/concerns/notes_actions.rb | 11 +- app/models/concerns/discussion_on_diff.rb | 4 + app/models/diff_discussion.rb | 2 + app/models/diff_note.rb | 14 +- app/models/discussion.rb | 4 + app/models/legacy_diff_discussion.rb | 8 + app/models/note.rb | 14 +- .../discussions/_diff_discussion.html.haml | 16 +- .../discussions/_diff_with_notes.html.haml | 31 +- app/views/discussions/_notes.html.haml | 19 +- .../diffs/_image_diff_frame.html.haml | 5 + .../diffs/_replaced_image_diff.html.haml | 61 +++ .../diffs/_single_image_diff.html.haml | 16 + .../projects/diffs/viewers/_image.html.haml | 70 +--- app/views/shared/notes/_form.html.haml | 29 +- app/views/shared/notes/_note.html.haml | 15 +- changelogs/unreleased/issue_35873.yml | 5 + lib/gitlab/diff/file.rb | 29 +- lib/gitlab/diff/formatters/base_formatter.rb | 61 +++ lib/gitlab/diff/formatters/image_formatter.rb | 43 +++ lib/gitlab/diff/formatters/text_formatter.rb | 49 +++ lib/gitlab/diff/image_point.rb | 23 ++ lib/gitlab/diff/position.rb | 88 ++--- spec/factories/merge_requests.rb | 5 + spec/features/merge_requests/diffs_spec.rb | 8 +- .../merge_requests/image_diff_notes.rb | 196 ++++++++++ .../user_posts_diff_notes_spec.rb | 1 + .../image_diff/helpers/badge_helper_spec.js | 132 +++++++ .../helpers/comment_indicator_helper_spec.js | 139 +++++++ .../image_diff/helpers/dom_helper_spec.js | 118 ++++++ .../image_diff/helpers/utils_helper_spec.js | 207 ++++++++++ .../image_diff/image_badge_spec.js | 84 ++++ .../javascripts/image_diff/image_diff_spec.js | 361 ++++++++++++++++++ .../image_diff/init_discussion_tab_spec.js | 37 ++ spec/javascripts/image_diff/mock_data.js | 28 ++ .../image_diff/replaced_image_diff_spec.js | 312 +++++++++++++++ .../javascripts/image_diff/view_types_spec.js | 24 ++ .../lib/utils/image_utility_spec.js | 32 ++ .../diff/formatters/image_formatter_spec.rb | 20 + .../diff/formatters/text_formatter_spec.rb | 42 ++ spec/lib/gitlab/diff/position_spec.rb | 85 ++++- spec/lib/gitlab/diff/position_tracer_spec.rb | 16 +- spec/models/diff_note_spec.rb | 40 +- spec/models/note_spec.rb | 50 +++ .../update_diff_position_service_spec.rb | 6 +- .../discussion_comments_shared_example.rb | 2 +- .../shared_examples/position_formatters.rb | 43 +++ spec/support/test_env.rb | 3 +- 77 files changed, 3314 insertions(+), 266 deletions(-) create mode 100644 app/assets/images/icon_image_comment.svg create mode 100644 app/assets/images/icon_image_comment@2x.svg delete mode 100644 app/assets/javascripts/commit.js delete mode 100644 app/assets/javascripts/commit/file.js create mode 100644 app/assets/javascripts/image_diff/helpers/badge_helper.js create mode 100644 app/assets/javascripts/image_diff/helpers/comment_indicator_helper.js create mode 100644 app/assets/javascripts/image_diff/helpers/dom_helper.js create mode 100644 app/assets/javascripts/image_diff/helpers/index.js create mode 100644 app/assets/javascripts/image_diff/helpers/utils_helper.js create mode 100644 app/assets/javascripts/image_diff/image_badge.js create mode 100644 app/assets/javascripts/image_diff/image_diff.js create mode 100644 app/assets/javascripts/image_diff/init_discussion_tab.js create mode 100644 app/assets/javascripts/image_diff/replaced_image_diff.js create mode 100644 app/assets/javascripts/image_diff/view_types.js create mode 100644 app/assets/javascripts/lib/utils/image_utility.js create mode 100644 app/views/projects/diffs/_image_diff_frame.html.haml create mode 100644 app/views/projects/diffs/_replaced_image_diff.html.haml create mode 100644 app/views/projects/diffs/_single_image_diff.html.haml create mode 100644 changelogs/unreleased/issue_35873.yml create mode 100644 lib/gitlab/diff/formatters/base_formatter.rb create mode 100644 lib/gitlab/diff/formatters/image_formatter.rb create mode 100644 lib/gitlab/diff/formatters/text_formatter.rb create mode 100644 lib/gitlab/diff/image_point.rb create mode 100644 spec/features/merge_requests/image_diff_notes.rb create mode 100644 spec/javascripts/image_diff/helpers/badge_helper_spec.js create mode 100644 spec/javascripts/image_diff/helpers/comment_indicator_helper_spec.js create mode 100644 spec/javascripts/image_diff/helpers/dom_helper_spec.js create mode 100644 spec/javascripts/image_diff/helpers/utils_helper_spec.js create mode 100644 spec/javascripts/image_diff/image_badge_spec.js create mode 100644 spec/javascripts/image_diff/image_diff_spec.js create mode 100644 spec/javascripts/image_diff/init_discussion_tab_spec.js create mode 100644 spec/javascripts/image_diff/mock_data.js create mode 100644 spec/javascripts/image_diff/replaced_image_diff_spec.js create mode 100644 spec/javascripts/image_diff/view_types_spec.js create mode 100644 spec/javascripts/lib/utils/image_utility_spec.js create mode 100644 spec/lib/gitlab/diff/formatters/image_formatter_spec.rb create mode 100644 spec/lib/gitlab/diff/formatters/text_formatter_spec.rb create mode 100644 spec/support/shared_examples/position_formatters.rb diff --git a/app/assets/images/icon_image_comment.svg b/app/assets/images/icon_image_comment.svg new file mode 100644 index 00000000000..cf6cb972940 --- /dev/null +++ b/app/assets/images/icon_image_comment.svg @@ -0,0 +1 @@ +cursor diff --git a/app/assets/images/icon_image_comment@2x.svg b/app/assets/images/icon_image_comment@2x.svg new file mode 100644 index 00000000000..83be91d3705 --- /dev/null +++ b/app/assets/images/icon_image_comment@2x.svg @@ -0,0 +1 @@ +cursor_2x diff --git a/app/assets/javascripts/commit.js b/app/assets/javascripts/commit.js deleted file mode 100644 index 5f637524e30..00000000000 --- a/app/assets/javascripts/commit.js +++ /dev/null @@ -1,12 +0,0 @@ -/* eslint-disable func-names, space-before-function-paren, wrap-iife */ -/* global CommitFile */ - -window.Commit = (function() { - function Commit() { - $('.files .diff-file').each(function() { - return new CommitFile(this); - }); - } - - return Commit; -})(); diff --git a/app/assets/javascripts/commit/file.js b/app/assets/javascripts/commit/file.js deleted file mode 100644 index ee087c978dd..00000000000 --- a/app/assets/javascripts/commit/file.js +++ /dev/null @@ -1,14 +0,0 @@ -/* eslint-disable func-names, space-before-function-paren, wrap-iife, no-new */ -/* global ImageFile */ - -(function() { - this.CommitFile = (function() { - function CommitFile(file) { - if ($('.image', file).length) { - new gl.ImageFile(file); - } - } - - return CommitFile; - })(); -}).call(window); diff --git a/app/assets/javascripts/commit/image_file.js b/app/assets/javascripts/commit/image_file.js index 4763985c802..e7adf8814b8 100644 --- a/app/assets/javascripts/commit/image_file.js +++ b/app/assets/javascripts/commit/image_file.js @@ -1,4 +1,6 @@ /* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, no-use-before-define, prefer-arrow-callback, no-else-return, consistent-return, prefer-template, quotes, one-var, one-var-declaration-per-line, no-unused-vars, no-return-assign, comma-dangle, quote-props, no-unused-expressions, no-sequences, object-shorthand, max-len */ +import 'vendor/jquery.waitforimages'; + (function() { gl.ImageFile = (function() { var prepareFrames; @@ -17,15 +19,10 @@ // Load two-up view after images are loaded // so that we can display the correct width and height information - const images = $('.two-up.view img', _this.file); - let loadedCount = 0; + const $images = $('.two-up.view img', _this.file); - images.on('load', () => { - loadedCount += 1; - - if (loadedCount === images.length) { - _this.initView('two-up'); - } + $images.waitForImages(function() { + _this.initView('two-up'); }); }); }; diff --git a/app/assets/javascripts/diff.js b/app/assets/javascripts/diff.js index 9ddfdb6ae21..6c78662baa7 100644 --- a/app/assets/javascripts/diff.js +++ b/app/assets/javascripts/diff.js @@ -3,6 +3,7 @@ import './lib/utils/url_utility'; import FilesCommentButton from './files_comment_button'; import SingleFileDiff from './single_file_diff'; +import imageDiffHelper from './image_diff/helpers/index'; const UNFOLD_COUNT = 20; let isBound = false; @@ -20,7 +21,9 @@ class Diff { const tab = document.getElementById('diffs'); if (!tab || (tab && tab.dataset && tab.dataset.isLocked !== '')) FilesCommentButton.init($diffFile); - $diffFile.each((index, file) => new gl.ImageFile(file)); + const firstFile = $('.files').first().get(0); + const canCreateNote = firstFile && firstFile.hasAttribute('data-can-create-note'); + $diffFile.each((index, file) => imageDiffHelper.initImageDiff(file, canCreateNote)); if (!isBound) { $(document) diff --git a/app/assets/javascripts/diff_notes/components/jump_to_discussion.js b/app/assets/javascripts/diff_notes/components/jump_to_discussion.js index 497c23f014f..e77910a83d4 100644 --- a/app/assets/javascripts/diff_notes/components/jump_to_discussion.js +++ b/app/assets/javascripts/diff_notes/components/jump_to_discussion.js @@ -171,7 +171,14 @@ const JumpToDiscussion = Vue.extend({ // When jumping between unresolved discussions on the diffs tab, we show them. $target.closest(".content").show(); - $target = $target.closest("tr.notes_holder"); + const $notesHolder = $target.closest("tr.notes_holder"); + + // Image diff discussions does not use notes_holder + // so we should keep original $target value in those cases + if ($notesHolder.length > 0) { + $target = $notesHolder; + } + $target.show(); // If we are on the diffs tab, we don't scroll to the discussion itself, but to diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index e4e7cae540e..33271c25146 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -7,7 +7,6 @@ /* global IssuableForm */ /* global LabelsSelect */ /* global MilestoneSelect */ -/* global Commit */ /* global CommitsList */ /* global NewBranchForm */ /* global NotificationsForm */ @@ -316,7 +315,6 @@ import { ajaxGet, convertPermissionToBoolean } from './lib/utils/common_utils'; new gl.Activities(); break; case 'projects:commit:show': - new Commit(); new gl.Diff(); new ZenMode(); shortcut_handler = new ShortcutsNavigation(); diff --git a/app/assets/javascripts/image_diff/helpers/badge_helper.js b/app/assets/javascripts/image_diff/helpers/badge_helper.js new file mode 100644 index 00000000000..6a6a668308d --- /dev/null +++ b/app/assets/javascripts/image_diff/helpers/badge_helper.js @@ -0,0 +1,38 @@ +export function createImageBadge(noteId, { x, y }, classNames = []) { + const buttonEl = document.createElement('button'); + const classList = classNames.concat(['js-image-badge']); + classList.forEach(className => buttonEl.classList.add(className)); + buttonEl.setAttribute('type', 'button'); + buttonEl.setAttribute('disabled', true); + buttonEl.dataset.noteId = noteId; + buttonEl.style.left = `${x}px`; + buttonEl.style.top = `${y}px`; + + return buttonEl; +} + +export function addImageBadge(containerEl, { coordinate, badgeText, noteId }) { + const buttonEl = createImageBadge(noteId, coordinate, ['badge']); + buttonEl.innerText = badgeText; + + containerEl.appendChild(buttonEl); +} + +export function addImageCommentBadge(containerEl, { coordinate, noteId }) { + const buttonEl = createImageBadge(noteId, coordinate, ['image-comment-badge', 'inverted']); + const iconEl = document.createElement('i'); + iconEl.className = 'fa fa-comment-o'; + iconEl.setAttribute('aria-label', 'comment'); + + buttonEl.appendChild(iconEl); + containerEl.appendChild(buttonEl); +} + +export function addAvatarBadge(el, event) { + const { noteId, badgeNumber } = event.detail; + + // Add badge to new comment + const avatarBadgeEl = el.querySelector(`#${noteId} .badge`); + avatarBadgeEl.innerText = badgeNumber; + avatarBadgeEl.classList.remove('hidden'); +} diff --git a/app/assets/javascripts/image_diff/helpers/comment_indicator_helper.js b/app/assets/javascripts/image_diff/helpers/comment_indicator_helper.js new file mode 100644 index 00000000000..05000c73052 --- /dev/null +++ b/app/assets/javascripts/image_diff/helpers/comment_indicator_helper.js @@ -0,0 +1,58 @@ +export function addCommentIndicator(containerEl, { x, y }) { + const buttonEl = document.createElement('button'); + buttonEl.classList.add('btn-transparent'); + buttonEl.classList.add('comment-indicator'); + buttonEl.setAttribute('type', 'button'); + buttonEl.style.left = `${x}px`; + buttonEl.style.top = `${y}px`; + + buttonEl.innerHTML = gl.utils.spriteIcon('image-comment-dark'); + + containerEl.appendChild(buttonEl); +} + +export function removeCommentIndicator(imageFrameEl) { + const commentIndicatorEl = imageFrameEl.querySelector('.comment-indicator'); + const imageEl = imageFrameEl.querySelector('img'); + const willRemove = !!commentIndicatorEl; + let meta = {}; + + if (willRemove) { + meta = { + x: parseInt(commentIndicatorEl.style.left, 10), + y: parseInt(commentIndicatorEl.style.top, 10), + image: { + width: imageEl.width, + height: imageEl.height, + }, + }; + + commentIndicatorEl.remove(); + } + + return Object.assign({}, meta, { + removed: willRemove, + }); +} + +export function showCommentIndicator(imageFrameEl, coordinate) { + const { x, y } = coordinate; + const commentIndicatorEl = imageFrameEl.querySelector('.comment-indicator'); + + if (commentIndicatorEl) { + commentIndicatorEl.style.left = `${x}px`; + commentIndicatorEl.style.top = `${y}px`; + } else { + addCommentIndicator(imageFrameEl, coordinate); + } +} + +export function commentIndicatorOnClick(event) { + // Prevent from triggering onAddImageDiffNote in notes.js + event.stopPropagation(); + + const buttonEl = event.currentTarget; + const diffViewerEl = buttonEl.closest('.diff-viewer'); + const textareaEl = diffViewerEl.querySelector('.note-container .note-textarea'); + textareaEl.focus(); +} diff --git a/app/assets/javascripts/image_diff/helpers/dom_helper.js b/app/assets/javascripts/image_diff/helpers/dom_helper.js new file mode 100644 index 00000000000..12d56714b34 --- /dev/null +++ b/app/assets/javascripts/image_diff/helpers/dom_helper.js @@ -0,0 +1,44 @@ +export function setPositionDataAttribute(el, options) { + // Update position data attribute so that the + // new comment form can use this data for ajax request + const { x, y, width, height } = options; + const position = el.dataset.position; + const positionObject = Object.assign({}, JSON.parse(position), { + x, + y, + width, + height, + }); + + el.setAttribute('data-position', JSON.stringify(positionObject)); +} + +export function updateDiscussionAvatarBadgeNumber(discussionEl, newBadgeNumber) { + const avatarBadgeEl = discussionEl.querySelector('.image-diff-avatar-link .badge'); + avatarBadgeEl.innerText = newBadgeNumber; +} + +export function updateDiscussionBadgeNumber(discussionEl, newBadgeNumber) { + const discussionBadgeEl = discussionEl.querySelector('.badge'); + discussionBadgeEl.innerText = newBadgeNumber; +} + +export function toggleCollapsed(event) { + const toggleButtonEl = event.currentTarget; + const discussionNotesEl = toggleButtonEl.closest('.discussion-notes'); + const formEl = discussionNotesEl.querySelector('.discussion-form'); + const isCollapsed = discussionNotesEl.classList.contains('collapsed'); + + if (isCollapsed) { + discussionNotesEl.classList.remove('collapsed'); + } else { + discussionNotesEl.classList.add('collapsed'); + } + + // Override the inline display style set in notes.js + if (formEl && !isCollapsed) { + formEl.style.display = 'none'; + } else if (formEl && isCollapsed) { + formEl.style.display = 'block'; + } +} diff --git a/app/assets/javascripts/image_diff/helpers/index.js b/app/assets/javascripts/image_diff/helpers/index.js new file mode 100644 index 00000000000..4a100631003 --- /dev/null +++ b/app/assets/javascripts/image_diff/helpers/index.js @@ -0,0 +1,25 @@ +import * as badgeHelper from './badge_helper'; +import * as commentIndicatorHelper from './comment_indicator_helper'; +import * as domHelper from './dom_helper'; +import * as utilsHelper from './utils_helper'; + +export default { + addCommentIndicator: commentIndicatorHelper.addCommentIndicator, + removeCommentIndicator: commentIndicatorHelper.removeCommentIndicator, + showCommentIndicator: commentIndicatorHelper.showCommentIndicator, + commentIndicatorOnClick: commentIndicatorHelper.commentIndicatorOnClick, + + addImageBadge: badgeHelper.addImageBadge, + addImageCommentBadge: badgeHelper.addImageCommentBadge, + addAvatarBadge: badgeHelper.addAvatarBadge, + + setPositionDataAttribute: domHelper.setPositionDataAttribute, + updateDiscussionAvatarBadgeNumber: domHelper.updateDiscussionAvatarBadgeNumber, + updateDiscussionBadgeNumber: domHelper.updateDiscussionBadgeNumber, + toggleCollapsed: domHelper.toggleCollapsed, + + resizeCoordinatesToImageElement: utilsHelper.resizeCoordinatesToImageElement, + generateBadgeFromDiscussionDOM: utilsHelper.generateBadgeFromDiscussionDOM, + getTargetSelection: utilsHelper.getTargetSelection, + initImageDiff: utilsHelper.initImageDiff, +}; diff --git a/app/assets/javascripts/image_diff/helpers/utils_helper.js b/app/assets/javascripts/image_diff/helpers/utils_helper.js new file mode 100644 index 00000000000..96fc735e629 --- /dev/null +++ b/app/assets/javascripts/image_diff/helpers/utils_helper.js @@ -0,0 +1,95 @@ +import ImageBadge from '../image_badge'; +import ImageDiff from '../image_diff'; +import ReplacedImageDiff from '../replaced_image_diff'; +import '../../commit/image_file'; + +export function resizeCoordinatesToImageElement(imageEl, meta) { + const { x, y, width, height } = meta; + + const imageWidth = imageEl.width; + const imageHeight = imageEl.height; + + const widthRatio = imageWidth / width; + const heightRatio = imageHeight / height; + + return { + x: Math.round(x * widthRatio), + y: Math.round(y * heightRatio), + width: imageWidth, + height: imageHeight, + }; +} + +export function generateBadgeFromDiscussionDOM(imageFrameEl, discussionEl) { + const position = JSON.parse(discussionEl.dataset.position); + const firstNoteEl = discussionEl.querySelector('.note'); + const badge = new ImageBadge({ + actual: position, + imageEl: imageFrameEl.querySelector('img'), + noteId: firstNoteEl.id, + discussionId: discussionEl.dataset.discussionId, + }); + + return badge; +} + +export function getTargetSelection(event) { + const containerEl = event.currentTarget; + const imageEl = containerEl.querySelector('img'); + + const x = event.offsetX; + const y = event.offsetY; + + const width = imageEl.width; + const height = imageEl.height; + + const actualWidth = imageEl.naturalWidth; + const actualHeight = imageEl.naturalHeight; + + const widthRatio = actualWidth / width; + const heightRatio = actualHeight / height; + + // Browser will include the frame as a clickable target, + // which would result in potential 1px out of bounds value + // This bound the coordinates to inside the frame + const normalizedX = Math.max(0, x) && Math.min(x, width); + const normalizedY = Math.max(0, y) && Math.min(y, height); + + return { + browser: { + x: normalizedX, + y: normalizedY, + width, + height, + }, + actual: { + // Round x, y so that we don't need to deal with decimals + x: Math.round(normalizedX * widthRatio), + y: Math.round(normalizedY * heightRatio), + width: actualWidth, + height: actualHeight, + }, + }; +} + +export function initImageDiff(fileEl, canCreateNote, renderCommentBadge) { + const options = { + canCreateNote, + renderCommentBadge, + }; + let diff; + + // ImageFile needs to be invoked before initImageDiff so that badges + // can mount to the correct location + new gl.ImageFile(fileEl); // eslint-disable-line no-new + + if (fileEl.querySelector('.diff-file .js-single-image')) { + diff = new ImageDiff(fileEl, options); + diff.init(); + } else if (fileEl.querySelector('.diff-file .js-replaced-image')) { + diff = new ReplacedImageDiff(fileEl, options); + diff.init(); + } + + return diff; +} diff --git a/app/assets/javascripts/image_diff/image_badge.js b/app/assets/javascripts/image_diff/image_badge.js new file mode 100644 index 00000000000..51a8cda98d7 --- /dev/null +++ b/app/assets/javascripts/image_diff/image_badge.js @@ -0,0 +1,23 @@ +import imageDiffHelper from './helpers/index'; + +const defaultMeta = { + x: 0, + y: 0, + width: 0, + height: 0, +}; + +export default class ImageBadge { + constructor(options) { + const { noteId, discussionId } = options; + + this.actual = options.actual || defaultMeta; + this.browser = options.browser || defaultMeta; + this.noteId = noteId; + this.discussionId = discussionId; + + if (options.imageEl && !options.browser) { + this.browser = imageDiffHelper.resizeCoordinatesToImageElement(options.imageEl, this.actual); + } + } +} diff --git a/app/assets/javascripts/image_diff/image_diff.js b/app/assets/javascripts/image_diff/image_diff.js new file mode 100644 index 00000000000..f3af92cf2b0 --- /dev/null +++ b/app/assets/javascripts/image_diff/image_diff.js @@ -0,0 +1,143 @@ +import imageDiffHelper from './helpers/index'; +import ImageBadge from './image_badge'; +import { isImageLoaded } from '../lib/utils/image_utility'; + +export default class ImageDiff { + constructor(el, options) { + this.el = el; + this.canCreateNote = !!(options && options.canCreateNote); + this.renderCommentBadge = !!(options && options.renderCommentBadge); + this.$noteContainer = $('.note-container', this.el); + this.imageBadges = []; + } + + init() { + this.imageFrameEl = this.el.querySelector('.diff-file .js-image-frame'); + this.imageEl = this.imageFrameEl.querySelector('img'); + + this.bindEvents(); + } + + bindEvents() { + this.imageClickedWrapper = this.imageClicked.bind(this); + this.imageBlurredWrapper = imageDiffHelper.removeCommentIndicator.bind(null, this.imageFrameEl); + this.addBadgeWrapper = this.addBadge.bind(this); + this.removeBadgeWrapper = this.removeBadge.bind(this); + this.renderBadgesWrapper = this.renderBadges.bind(this); + + // Render badges + if (isImageLoaded(this.imageEl)) { + this.renderBadges(); + } else { + this.imageEl.addEventListener('load', this.renderBadgesWrapper); + } + + // jquery makes the event delegation here much simpler + this.$noteContainer.on('click', '.js-diff-notes-toggle', imageDiffHelper.toggleCollapsed); + $(this.el).on('click', '.comment-indicator', imageDiffHelper.commentIndicatorOnClick); + + if (this.canCreateNote) { + this.el.addEventListener('click.imageDiff', this.imageClickedWrapper); + this.el.addEventListener('blur.imageDiff', this.imageBlurredWrapper); + this.el.addEventListener('addBadge.imageDiff', this.addBadgeWrapper); + this.el.addEventListener('removeBadge.imageDiff', this.removeBadgeWrapper); + } + } + + imageClicked(event) { + const customEvent = event.detail; + const selection = imageDiffHelper.getTargetSelection(customEvent); + const el = customEvent.currentTarget; + + imageDiffHelper.setPositionDataAttribute(el, selection.actual); + imageDiffHelper.showCommentIndicator(this.imageFrameEl, selection.browser); + } + + renderBadges() { + const discussionsEls = this.el.querySelectorAll('.note-container .discussion-notes .notes'); + [...discussionsEls].forEach(this.renderBadge.bind(this)); + } + + renderBadge(discussionEl, index) { + const imageBadge = imageDiffHelper + .generateBadgeFromDiscussionDOM(this.imageFrameEl, discussionEl); + + this.imageBadges.push(imageBadge); + + const options = { + coordinate: imageBadge.browser, + noteId: imageBadge.noteId, + }; + + if (this.renderCommentBadge) { + imageDiffHelper.addImageCommentBadge(this.imageFrameEl, options); + } else { + const numberBadgeOptions = Object.assign({}, options, { + badgeText: index + 1, + }); + + imageDiffHelper.addImageBadge(this.imageFrameEl, numberBadgeOptions); + } + } + + addBadge(event) { + const { x, y, width, height, noteId, discussionId } = event.detail; + const badgeText = this.imageBadges.length + 1; + const imageBadge = new ImageBadge({ + actual: { + x, + y, + width, + height, + }, + imageEl: this.imageFrameEl.querySelector('img'), + noteId, + discussionId, + }); + + this.imageBadges.push(imageBadge); + + imageDiffHelper.addImageBadge(this.imageFrameEl, { + coordinate: imageBadge.browser, + badgeText, + noteId, + }); + + imageDiffHelper.addAvatarBadge(this.el, { + detail: { + noteId, + badgeNumber: badgeText, + }, + }); + + const discussionEl = this.el.querySelector(`#discussion_${discussionId}`); + imageDiffHelper.updateDiscussionBadgeNumber(discussionEl, badgeText); + } + + removeBadge(event) { + const { badgeNumber } = event.detail; + const indexToRemove = badgeNumber - 1; + const imageBadgeEls = this.imageFrameEl.querySelectorAll('.badge'); + + if (this.imageBadges.length !== badgeNumber) { + // Cascade badges count numbers for (avatar badges + image badges) + this.imageBadges.forEach((badge, index) => { + if (index > indexToRemove) { + const { discussionId } = badge; + const updatedBadgeNumber = index; + const discussionEl = this.el.querySelector(`#discussion_${discussionId}`); + + imageBadgeEls[index].innerText = updatedBadgeNumber; + + imageDiffHelper.updateDiscussionBadgeNumber(discussionEl, updatedBadgeNumber); + imageDiffHelper.updateDiscussionAvatarBadgeNumber(discussionEl, updatedBadgeNumber); + } + }); + } + + this.imageBadges.splice(indexToRemove, 1); + + const imageBadgeEl = imageBadgeEls[indexToRemove]; + imageBadgeEl.remove(); + } +} diff --git a/app/assets/javascripts/image_diff/init_discussion_tab.js b/app/assets/javascripts/image_diff/init_discussion_tab.js new file mode 100644 index 00000000000..2f16c6ef115 --- /dev/null +++ b/app/assets/javascripts/image_diff/init_discussion_tab.js @@ -0,0 +1,12 @@ +import imageDiffHelper from './helpers/index'; + +export default () => { + // Always pass can-create-note as false because a user + // cannot place new badge markers on discussion tab + const canCreateNote = false; + const renderCommentBadge = true; + + const diffFileEls = document.querySelectorAll('.timeline-content .diff-file.js-image-file'); + [...diffFileEls].forEach(diffFileEl => + imageDiffHelper.initImageDiff(diffFileEl, canCreateNote, renderCommentBadge)); +}; diff --git a/app/assets/javascripts/image_diff/replaced_image_diff.js b/app/assets/javascripts/image_diff/replaced_image_diff.js new file mode 100644 index 00000000000..4abd13fb472 --- /dev/null +++ b/app/assets/javascripts/image_diff/replaced_image_diff.js @@ -0,0 +1,92 @@ +import imageDiffHelper from './helpers/index'; +import { viewTypes, isValidViewType } from './view_types'; +import ImageDiff from './image_diff'; + +export default class ReplacedImageDiff extends ImageDiff { + init(defaultViewType = viewTypes.TWO_UP) { + this.imageFrameEls = { + [viewTypes.TWO_UP]: this.el.querySelector('.two-up .js-image-frame'), + [viewTypes.SWIPE]: this.el.querySelector('.swipe .js-image-frame'), + [viewTypes.ONION_SKIN]: this.el.querySelector('.onion-skin .js-image-frame'), + }; + + const viewModesEl = this.el.querySelector('.view-modes-menu'); + this.viewModesEls = { + [viewTypes.TWO_UP]: viewModesEl.querySelector('.two-up'), + [viewTypes.SWIPE]: viewModesEl.querySelector('.swipe'), + [viewTypes.ONION_SKIN]: viewModesEl.querySelector('.onion-skin'), + }; + + this.currentView = defaultViewType; + this.generateImageEls(); + this.bindEvents(); + } + + generateImageEls() { + this.imageEls = {}; + + const viewTypeNames = Object.getOwnPropertyNames(viewTypes); + viewTypeNames.forEach((viewType) => { + this.imageEls[viewType] = this.imageFrameEls[viewType].querySelector('img'); + }); + } + + bindEvents() { + super.bindEvents(); + + this.changeToViewTwoUp = this.changeView.bind(this, viewTypes.TWO_UP); + this.changeToViewSwipe = this.changeView.bind(this, viewTypes.SWIPE); + this.changeToViewOnionSkin = this.changeView.bind(this, viewTypes.ONION_SKIN); + + this.viewModesEls[viewTypes.TWO_UP].addEventListener('click', this.changeToViewTwoUp); + this.viewModesEls[viewTypes.SWIPE].addEventListener('click', this.changeToViewSwipe); + this.viewModesEls[viewTypes.ONION_SKIN].addEventListener('click', this.changeToViewOnionSkin); + } + + get imageEl() { + return this.imageEls[this.currentView]; + } + + get imageFrameEl() { + return this.imageFrameEls[this.currentView]; + } + + changeView(newView) { + if (!isValidViewType(newView)) { + return; + } + + const indicator = imageDiffHelper.removeCommentIndicator(this.imageFrameEl); + + this.currentView = newView; + + // Clear existing badges on new view + const existingBadges = this.imageFrameEl.querySelectorAll('.badge'); + [...existingBadges].map(badge => badge.remove()); + + // Remove existing references to old view image badges + this.imageBadges = []; + + // Image_file.js has a fade animation of 200ms for loading the view + // Need to wait an additional 250ms for the images to be displayed + // on window in order to re-normalize their dimensions + setTimeout(this.renderNewView.bind(this, indicator), 250); + } + + renderNewView(indicator) { + // Generate badge coordinates on new view + this.renderBadges(); + + // Re-render indicator in new view + if (indicator.removed) { + const normalizedIndicator = imageDiffHelper + .resizeCoordinatesToImageElement(this.imageEl, { + x: indicator.x, + y: indicator.y, + width: indicator.image.width, + height: indicator.image.height, + }); + imageDiffHelper.showCommentIndicator(this.imageFrameEl, normalizedIndicator); + } + } +} diff --git a/app/assets/javascripts/image_diff/view_types.js b/app/assets/javascripts/image_diff/view_types.js new file mode 100644 index 00000000000..ab0a595571f --- /dev/null +++ b/app/assets/javascripts/image_diff/view_types.js @@ -0,0 +1,9 @@ +export const viewTypes = { + TWO_UP: 'TWO_UP', + SWIPE: 'SWIPE', + ONION_SKIN: 'ONION_SKIN', +}; + +export function isValidViewType(validate) { + return !!Object.getOwnPropertyNames(viewTypes).find(viewType => viewType === validate); +} diff --git a/app/assets/javascripts/lib/utils/image_utility.js b/app/assets/javascripts/lib/utils/image_utility.js new file mode 100644 index 00000000000..2977ec821cb --- /dev/null +++ b/app/assets/javascripts/lib/utils/image_utility.js @@ -0,0 +1,5 @@ +/* eslint-disable import/prefer-default-export */ + +export function isImageLoaded(element) { + return element.complete && element.naturalHeight !== 0; +} diff --git a/app/assets/javascripts/main.js b/app/assets/javascripts/main.js index 2c1dcc374ff..5858c2b6fd8 100644 --- a/app/assets/javascripts/main.js +++ b/app/assets/javascripts/main.js @@ -35,8 +35,6 @@ import './shortcuts_network'; import './templates/issuable_template_selector'; import './templates/issuable_template_selectors'; -// commit -import './commit/file'; import './commit/image_file'; // lib/utils @@ -70,7 +68,6 @@ import './build'; import './build_artifacts'; import './build_variables'; import './ci_lint_editor'; -import './commit'; import './commits'; import './compare'; import './compare_autocomplete'; diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index d3299c15720..c042b22d1fd 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -13,6 +13,8 @@ import { isMetaClick, } from './lib/utils/common_utils'; +import initDiscussionTab from './image_diff/init_discussion_tab'; + /* eslint-disable max-len */ // MergeRequestTabs // @@ -154,6 +156,8 @@ import { } this.resetViewContainer(); this.destroyPipelinesView(); + + initDiscussionTab(); } if (this.setUrl) { this.setCurrentAction(action); diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index 93aa29454a0..24de21f2ce2 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -24,6 +24,7 @@ import './autosave'; import './dropzone_input'; import TaskList from './task_list'; import { ajaxPost, isInViewport, getPagePath, scrollToElement, isMetaKey } from './lib/utils/common_utils'; +import imageDiffHelper from './image_diff/helpers/index'; window.autosize = autosize; window.Dropzone = Dropzone; @@ -42,6 +43,7 @@ export default class Notes { this.visibilityChange = this.visibilityChange.bind(this); this.cancelDiscussionForm = this.cancelDiscussionForm.bind(this); this.onAddDiffNote = this.onAddDiffNote.bind(this); + this.onAddImageDiffNote = this.onAddImageDiffNote.bind(this); this.setupDiscussionNoteForm = this.setupDiscussionNoteForm.bind(this); this.onReplyToDiscussionNote = this.onReplyToDiscussionNote.bind(this); this.removeNote = this.removeNote.bind(this); @@ -114,6 +116,8 @@ export default class Notes { $(document).on('click', '.js-discussion-reply-button', this.onReplyToDiscussionNote); // add diff note $(document).on('click', '.js-add-diff-note-button', this.onAddDiffNote); + // add diff note for images + $(document).on('click', '.js-add-image-diff-note-button', this.onAddImageDiffNote); // hide diff note form $(document).on('click', '.js-close-discussion-note-form', this.cancelDiscussionForm); // toggle commit list @@ -140,6 +144,7 @@ export default class Notes { $(document).off('click', '.js-note-attachment-delete'); $(document).off('click', '.js-discussion-reply-button'); $(document).off('click', '.js-add-diff-note-button'); + $(document).off('click', '.js-add-image-diff-note-button'); $(document).off('visibilitychange'); $(document).off('keyup input', '.js-note-text'); $(document).off('click', '.js-note-target-reopen'); @@ -412,6 +417,11 @@ export default class Notes { this.note_ids.push(noteEntity.id); form = $form || $(`.js-discussion-note-form[data-discussion-id="${noteEntity.discussion_id}"]`); row = form.closest('tr'); + + if (noteEntity.on_image) { + row = form; + } + lineType = this.isParallelView() ? form.find('#line_type').val() : 'old'; diffAvatarContainer = row.prevAll('.line_holder').first().find('.js-avatar-container.' + lineType + '_line'); // is this the first note of discussion? @@ -423,7 +433,7 @@ export default class Notes { if (noteEntity.diff_discussion_html) { var $discussion = $(noteEntity.diff_discussion_html).renderGFM(); - if (!this.isParallelView() || row.hasClass('js-temp-notes-holder')) { + if (!this.isParallelView() || row.hasClass('js-temp-notes-holder') || noteEntity.on_image) { // insert the note and the reply button after the temp row row.after($discussion); } else { @@ -449,6 +459,7 @@ export default class Notes { if (typeof gl.diffNotesCompileComponents !== 'undefined' && noteEntity.discussion_resolvable) { gl.diffNotesCompileComponents(); + this.renderDiscussionAvatar(diffAvatarContainer, noteEntity); } @@ -561,7 +572,7 @@ export default class Notes { form.find('#note_line_code').val(), // DiffNote - form.find('#note_position').val() + form.find('#note_position').val(), ]; return new Autosave(textarea, key); } @@ -783,9 +794,22 @@ export default class Notes { $(`.js-diff-avatars-${discussionId}`).trigger('remove.vue'); // The notes tr can contain multiple lists of notes, like on the parallel diff - if (notesTr.find('.discussion-notes').length > 1) { + // notesTr does not exist for image diffs + if (notesTr.find('.discussion-notes').length > 1 || notesTr.length === 0) { + const $diffFile = $notes.closest('.diff-file'); + if ($diffFile.length > 0) { + const removeBadgeEvent = new CustomEvent('removeBadge.imageDiff', { + detail: { + // badgeNumber's start with 1 and index starts with 0 + badgeNumber: $notes.index() + 1, + }, + }); + + $diffFile[0].dispatchEvent(removeBadgeEvent); + } + $notes.remove(); - } else { + } else if (notesTr.length > 0) { notesTr.remove(); } } @@ -841,7 +865,11 @@ export default class Notes { */ setupDiscussionNoteForm(dataHolder, form) { // setup note target - const diffFileData = dataHolder.closest('.text-file'); + let diffFileData = dataHolder.closest('.text-file'); + + if (diffFileData.length === 0) { + diffFileData = dataHolder.closest('.image'); + } var discussionID = dataHolder.data('discussionId'); @@ -907,6 +935,31 @@ export default class Notes { }); } + onAddImageDiffNote(e) { + const $link = $(e.currentTarget || e.target); + const $diffFile = $link.closest('.diff-file'); + + const clickEvent = new CustomEvent('click.imageDiff', { + detail: e, + }); + + $diffFile[0].dispatchEvent(clickEvent); + + // Setup comment form + let newForm; + const $noteContainer = $link.closest('.diff-viewer').find('.note-container'); + const $form = $noteContainer.find('> .discussion-form'); + + if ($form.length === 0) { + newForm = this.cleanForm(this.formClone.clone()); + newForm.appendTo($noteContainer); + } else { + newForm = $form; + } + + this.setupDiscussionNoteForm($link, newForm); + } + toggleDiffNote({ target, lineType, @@ -999,10 +1052,25 @@ export default class Notes { } cancelDiscussionForm(e) { - var form; e.preventDefault(); - form = $(e.target).closest('.js-discussion-note-form'); - return this.removeDiscussionNoteForm(form); + const $form = $(e.target).closest('.js-discussion-note-form'); + const $discussionNote = $(e.target).closest('.discussion-notes'); + + if ($discussionNote.length === 0) { + // Only send blur event when the discussion form + // is not part of a discussion note + const $diffFile = $form.closest('.diff-file'); + + if ($diffFile.length > 0) { + const blurEvent = new CustomEvent('blur.imageDiff', { + detail: e, + }); + + $diffFile[0].dispatchEvent(blurEvent); + } + } + + return this.removeDiscussionNoteForm($form); } /** @@ -1414,6 +1482,15 @@ export default class Notes { // Submission successful! remove placeholder $notesContainer.find(`#${noteUniqueId}`).remove(); + const $diffFile = $form.closest('.diff-file'); + if ($diffFile.length > 0) { + const blurEvent = new CustomEvent('blur.imageDiff', { + detail: e, + }); + + $diffFile[0].dispatchEvent(blurEvent); + } + // Reset cached commands list when command is applied if (hasQuickActions) { $form.find('textarea.js-note-text').trigger('clear-commands-cache.atwho'); @@ -1436,7 +1513,28 @@ export default class Notes { } // Show final note element on UI - this.addDiscussionNote($form, note, $notesContainer.length === 0); + const isNewDiffComment = $notesContainer.length === 0; + this.addDiscussionNote($form, note, isNewDiffComment); + + if (isNewDiffComment) { + // Add image badge, avatar badge and toggle discussion badge for new image diffs + const notePosition = $form.find('#note_position').val(); + if ($diffFile.length > 0 && notePosition.length > 0) { + const { x, y, width, height } = JSON.parse(notePosition); + const addBadgeEvent = new CustomEvent('addBadge.imageDiff', { + detail: { + x, + y, + width, + height, + noteId: `note_${note.id}`, + discussionId: note.discussion_id, + }, + }); + + $diffFile[0].dispatchEvent(addBadgeEvent); + } + } // append flash-container to the Notes list if ($notesContainer.length) { @@ -1457,6 +1555,16 @@ export default class Notes { // Submission failed, remove placeholder note and show Flash error message $notesContainer.find(`#${noteUniqueId}`).remove(); + const blurEvent = new CustomEvent('blur.imageDiff', { + detail: e, + }); + + const closestDiffFile = $form.closest('.diff-file'); + + if (closestDiffFile.length) { + closestDiffFile[0].dispatchEvent(blurEvent); + } + if (hasQuickActions) { $notesContainer.find(`#${systemNoteUniqueId}`).remove(); } @@ -1500,6 +1608,8 @@ export default class Notes { const $noteBody = $editingNote.find('.js-task-list-container'); const $noteBodyText = $noteBody.find('.note-text'); const { formData, formContent, formAction } = this.getFormData($form); + const $diffFile = $form.closest('.diff-file'); + const $notesContainer = $form.closest('.notes'); // Cache original comment content const cachedNoteBodyText = $noteBodyText.html(); diff --git a/app/assets/javascripts/single_file_diff.js b/app/assets/javascripts/single_file_diff.js index 4505a79a2df..3f811c59cb9 100644 --- a/app/assets/javascripts/single_file_diff.js +++ b/app/assets/javascripts/single_file_diff.js @@ -1,6 +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'; +import imageDiffHelper from './image_diff/helpers/index'; const WRAPPER = '
'; const LOADING_HTML = ''; @@ -74,7 +75,11 @@ export default class SingleFileDiff { gl.diffNotesCompileComponents(); } - FilesCommentButton.init($(_this.file)); + const $file = $(_this.file); + FilesCommentButton.init($file); + + const canCreateNote = $file.closest('.files').is('[data-can-create-note]'); + imageDiffHelper.initImageDiff($file[0], canCreateNote); if (cb) cb(); }; diff --git a/app/assets/stylesheets/framework/buttons.scss b/app/assets/stylesheets/framework/buttons.scss index c77160a678b..b131e2d57ee 100644 --- a/app/assets/stylesheets/framework/buttons.scss +++ b/app/assets/stylesheets/framework/buttons.scss @@ -1,3 +1,25 @@ +@mixin btn-comment-icon { + border-radius: 50%; + background: $white-light; + padding: 1px 5px; + font-size: 12px; + color: $blue-500; + width: 23px; + height: 23px; + border: 1px solid $blue-500; + + &:hover, + &.inverted { + background: $blue-500; + border-color: $blue-600; + color: $white-light; + } + + &:active { + outline: 0; + } +} + @mixin btn-default { border-radius: 3px; font-size: $gl-font-size; diff --git a/app/assets/stylesheets/framework/timeline.scss b/app/assets/stylesheets/framework/timeline.scss index 3d68a50f91f..f718ec4bcad 100644 --- a/app/assets/stylesheets/framework/timeline.scss +++ b/app/assets/stylesheets/framework/timeline.scss @@ -17,15 +17,19 @@ .diff-file { border: 1px solid $border-color; - border-bottom: none; margin: 0; } + + &.text-file .diff-file { + border-bottom: none; + } } .timeline-entry { border-color: $white-normal; color: $gl-text-color; border-bottom: 1px solid $border-white-light; + background: $white-light; .timeline-entry-inner { position: relative; diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index 2f9def51d91..5f604680181 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/app/assets/stylesheets/framework/variables.scss @@ -323,6 +323,7 @@ $diff-image-info-color: grey; $diff-swipe-border: #999; $diff-view-modes-color: grey; $diff-view-modes-border: #c1c1c1; +$diff-jagged-border-gradient-color: darken($white-normal, 8%); /* * Fonts @@ -712,3 +713,9 @@ Issuable warning */ $issuable-warning-size: 24px; $issuable-warning-icon-margin: 4px; + +/* +Image Commenting cursor +*/ +$image-comment-cursor-left-offset: 12; +$image-comment-cursor-top-offset: 30; diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index fb23343b966..ffb5fc94475 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -297,6 +297,7 @@ .drag-track { display: block; position: absolute; + top: 0; left: 12px; height: 10px; width: 276px; @@ -547,16 +548,23 @@ } .diff-notes-collapse { - width: 19px; - height: 19px; + width: 24px; + height: 24px; + border-radius: 50%; padding: 0; transition: transform .1s ease-out; z-index: 100; - svg { - vertical-align: text-top; + .collapse-icon { + height: 50%; + width: 100%; } + svg { + vertical-align: middle; + } + + .collapse-icon, path { fill: $white-light; } @@ -644,3 +652,157 @@ text-overflow: ellipsis; white-space: nowrap; } + +.note-container { + background-color: $gray-light; + border-top: 1px solid $white-normal; + + // double jagged line divider + .discussion-notes + .discussion-notes::before, + .discussion-notes + .discussion-form::before { + content: ''; + position: relative; + display: block; + width: 100%; + height: 10px; + background-color: $white-light; + background-image: linear-gradient(45deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%), + linear-gradient(225deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%), + linear-gradient(135deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%), + linear-gradient(-45deg, transparent, transparent 73%, $diff-jagged-border-gradient-color 75%, $white-light 80%); + background-position: 5px 5px,0 5px,0 5px,5px 5px; + background-size: 10px 10px; + background-repeat: repeat; + } + + .notes { + position: relative; + } + + .diff-notes-collapse { + position: absolute; + left: -12px; + } +} + +.diff-file .note-container > .new-note, +.note-container .discussion-notes { + margin-left: 100px; + border-left: 1px solid $white-normal; +} + +.notes.active { + .diff-file .note-container > .new-note, + .note-container .discussion-notes { + // Override our margin and border (set for diff tab) + // when user is on the discussion tab for MR + margin-left: inherit; + border-left: inherit; + } +} + +.files:not([data-can-create-note]) .frame { + cursor: auto; +} + +.frame.click-to-comment { + position: relative; + cursor: url(icon_image_comment.svg) + $image-comment-cursor-left-offset $image-comment-cursor-top-offset, auto; + + // Retina cursor + cursor: -webkit-image-set(url(icon_image_comment.svg) 1x, url(icon_image_comment@2x.svg) 2x) + $image-comment-cursor-left-offset $image-comment-cursor-top-offset, auto; + + .comment-indicator { + position: absolute; + padding: 0; + width: (2px * $image-comment-cursor-left-offset); + height: (1px * $image-comment-cursor-top-offset); + // center the indicator to match the top left click region + margin-top: (-1px * $image-comment-cursor-top-offset) + 2; + margin-left: (-1px * $image-comment-cursor-left-offset) + 1; + + svg { + width: 100%; + height: 100%; + } + + &:focus { + outline: none; + } + } +} + +.frame .badge, +.image-diff-avatar-link .badge, +.notes > .badge { + position: absolute; + background-color: $blue-400; + color: $white-light; + border: $white-light 1px solid; + min-height: $gl-padding; + padding: 5px 8px; + border-radius: 12px; + + &:focus { + outline: none; + } +} + +.frame .badge, +.frame .image-comment-badge { + // Center align badges on the frame + transform: translate3d(-50%, -50%, 0); +} + +.image-comment-badge { + @include btn-comment-icon; + position: absolute; + + &.inverted { + border-color: $white-light; + } +} + +.image-diff-avatar-link { + position: relative; + + .badge, + .image-comment-badge { + top: 25px; + right: 8px; + } +} + +.notes > .badge { + display: none; + left: -13px; +} + +.discussion-notes { + min-height: 35px; + + &:first-child { + // First child does not have the jagged borders + min-height: 25px; + } + + &.collapsed { + background-color: $white-light; + + .diff-notes-collapse, + .note, + .discussion-reply-holder, { + display: none; + } + + .notes > .badge { + display: block; + } + } +} + +.discussion-body .image .frame { + position: relative; +} diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 420bca9ece5..04b132415eb 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -161,10 +161,13 @@ } .discussion-form { - padding: $gl-padding-top $gl-padding $gl-padding; background-color: $white-light; } +.discussion-form-container { + padding: $gl-padding-top $gl-padding $gl-padding; +} + .discussion-notes .disabled-comment { padding: 6px 0; } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 925fe4513ee..96b7db3b85d 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -650,29 +650,12 @@ ul.notes { } .add-diff-note { + @include btn-comment-icon; opacity: 0; margin-top: -2px; - border-radius: 50%; - background: $white-light; - padding: 1px 5px; - font-size: 12px; - color: $blue-500; margin-left: -55px; position: absolute; z-index: 10; - width: 23px; - height: 23px; - border: 1px solid $blue-500; - - &:hover { - background: $blue-500; - border-color: $blue-600; - color: $white-light; - } - - &:active { - outline: 0; - } } .discussion-body, diff --git a/app/controllers/concerns/notes_actions.rb b/app/controllers/concerns/notes_actions.rb index 915f32b4c33..1126f706393 100644 --- a/app/controllers/concerns/notes_actions.rb +++ b/app/controllers/concerns/notes_actions.rb @@ -96,7 +96,8 @@ module NotesActions id: note.id, discussion_id: note.discussion_id(noteable), html: note_html(note), - note: note.note + note: note.note, + on_image: note.try(:on_image?) ) discussion = note.to_discussion(noteable) @@ -122,7 +123,9 @@ module NotesActions def diff_discussion_html(discussion) return unless discussion.diff_discussion? - if params[:view] == 'parallel' + on_image = discussion.on_image? + + if params[:view] == 'parallel' && !on_image template = "discussions/_parallel_diff_discussion" locals = if params[:line_type] == 'old' @@ -132,7 +135,9 @@ module NotesActions end else template = "discussions/_diff_discussion" - locals = { discussions: [discussion] } + @fresh_discussion = true + + locals = { discussions: [discussion], on_image: on_image } end render_to_string( diff --git a/app/models/concerns/discussion_on_diff.rb b/app/models/concerns/discussion_on_diff.rb index eee1a36ac6b..f5cbb3becad 100644 --- a/app/models/concerns/discussion_on_diff.rb +++ b/app/models/concerns/discussion_on_diff.rb @@ -28,6 +28,10 @@ module DiscussionOnDiff true end + def file_new_path + first_note.position.new_path + end + # Returns an array of at most 16 highlighted lines above a diff note def truncated_diff_lines(highlight: true) lines = highlight ? highlighted_diff_lines : diff_lines diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 07c4846e2ac..6eba87da1a1 100644 --- a/app/models/diff_discussion.rb +++ b/app/models/diff_discussion.rb @@ -11,6 +11,8 @@ class DiffDiscussion < Discussion delegate :position, :original_position, :change_position, + :on_text?, + :on_image?, to: :first_note diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index e9a60e6ce09..d88a92dc027 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -12,8 +12,8 @@ class DiffNote < Note validates :original_position, presence: true validates :position, presence: true - validates :diff_line, presence: true - validates :line_code, presence: true, line_code: true + validates :diff_line, presence: true, if: :on_text? + validates :line_code, presence: true, line_code: true, if: :on_text? validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } validate :positions_complete validate :verify_supported @@ -43,6 +43,14 @@ class DiffNote < Note end end + def on_text? + position.position_type == "text" + end + + def on_image? + position.position_type == "image" + end + def diff_file @diff_file ||= self.original_position.diff_file(self.project.repository) end @@ -56,6 +64,8 @@ class DiffNote < Note end def original_line_code + return unless on_text? + self.diff_file.line_code(self.diff_line) end diff --git a/app/models/discussion.rb b/app/models/discussion.rb index b80da7b246a..437df923d2d 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -66,6 +66,10 @@ class Discussion @context_noteable = context_noteable end + def on_image? + false + end + def ==(other) other.class == self.class && other.context_noteable == self.context_noteable && diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb index 3c1d34db5fa..80fc6304fd4 100644 --- a/app/models/legacy_diff_discussion.rb +++ b/app/models/legacy_diff_discussion.rb @@ -17,6 +17,14 @@ class LegacyDiffDiscussion < Discussion true end + def on_image? + false + end + + def on_text? + true + end + def active?(*args) return @active if @active.present? diff --git a/app/models/note.rb b/app/models/note.rb index f44590e2144..ceded9f2aef 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -134,14 +134,22 @@ class Note < ActiveRecord::Base Discussion.build(notes) end + # Group diff discussions by line code or file path. + # It is not needed to group by line code when comment is + # on an image. def grouped_diff_discussions(diff_refs = nil) groups = {} diff_notes.fresh.discussions.each do |discussion| - line_code = discussion.line_code_in_diffs(diff_refs) + group_key = + if discussion.on_image? + discussion.file_new_path + else + discussion.line_code_in_diffs(diff_refs) + end - if line_code - discussions = groups[line_code] ||= [] + if group_key + discussions = groups[group_key] ||= [] discussions << discussion end end diff --git a/app/views/discussions/_diff_discussion.html.haml b/app/views/discussions/_diff_discussion.html.haml index e6d307e5568..52279d0a870 100644 --- a/app/views/discussions/_diff_discussion.html.haml +++ b/app/views/discussions/_diff_discussion.html.haml @@ -1,6 +1,10 @@ -- expanded = local_assigns.fetch(:expanded, true) -%tr.notes_holder{ class: ('hide' unless expanded) } - %td.notes_line{ colspan: 2 } - %td.notes_content - .content{ class: ('hide' unless expanded) } - = render partial: "discussions/notes", collection: discussions, as: :discussion +- if local_assigns[:on_image] + = render partial: "discussions/notes", collection: discussions, as: :discussion +- else + -# Text diff discussions + - expanded = local_assigns.fetch(:expanded, true) + %tr.notes_holder{ class: ('hide' unless expanded) } + %td.notes_line{ colspan: 2 } + %td.notes_content + .content{ class: ('hide' unless expanded) } + = render partial: "discussions/notes", collection: discussions, as: :discussion diff --git a/app/views/discussions/_diff_with_notes.html.haml b/app/views/discussions/_diff_with_notes.html.haml index 4a41be972da..636d06cab53 100644 --- a/app/views/discussions/_diff_with_notes.html.haml +++ b/app/views/discussions/_diff_with_notes.html.haml @@ -1,18 +1,27 @@ - diff_file = discussion.diff_file - blob = discussion.blob +- discussions = { discussion.original_line_code => [discussion] } +- diff_file_class = diff_file.text? ? 'text-file' : 'js-image-file' -.diff-file.file-holder +.diff-file.file-holder{ class: diff_file_class } .js-file-title.file-title.file-title-flex-parent .file-header-content = render "projects/diffs/file_header", diff_file: diff_file, url: discussion_path(discussion), show_toggle: false - .diff-content.code.js-syntax-highlight - %table - - discussions = { discussion.original_line_code => [discussion] } - = render partial: "projects/diffs/line", - collection: discussion.truncated_diff_lines, - as: :line, - locals: { diff_file: diff_file, - discussions: discussions, - discussion_expanded: true, - plain: true } + - if diff_file.text? + .diff-content.code.js-syntax-highlight + %table + = render partial: "projects/diffs/line", + collection: discussion.truncated_diff_lines, + as: :line, + locals: { diff_file: diff_file, + discussions: discussions, + discussion_expanded: true, + plain: true } + - else + - partial = (diff_file.new_file? || diff_file.deleted_file?) ? 'single_image_diff' : 'replaced_image_diff' + + = render partial: "projects/diffs/#{partial}", locals: { diff_file: diff_file, position: discussion.position.to_json, click_to_comment: false } + + .note-container + = render partial: "discussions/notes", locals: { discussion: discussion, show_toggle: false, show_image_comment_badge: true, disable_collapse: true } diff --git a/app/views/discussions/_notes.html.haml b/app/views/discussions/_notes.html.haml index db5ab939948..9efcfef690f 100644 --- a/app/views/discussions/_notes.html.haml +++ b/app/views/discussions/_notes.html.haml @@ -1,6 +1,19 @@ -.discussion-notes - %ul.notes{ data: { discussion_id: discussion.id } } - = render partial: "shared/notes/note", collection: discussion.notes, as: :note +- disable_collapse = local_assigns.fetch(:disable_collapse, false) +- collapsed_class = 'collapsed' if discussion.resolved? && !disable_collapse +- badge_counter = discussion_counter + 1 if local_assigns[:discussion_counter] +- show_toggle = local_assigns.fetch(:show_toggle, true) +- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false) + +.discussion-notes{ class: collapsed_class } + -# Save the first note position data so that we have a reference and can go + -# to the first note position when we click on a badge diff discussion + %ul.notes{ id: "discussion_#{discussion.id}", data: { discussion_id: discussion.id, position: discussion.notes[0].position.to_json } } + - if discussion.try(:on_image?) && show_toggle + %button.diff-notes-collapse.js-diff-notes-toggle{ type: 'button' } + = sprite_icon('collapse', css_class: 'collapse-icon') + %button.btn-transparent.badge.js-diff-notes-toggle{ type: 'button' } + = badge_counter + = render partial: "shared/notes/note", collection: discussion.notes, as: :note, locals: { badge_counter: badge_counter, show_image_comment_badge: show_image_comment_badge } .flash-container diff --git a/app/views/projects/diffs/_image_diff_frame.html.haml b/app/views/projects/diffs/_image_diff_frame.html.haml new file mode 100644 index 00000000000..dae73e10460 --- /dev/null +++ b/app/views/projects/diffs/_image_diff_frame.html.haml @@ -0,0 +1,5 @@ +- class_name = local_assigns.fetch(:class_name, '') +- note_type = local_assigns.fetch(:note_type, '') + +.frame{ class: class_name, data: { position: position, note_type: note_type } } + = image_tag(image_path, alt: alt, draggable: false, lazy: false) diff --git a/app/views/projects/diffs/_replaced_image_diff.html.haml b/app/views/projects/diffs/_replaced_image_diff.html.haml new file mode 100644 index 00000000000..8fc232b464e --- /dev/null +++ b/app/views/projects/diffs/_replaced_image_diff.html.haml @@ -0,0 +1,61 @@ +- blob = diff_file.blob +- old_blob = diff_file.old_blob +- blob_raw_path = diff_file_blob_raw_path(diff_file) +- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file) +- click_to_comment = local_assigns.fetch(:click_to_comment, true) +- diff_view_data = local_assigns.fetch(:diff_view_data, '') +- class_name = '' + +- if click_to_comment + - class_name = 'js-add-image-diff-note-button click-to-comment' + +.image.js-replaced-image{ data: diff_view_data } + .two-up.view + .wrap + .frame.deleted + = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false) + %p.image-info.hide + %span.meta-filesize= number_to_human_size(old_blob.size) + | + %strong W: + %span.meta-width + | + %strong H: + %span.meta-height + .wrap + = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path } + %p.image-info.hide + %span.meta-filesize= number_to_human_size(blob.size) + | + %strong W: + %span.meta-width + | + %strong H: + %span.meta-height + + .swipe.view.hide + .swipe-frame + .frame.deleted + = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false) + .swipe-wrap + = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path } + %span.swipe-bar + %span.top-handle + %span.bottom-handle + + .onion-skin.view.hide + .onion-skin-frame + .frame.deleted + = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false) + = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "added js-image-frame #{class_name}", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.new_path } + .controls + .transparent + .drag-track + .dragger{ :style => "left: 0px;" } + .opaque + +.view-modes.hide + %ul.view-modes-menu + %li.two-up{ data: { mode: 'two-up' } } 2-up + %li.swipe{ data: { mode: 'swipe' } } Swipe + %li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin diff --git a/app/views/projects/diffs/_single_image_diff.html.haml b/app/views/projects/diffs/_single_image_diff.html.haml new file mode 100644 index 00000000000..6b0c6bbe48f --- /dev/null +++ b/app/views/projects/diffs/_single_image_diff.html.haml @@ -0,0 +1,16 @@ +- blob = diff_file.blob +- old_blob = diff_file.old_blob +- blob_raw_path = diff_file_blob_raw_path(diff_file) +- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file) +- click_to_comment = local_assigns.fetch(:click_to_comment, true) +- diff_view_data = local_assigns.fetch(:diff_view_data, '') +- class_name = '' + +- if click_to_comment + - class_name = 'js-add-image-diff-note-button click-to-comment' + +.image.js-single-image{ data: diff_view_data } + .wrap + - single_class_name = diff_file.deleted_file? ? 'deleted' : 'added' + = render partial: "projects/diffs/image_diff_frame", locals: { class_name: "#{single_class_name} #{class_name} js-image-frame", position: position, note_type: DiffNote.name, image_path: blob_raw_path, alt: diff_file.file_path } + %p.image-info= number_to_human_size(blob.size) diff --git a/app/views/projects/diffs/viewers/_image.html.haml b/app/views/projects/diffs/viewers/_image.html.haml index 6b5233833c6..f190073c2fc 100644 --- a/app/views/projects/diffs/viewers/_image.html.haml +++ b/app/views/projects/diffs/viewers/_image.html.haml @@ -1,67 +1,13 @@ - diff_file = viewer.diff_file -- blob = diff_file.blob -- old_blob = diff_file.old_blob -- blob_raw_path = diff_file_blob_raw_path(diff_file) -- old_blob_raw_path = diff_file_old_blob_raw_path(diff_file) +- image_point = Gitlab::Diff::ImagePoint.new(nil, nil, nil, nil) +- discussions = @grouped_diff_discussions[diff_file.new_path] if @grouped_diff_discussions + +- locals = { diff_file: diff_file, position: diff_file.position(image_point, position_type: :image).to_json, click_to_comment: true, diff_view_data: diff_view_data } - if diff_file.new_file? || diff_file.deleted_file? - .image - %span.wrap - .frame{ class: (diff_file.deleted_file? ? 'deleted' : 'added') } - = image_tag(blob_raw_path, alt: diff_file.file_path) - %p.image-info= number_to_human_size(blob.size) + = render partial: "projects/diffs/single_image_diff", locals: locals - else - .image - .two-up.view - %span.wrap - .frame.deleted - = image_tag(old_blob_raw_path, alt: diff_file.old_path) - %p.image-info.hide - %span.meta-filesize= number_to_human_size(old_blob.size) - | - %b W: - %span.meta-width - | - %b H: - %span.meta-height - %span.wrap - .frame.added - = image_tag(blob_raw_path, alt: diff_file.new_path) - %p.image-info.hide - %span.meta-filesize= number_to_human_size(blob.size) - | - %b W: - %span.meta-width - | - %b H: - %span.meta-height + = render partial: "projects/diffs/replaced_image_diff", locals: locals - .swipe.view.hide - .swipe-frame - .frame.deleted - = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false) - .swipe-wrap - .frame.added - = image_tag(blob_raw_path, alt: diff_file.new_path, lazy: false) - %span.swipe-bar - %span.top-handle - %span.bottom-handle - - .onion-skin.view.hide - .onion-skin-frame - .frame.deleted - = image_tag(old_blob_raw_path, alt: diff_file.old_path, lazy: false) - .frame.added - = image_tag(blob_raw_path, alt: diff_file.new_path, lazy: false) - .controls - .transparent - .drag-track - .dragger{ :style => "left: 0px;" } - .opaque - - - .view-modes.hide - %ul.view-modes-menu - %li.two-up{ data: { mode: 'two-up' } } 2-up - %li.swipe{ data: { mode: 'swipe' } } Swipe - %li.onion-skin{ data: { mode: 'onion-skin' } } Onion skin +.note-container + = render partial: "discussions/notes", collection: discussions, as: :discussion diff --git a/app/views/shared/notes/_form.html.haml b/app/views/shared/notes/_form.html.haml index 725bf916592..71c0d740bc8 100644 --- a/app/views/shared/notes/_form.html.haml +++ b/app/views/shared/notes/_form.html.haml @@ -24,20 +24,21 @@ -# DiffNote = f.hidden_field :position - = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do - = render 'projects/zen', f: f, - attr: :note, - classes: 'note-textarea js-note-text', - placeholder: "Write a comment or drag your files here...", - supports_quick_actions: supports_quick_actions, - supports_autocomplete: supports_autocomplete - = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions - .error-alert + .discussion-form-container + = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do + = render 'projects/zen', f: f, + attr: :note, + classes: 'note-textarea js-note-text', + placeholder: "Write a comment or drag your files here...", + supports_quick_actions: supports_quick_actions, + supports_autocomplete: supports_autocomplete + = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions + .error-alert - .note-form-actions.clearfix - = render partial: 'shared/notes/comment_button' + .note-form-actions.clearfix + = render partial: 'shared/notes/comment_button' - = yield(:note_actions) + = yield(:note_actions) - %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } } - Discard draft + %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } } + Discard draft diff --git a/app/views/shared/notes/_note.html.haml b/app/views/shared/notes/_note.html.haml index 4f00a9f2759..b6085fd3af0 100644 --- a/app/views/shared/notes/_note.html.haml +++ b/app/views/shared/notes/_note.html.haml @@ -1,7 +1,10 @@ - return unless note.author - return if note.cross_reference_not_visible_for?(current_user) +- show_image_comment_badge = local_assigns.fetch(:show_image_comment_badge, false) - note_editable = note_editable?(note) +- note_counter = local_assigns.fetch(:note_counter, 0) + %li.timeline-entry{ id: dom_id(note), class: ["note", "note-row-#{note.id}", ('system-note' if note.system)], data: { author_id: note.author.id, @@ -12,8 +15,18 @@ - if note.system = icon_for_system_note(note) - else - %a{ href: user_path(note.author) } + %a.image-diff-avatar-link{ href: user_path(note.author) } = image_tag avatar_icon(note.author), alt: '', class: 'avatar s40' + - if note.is_a?(DiffNote) && note.on_image? + - if show_image_comment_badge && note_counter == 0 + -# Only show this for the first comment in the discussion + %span.image-comment-badge.inverted + = icon('comment-o') + - elsif note_counter == 0 + - counter = badge_counter if local_assigns[:badge_counter] + - badge_class = "hidden" if @fresh_discussion || counter.nil? + %span.badge{ class: badge_class } + = counter .timeline-content .note-header .note-header-info diff --git a/changelogs/unreleased/issue_35873.yml b/changelogs/unreleased/issue_35873.yml new file mode 100644 index 00000000000..65064b97e56 --- /dev/null +++ b/changelogs/unreleased/issue_35873.yml @@ -0,0 +1,5 @@ +--- +title: Commenting on image diffs +merge_request: 14061 +author: +type: added diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index fcac85ff892..599c3c5deab 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -27,16 +27,23 @@ module Gitlab @fallback_diff_refs = fallback_diff_refs end - def position(line) + def position(position_marker, position_type: :text) return unless diff_refs - Position.new( + data = { + diff_refs: diff_refs, + position_type: position_type.to_s, old_path: old_path, - new_path: new_path, - old_line: line.old_line, - new_line: line.new_line, - diff_refs: diff_refs - ) + new_path: new_path + } + + if position_type == :text + data.merge!(text_position_properties(position_marker)) + else + data.merge!(image_position_properties(position_marker)) + end + + Position.new(data) end def line_code(line) @@ -228,6 +235,14 @@ module Gitlab private + def text_position_properties(line) + { old_line: line.old_line, new_line: line.new_line } + end + + def image_position_properties(image_point) + image_point.to_h + end + def blobs_changed? old_blob && new_blob && old_blob.id != new_blob.id end diff --git a/lib/gitlab/diff/formatters/base_formatter.rb b/lib/gitlab/diff/formatters/base_formatter.rb new file mode 100644 index 00000000000..5e923b9e602 --- /dev/null +++ b/lib/gitlab/diff/formatters/base_formatter.rb @@ -0,0 +1,61 @@ +module Gitlab + module Diff + module Formatters + class BaseFormatter + attr_reader :old_path + attr_reader :new_path + attr_reader :base_sha + attr_reader :start_sha + attr_reader :head_sha + attr_reader :position_type + + def initialize(attrs) + if diff_file = attrs[:diff_file] + attrs[:diff_refs] = diff_file.diff_refs + attrs[:old_path] = diff_file.old_path + attrs[:new_path] = diff_file.new_path + end + + if diff_refs = attrs[:diff_refs] + attrs[:base_sha] = diff_refs.base_sha + attrs[:start_sha] = diff_refs.start_sha + attrs[:head_sha] = diff_refs.head_sha + end + + @old_path = attrs[:old_path] + @new_path = attrs[:new_path] + @base_sha = attrs[:base_sha] + @start_sha = attrs[:start_sha] + @head_sha = attrs[:head_sha] + end + + def key + [base_sha, start_sha, head_sha, Digest::SHA1.hexdigest(old_path || ""), Digest::SHA1.hexdigest(new_path || "")] + end + + def to_h + { + base_sha: base_sha, + start_sha: start_sha, + head_sha: head_sha, + old_path: old_path, + new_path: new_path, + position_type: position_type + } + end + + def position_type + raise NotImplementedError + end + + def ==(other) + raise NotImplementedError + end + + def complete? + raise NotImplementedError + end + end + end + end +end diff --git a/lib/gitlab/diff/formatters/image_formatter.rb b/lib/gitlab/diff/formatters/image_formatter.rb new file mode 100644 index 00000000000..ccd0d309972 --- /dev/null +++ b/lib/gitlab/diff/formatters/image_formatter.rb @@ -0,0 +1,43 @@ +module Gitlab + module Diff + module Formatters + class ImageFormatter < BaseFormatter + attr_reader :width + attr_reader :height + attr_reader :x + attr_reader :y + + def initialize(attrs) + @x = attrs[:x] + @y = attrs[:y] + @width = attrs[:width] + @height = attrs[:height] + + super(attrs) + end + + def key + @key ||= super.push(x, y) + end + + def complete? + x && y && width && height + end + + def to_h + super.merge(width: width, height: height, x: x, y: y) + end + + def position_type + "image" + end + + def ==(other) + other.is_a?(self.class) && + x == other.x && + y == other.y + end + end + end + end +end diff --git a/lib/gitlab/diff/formatters/text_formatter.rb b/lib/gitlab/diff/formatters/text_formatter.rb new file mode 100644 index 00000000000..01c7e9f51ab --- /dev/null +++ b/lib/gitlab/diff/formatters/text_formatter.rb @@ -0,0 +1,49 @@ +module Gitlab + module Diff + module Formatters + class TextFormatter < BaseFormatter + attr_reader :old_line + attr_reader :new_line + + def initialize(attrs) + @old_line = attrs[:old_line] + @new_line = attrs[:new_line] + + super(attrs) + end + + def key + @key ||= super.push(old_line, new_line) + end + + def complete? + old_line || new_line + end + + def to_h + super.merge(old_line: old_line, new_line: new_line) + end + + def line_age + if old_line && new_line + nil + elsif new_line + 'new' + else + 'old' + end + end + + def position_type + "text" + end + + def ==(other) + other.is_a?(self.class) && + new_line == other.new_line && + old_line == other.old_line + end + end + end + end +end diff --git a/lib/gitlab/diff/image_point.rb b/lib/gitlab/diff/image_point.rb new file mode 100644 index 00000000000..65332dfd239 --- /dev/null +++ b/lib/gitlab/diff/image_point.rb @@ -0,0 +1,23 @@ +module Gitlab + module Diff + class ImagePoint + attr_reader :width, :height, :x, :y + + def initialize(width, height, x, y) + @width = width + @height = height + @x = x + @y = y + end + + def to_h + { + width: width, + height: height, + x: x, + y: y + } + end + end + end +end diff --git a/lib/gitlab/diff/position.rb b/lib/gitlab/diff/position.rb index b8db3adef0a..bd0a9502a5e 100644 --- a/lib/gitlab/diff/position.rb +++ b/lib/gitlab/diff/position.rb @@ -1,37 +1,25 @@ -# Defines a specific location, identified by paths and line numbers, +# Defines a specific location, identified by paths line numbers and image coordinates, # within a specific diff, identified by start, head and base commit ids. module Gitlab module Diff class Position - attr_reader :old_path - attr_reader :new_path - attr_reader :old_line - attr_reader :new_line - attr_reader :base_sha - attr_reader :start_sha - attr_reader :head_sha + attr_accessor :formatter + delegate :old_path, + :new_path, + :base_sha, + :start_sha, + :head_sha, + :old_line, + :new_line, + :position_type, to: :formatter + + # A position can belong to a text line or to an image coordinate + # it depends of the position_type argument. + # Text position will have: new_line and old_line + # Image position will have: width, height, x, y def initialize(attrs = {}) - if diff_file = attrs[:diff_file] - attrs[:diff_refs] = diff_file.diff_refs - attrs[:old_path] = diff_file.old_path - attrs[:new_path] = diff_file.new_path - end - - if diff_refs = attrs[:diff_refs] - attrs[:base_sha] = diff_refs.base_sha - attrs[:start_sha] = diff_refs.start_sha - attrs[:head_sha] = diff_refs.head_sha - end - - @old_path = attrs[:old_path] - @new_path = attrs[:new_path] - @base_sha = attrs[:base_sha] - @start_sha = attrs[:start_sha] - @head_sha = attrs[:head_sha] - - @old_line = attrs[:old_line] - @new_line = attrs[:new_line] + @formatter = get_formatter_class(attrs[:position_type]).new(attrs) end # `Gitlab::Diff::Position` objects are stored as serialized attributes in @@ -46,7 +34,11 @@ module Gitlab end def encode_with(coder) - coder['attributes'] = self.to_h + coder['attributes'] = formatter.to_h + end + + def key + formatter.key end def ==(other) @@ -54,20 +46,11 @@ module Gitlab other.diff_refs == diff_refs && other.old_path == old_path && other.new_path == new_path && - other.old_line == old_line && - other.new_line == new_line + other.formatter == formatter end def to_h - { - old_path: old_path, - new_path: new_path, - old_line: old_line, - new_line: new_line, - base_sha: base_sha, - start_sha: start_sha, - head_sha: head_sha - } + formatter.to_h end def inspect @@ -75,23 +58,15 @@ module Gitlab end def complete? - file_path.present? && - (old_line || new_line) && - diff_refs.complete? + file_path.present? && formatter.complete? && diff_refs.complete? end def to_json(opts = nil) - JSON.generate(self.to_h, opts) + JSON.generate(formatter.to_h, opts) end def type - if old_line && new_line - nil - elsif new_line - 'new' - else - 'old' - end + formatter.line_age end def unchanged? @@ -150,6 +125,17 @@ module Gitlab diff_refs.compare_in(repository.project).diffs(paths: paths, expanded: true).diff_files.first end + + def get_formatter_class(type) + type ||= "text" + + case type + when 'image' + Gitlab::Diff::Formatters::ImageFormatter + else + Gitlab::Diff::Formatters::TextFormatter + end + end end end end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index cbec716d6ea..2c732aaf4ed 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -22,6 +22,11 @@ FactoryGirl.define do trait :with_diffs do end + trait :with_image_diffs do + source_branch "add_images_and_changes" + target_branch "master" + end + trait :without_diffs do source_branch "improve/awesome" target_branch "master" diff --git a/spec/features/merge_requests/diffs_spec.rb b/spec/features/merge_requests/diffs_spec.rb index e9068f722d5..f4ba660ca39 100644 --- a/spec/features/merge_requests/diffs_spec.rb +++ b/spec/features/merge_requests/diffs_spec.rb @@ -42,8 +42,12 @@ feature 'Diffs URL', js: true do visit "#{diffs_project_merge_request_path(project, merge_request)}#{fragment}" end - it 'shows expanded note' do - expect(page).to have_selector(fragment, visible: true) + it 'shows collapsed note' do + wait_for_requests + + expect(page).to have_selector('.discussion-notes.collapsed') do |note_container| + expect(note_container).to have_selector(fragment, visible: false) + end end end end diff --git a/spec/features/merge_requests/image_diff_notes.rb b/spec/features/merge_requests/image_diff_notes.rb new file mode 100644 index 00000000000..e8ca8a98d70 --- /dev/null +++ b/spec/features/merge_requests/image_diff_notes.rb @@ -0,0 +1,196 @@ +require 'spec_helper' + +feature 'image diff notes', js: true do + include NoteInteractionHelpers + + let(:user) { create(:user) } + let(:project) { create(:project, :public, :repository) } + + before do + project.team << [user, :master] + sign_in user + + page.driver.set_cookie('sidebar_collapsed', 'true') + + # Stub helper to return any blob file as image from public app folder. + # This is necessary to run this specs since we don't display repo images in capybara. + allow_any_instance_of(DiffHelper).to receive(:diff_file_blob_raw_path).and_return('/apple-touch-icon.png') + end + + context 'create commit diff notes' do + commit_id = '2f63565e7aac07bcdadb654e253078b727143ec4' + + describe 'create a new diff note' do + before do + visit project_commit_path(project, commit_id) + create_image_diff_note + end + + it 'shows indicator badge on image diff' do + indicator = find('.js-image-badge') + + expect(indicator).to have_content('1') + end + + it 'shows the avatar badge on the new note' do + badge = find('.image-diff-avatar-link .badge') + + expect(badge).to have_content('1') + end + + it 'allows collapsing/expanding the discussion notes' do + find('.js-diff-notes-toggle', :first).click + + expect(page).not_to have_content('image diff test comment') + + find('.js-diff-notes-toggle').click + + expect(page).to have_content('image diff test comment') + end + end + + describe 'render commit diff notes' do + let(:path) { "files/images/6049019_460s.jpg" } + let(:commit) { project.commit('2f63565e7aac07bcdadb654e253078b727143ec4') } + + let(:note1_position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + width: 100, + height: 100, + x: 10, + y: 10, + position_type: "image", + diff_refs: commit.diff_refs + ) + end + + let(:note2_position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + width: 100, + height: 100, + x: 20, + y: 20, + position_type: "image", + diff_refs: commit.diff_refs + ) + end + + let!(:note1) { create(:diff_note_on_commit, commit_id: commit.id, project: project, position: note1_position, note: 'my note 1') } + let!(:note2) { create(:diff_note_on_commit, commit_id: commit.id, project: project, position: note2_position, note: 'my note 2') } + + before do + visit project_commit_path(project, commit.id) + wait_for_requests + end + + it 'render diff indicators within the image diff frame' do + expect(page).to have_css('.js-image-badge', count: 2) + end + + it 'shows the diff notes' do + expect(page).to have_css('.diff-content .note', count: 2) + end + + it 'shows the diff notes with correct avatar badge numbers' do + expect(page).to have_css('.image-diff-avatar-link', text: 1) + expect(page).to have_css('.image-diff-avatar-link', text: 2) + end + end + end + + %w(inline parallel).each do |view| + context "#{view} view" do + let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) } + let(:path) { "files/images/ee_repo_logo.png" } + + let(:position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + width: 100, + height: 100, + x: 1, + y: 1, + position_type: "image", + diff_refs: merge_request.diff_refs + ) + end + + let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) } + + describe 'creating a new diff note' do + before do + visit diffs_project_merge_request_path(project, merge_request, view: view) + create_image_diff_note + end + + it 'shows indicator badge on image diff' do + indicator = find('.js-image-badge', match: :first) + + expect(indicator).to have_content('1') + end + + it 'shows the avatar badge on the new note' do + badge = find('.image-diff-avatar-link .badge', match: :first) + + expect(badge).to have_content('1') + end + + it 'allows expanding/collapsing the discussion notes' do + page.all('.js-diff-notes-toggle')[0].trigger('click') + page.all('.js-diff-notes-toggle')[1].trigger('click') + + expect(page).not_to have_content('image diff test comment') + + page.all('.js-diff-notes-toggle')[0].trigger('click') + page.all('.js-diff-notes-toggle')[1].trigger('click') + + expect(page).to have_content('image diff test comment') + end + end + end + end + + describe 'discussion tab polling', :js do + let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) } + let(:path) { "files/images/ee_repo_logo.png" } + + let(:position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + width: 100, + height: 100, + x: 50, + y: 50, + position_type: "image", + diff_refs: merge_request.diff_refs + ) + end + + before do + visit project_merge_request_path(project, merge_request) + end + + it 'render diff indicators within the image frame' do + diff_note = create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) + + wait_for_requests + + expect(page).to have_selector('.image-comment-badge') + expect(page).to have_content(diff_note.note) + end + end +end + +def create_image_diff_note + find('.js-add-image-diff-note-button', match: :first).click + page.all('.js-add-image-diff-note-button')[0].trigger('click') + find('.diff-content .note-textarea').native.send_keys('image diff test comment') + click_button 'Comment' + wait_for_requests +end diff --git a/spec/features/merge_requests/user_posts_diff_notes_spec.rb b/spec/features/merge_requests/user_posts_diff_notes_spec.rb index e1ca4fe186c..7a773fb2baa 100644 --- a/spec/features/merge_requests/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_requests/user_posts_diff_notes_spec.rb @@ -227,6 +227,7 @@ feature 'Merge requests > User posts diff notes', :js do write_comment_on_line(line_holder, diff_side) click_button 'Comment' + wait_for_requests assert_comment_persistence(line_holder, asset_form_reset: asset_form_reset) diff --git a/spec/javascripts/image_diff/helpers/badge_helper_spec.js b/spec/javascripts/image_diff/helpers/badge_helper_spec.js new file mode 100644 index 00000000000..fb9c7e59031 --- /dev/null +++ b/spec/javascripts/image_diff/helpers/badge_helper_spec.js @@ -0,0 +1,132 @@ +import * as badgeHelper from '~/image_diff/helpers/badge_helper'; +import * as mockData from '../mock_data'; + +describe('badge helper', () => { + const { coordinate, noteId, badgeText, badgeNumber } = mockData; + let containerEl; + let buttonEl; + + beforeEach(() => { + containerEl = document.createElement('div'); + }); + + describe('createImageBadge', () => { + beforeEach(() => { + buttonEl = badgeHelper.createImageBadge(noteId, coordinate); + }); + + it('should create button', () => { + expect(buttonEl.tagName).toEqual('BUTTON'); + expect(buttonEl.getAttribute('type')).toEqual('button'); + }); + + it('should set disabled attribute', () => { + expect(buttonEl.hasAttribute('disabled')).toEqual(true); + }); + + it('should set noteId', () => { + expect(buttonEl.dataset.noteId).toEqual(noteId); + }); + + it('should set coordinate', () => { + expect(buttonEl.style.left).toEqual(`${coordinate.x}px`); + expect(buttonEl.style.top).toEqual(`${coordinate.y}px`); + }); + + describe('classNames', () => { + it('should set .js-image-badge by default', () => { + expect(buttonEl.className).toEqual('js-image-badge'); + }); + + it('should add additional class names if parameter is passed', () => { + const classNames = ['first-class', 'second-class']; + buttonEl = badgeHelper.createImageBadge(noteId, coordinate, classNames); + + expect(buttonEl.className).toEqual(classNames.concat('js-image-badge').join(' ')); + }); + }); + }); + + describe('addImageBadge', () => { + beforeEach(() => { + badgeHelper.addImageBadge(containerEl, { + coordinate, + badgeText, + noteId, + }); + buttonEl = containerEl.querySelector('button'); + }); + + it('should appends button to container', () => { + expect(buttonEl).toBeDefined(); + }); + + it('should set the badge text', () => { + expect(buttonEl.innerText).toEqual(badgeText); + }); + + it('should set the button coordinates', () => { + expect(buttonEl.style.left).toEqual(`${coordinate.x}px`); + expect(buttonEl.style.top).toEqual(`${coordinate.y}px`); + }); + + it('should set the button noteId', () => { + expect(buttonEl.dataset.noteId).toEqual(noteId); + }); + }); + + describe('addImageCommentBadge', () => { + beforeEach(() => { + badgeHelper.addImageCommentBadge(containerEl, { + coordinate, + noteId, + }); + buttonEl = containerEl.querySelector('button'); + }); + + it('should append icon button to container', () => { + expect(buttonEl).toBeDefined(); + }); + + it('should create icon comment button', () => { + const iconEl = buttonEl.querySelector('i'); + expect(iconEl).toBeDefined(); + expect(iconEl.classList.contains('fa')).toEqual(true); + expect(iconEl.classList.contains('fa-comment-o')).toEqual(true); + }); + + it('should have .image-comment-badge.inverted in button class', () => { + expect(buttonEl.classList.contains('image-comment-badge')).toEqual(true); + expect(buttonEl.classList.contains('inverted')).toEqual(true); + }); + }); + + describe('addAvatarBadge', () => { + let avatarBadgeEl; + + beforeEach(() => { + containerEl.innerHTML = ` +
+ +
+ `; + + badgeHelper.addAvatarBadge(containerEl, { + detail: { + noteId, + badgeNumber, + }, + }); + avatarBadgeEl = containerEl.querySelector(`#${noteId} .badge`); + }); + + it('should update badge number', () => { + expect(avatarBadgeEl.innerText).toEqual(badgeNumber.toString()); + }); + + it('should remove hidden class', () => { + expect(avatarBadgeEl.classList.contains('hidden')).toEqual(false); + }); + }); +}); diff --git a/spec/javascripts/image_diff/helpers/comment_indicator_helper_spec.js b/spec/javascripts/image_diff/helpers/comment_indicator_helper_spec.js new file mode 100644 index 00000000000..a284b981d2a --- /dev/null +++ b/spec/javascripts/image_diff/helpers/comment_indicator_helper_spec.js @@ -0,0 +1,139 @@ +import * as commentIndicatorHelper from '~/image_diff/helpers/comment_indicator_helper'; +import * as mockData from '../mock_data'; + +describe('commentIndicatorHelper', () => { + const { coordinate } = mockData; + let containerEl; + + beforeEach(() => { + containerEl = document.createElement('div'); + }); + + describe('addCommentIndicator', () => { + let buttonEl; + + beforeEach(() => { + commentIndicatorHelper.addCommentIndicator(containerEl, coordinate); + buttonEl = containerEl.querySelector('button'); + }); + + it('should append button to container', () => { + expect(buttonEl).toBeDefined(); + }); + + describe('button', () => { + it('should set coordinate', () => { + expect(buttonEl.style.left).toEqual(`${coordinate.x}px`); + expect(buttonEl.style.top).toEqual(`${coordinate.y}px`); + }); + + it('should contain image-comment-dark svg', () => { + const svgEl = buttonEl.querySelector('svg'); + expect(svgEl).toBeDefined(); + + const svgLink = svgEl.querySelector('use').getAttribute('xlink:href'); + expect(svgLink.indexOf('image-comment-dark') !== -1).toEqual(true); + }); + }); + }); + + describe('removeCommentIndicator', () => { + it('should return removed false if there is no comment-indicator', () => { + const result = commentIndicatorHelper.removeCommentIndicator(containerEl); + expect(result.removed).toEqual(false); + }); + + describe('has comment indicator', () => { + let result; + + beforeEach(() => { + containerEl.innerHTML = ` +
+ +
+ `; + result = commentIndicatorHelper.removeCommentIndicator(containerEl); + }); + + it('should remove comment indicator', () => { + expect(containerEl.querySelector('.comment-indicator')).toBeNull(); + }); + + it('should return removed true', () => { + expect(result.removed).toEqual(true); + }); + + it('should return indicator meta', () => { + expect(result.x).toEqual(coordinate.x); + expect(result.y).toEqual(coordinate.y); + expect(result.image).toBeDefined(); + expect(result.image.width).toBeDefined(); + expect(result.image.height).toBeDefined(); + }); + }); + }); + + describe('showCommentIndicator', () => { + describe('commentIndicator exists', () => { + beforeEach(() => { + containerEl.innerHTML = ` + + `; + commentIndicatorHelper.showCommentIndicator(containerEl, coordinate); + }); + + it('should set commentIndicator coordinates', () => { + const commentIndicatorEl = containerEl.querySelector('.comment-indicator'); + expect(commentIndicatorEl.style.left).toEqual(`${coordinate.x}px`); + expect(commentIndicatorEl.style.top).toEqual(`${coordinate.y}px`); + }); + }); + + describe('commentIndicator does not exist', () => { + beforeEach(() => { + commentIndicatorHelper.showCommentIndicator(containerEl, coordinate); + }); + + it('should addCommentIndicator', () => { + const buttonEl = containerEl.querySelector('.comment-indicator'); + expect(buttonEl).toBeDefined(); + expect(buttonEl.style.left).toEqual(`${coordinate.x}px`); + expect(buttonEl.style.top).toEqual(`${coordinate.y}px`); + }); + }); + }); + + describe('commentIndicatorOnClick', () => { + let event; + let textAreaEl; + + beforeEach(() => { + containerEl.innerHTML = ` +
+ +
+ +
+
+ `; + textAreaEl = containerEl.querySelector('textarea'); + + event = { + stopPropagation: () => {}, + currentTarget: containerEl.querySelector('button'), + }; + + spyOn(event, 'stopPropagation'); + spyOn(textAreaEl, 'focus'); + commentIndicatorHelper.commentIndicatorOnClick(event); + }); + + it('should stopPropagation', () => { + expect(event.stopPropagation).toHaveBeenCalled(); + }); + + it('should focus textAreaEl', () => { + expect(textAreaEl.focus).toHaveBeenCalled(); + }); + }); +}); diff --git a/spec/javascripts/image_diff/helpers/dom_helper_spec.js b/spec/javascripts/image_diff/helpers/dom_helper_spec.js new file mode 100644 index 00000000000..8dde924e8ae --- /dev/null +++ b/spec/javascripts/image_diff/helpers/dom_helper_spec.js @@ -0,0 +1,118 @@ +import * as domHelper from '~/image_diff/helpers/dom_helper'; +import * as mockData from '../mock_data'; + +describe('domHelper', () => { + const { imageMeta, badgeNumber } = mockData; + + describe('setPositionDataAttribute', () => { + let containerEl; + let attributeAfterCall; + const position = { + myProperty: 'myProperty', + }; + + beforeEach(() => { + containerEl = document.createElement('div'); + containerEl.dataset.position = JSON.stringify(position); + domHelper.setPositionDataAttribute(containerEl, imageMeta); + attributeAfterCall = JSON.parse(containerEl.dataset.position); + }); + + it('should set x, y, width, height', () => { + expect(attributeAfterCall.x).toEqual(imageMeta.x); + expect(attributeAfterCall.y).toEqual(imageMeta.y); + expect(attributeAfterCall.width).toEqual(imageMeta.width); + expect(attributeAfterCall.height).toEqual(imageMeta.height); + }); + + it('should not override other properties', () => { + expect(attributeAfterCall.myProperty).toEqual('myProperty'); + }); + }); + + describe('updateDiscussionAvatarBadgeNumber', () => { + let discussionEl; + + beforeEach(() => { + discussionEl = document.createElement('div'); + discussionEl.innerHTML = ` + +
+ + `; + domHelper.updateDiscussionAvatarBadgeNumber(discussionEl, badgeNumber); + }); + + it('should update avatar badge number', () => { + expect(discussionEl.querySelector('.badge').innerText).toEqual(badgeNumber.toString()); + }); + }); + + describe('updateDiscussionBadgeNumber', () => { + let discussionEl; + + beforeEach(() => { + discussionEl = document.createElement('div'); + discussionEl.innerHTML = ` +
+ `; + domHelper.updateDiscussionBadgeNumber(discussionEl, badgeNumber); + }); + + it('should update discussion badge number', () => { + expect(discussionEl.querySelector('.badge').innerText).toEqual(badgeNumber.toString()); + }); + }); + + describe('toggleCollapsed', () => { + let element; + let discussionNotesEl; + + beforeEach(() => { + element = document.createElement('div'); + element.innerHTML = ` +
+ +
+
+ `; + discussionNotesEl = element.querySelector('.discussion-notes'); + }); + + describe('not collapsed', () => { + beforeEach(() => { + domHelper.toggleCollapsed({ + currentTarget: element.querySelector('button'), + }); + }); + + it('should add collapsed class', () => { + expect(discussionNotesEl.classList.contains('collapsed')).toEqual(true); + }); + + it('should force formEl to display none', () => { + const formEl = element.querySelector('.discussion-form'); + expect(formEl.style.display).toEqual('none'); + }); + }); + + describe('collapsed', () => { + beforeEach(() => { + discussionNotesEl.classList.add('collapsed'); + + domHelper.toggleCollapsed({ + currentTarget: element.querySelector('button'), + }); + }); + + it('should remove collapsed class', () => { + expect(discussionNotesEl.classList.contains('collapsed')).toEqual(false); + }); + + it('should force formEl to display block', () => { + const formEl = element.querySelector('.discussion-form'); + expect(formEl.style.display).toEqual('block'); + }); + }); + }); +}); diff --git a/spec/javascripts/image_diff/helpers/utils_helper_spec.js b/spec/javascripts/image_diff/helpers/utils_helper_spec.js new file mode 100644 index 00000000000..56d77a05c4c --- /dev/null +++ b/spec/javascripts/image_diff/helpers/utils_helper_spec.js @@ -0,0 +1,207 @@ +import * as utilsHelper from '~/image_diff/helpers/utils_helper'; +import ImageDiff from '~/image_diff/image_diff'; +import ReplacedImageDiff from '~/image_diff/replaced_image_diff'; +import ImageBadge from '~/image_diff/image_badge'; +import * as mockData from '../mock_data'; + +describe('utilsHelper', () => { + const { + noteId, + discussionId, + image, + imageProperties, + imageMeta, + } = mockData; + + describe('resizeCoordinatesToImageElement', () => { + let result; + + beforeEach(() => { + result = utilsHelper.resizeCoordinatesToImageElement(image, imageMeta); + }); + + it('should return x based on widthRatio', () => { + expect(result.x).toEqual(imageMeta.x * 0.5); + }); + + it('should return y based on heightRatio', () => { + expect(result.y).toEqual(imageMeta.y * 0.5); + }); + + it('should return image width', () => { + expect(result.width).toEqual(image.width); + }); + + it('should return image height', () => { + expect(result.height).toEqual(image.height); + }); + }); + + describe('generateBadgeFromDiscussionDOM', () => { + let discussionEl; + let result; + + beforeEach(() => { + const imageFrameEl = document.createElement('div'); + imageFrameEl.innerHTML = ` + + `; + discussionEl = document.createElement('div'); + discussionEl.dataset.discussionId = discussionId; + discussionEl.innerHTML = ` +
+ `; + discussionEl.dataset.position = JSON.stringify(imageMeta); + result = utilsHelper.generateBadgeFromDiscussionDOM(imageFrameEl, discussionEl); + }); + + it('should return actual image properties', () => { + const { actual } = result; + expect(actual.x).toEqual(imageMeta.x); + expect(actual.y).toEqual(imageMeta.y); + expect(actual.width).toEqual(imageMeta.width); + expect(actual.height).toEqual(imageMeta.height); + }); + + it('should return browser image properties', () => { + const { browser } = result; + expect(browser.x).toBeDefined(); + expect(browser.y).toBeDefined(); + expect(browser.width).toBeDefined(); + expect(browser.height).toBeDefined(); + }); + + it('should return instance of ImageBadge', () => { + expect(result instanceof ImageBadge).toEqual(true); + }); + + it('should return noteId', () => { + expect(result.noteId).toEqual(noteId); + }); + + it('should return discussionId', () => { + expect(result.discussionId).toEqual(discussionId); + }); + }); + + describe('getTargetSelection', () => { + let containerEl; + + beforeEach(() => { + containerEl = { + querySelector: () => imageProperties, + }; + }); + + function generateEvent(offsetX, offsetY) { + return { + currentTarget: containerEl, + offsetX, + offsetY, + }; + } + + it('should return browser properties', () => { + const event = generateEvent(25, 25); + const result = utilsHelper.getTargetSelection(event); + + const { browser } = result; + expect(browser.x).toEqual(event.offsetX); + expect(browser.y).toEqual(event.offsetY); + expect(browser.width).toEqual(imageProperties.width); + expect(browser.height).toEqual(imageProperties.height); + }); + + it('should return resized actual image properties', () => { + const event = generateEvent(50, 50); + const result = utilsHelper.getTargetSelection(event); + + const { actual } = result; + expect(actual.x).toEqual(100); + expect(actual.y).toEqual(100); + expect(actual.width).toEqual(imageProperties.naturalWidth); + expect(actual.height).toEqual(imageProperties.naturalHeight); + }); + + describe('normalize coordinates', () => { + it('should return x = 0 if x < 0', () => { + const event = generateEvent(-5, 50); + const result = utilsHelper.getTargetSelection(event); + expect(result.browser.x).toEqual(0); + }); + + it('should return x = width if x > width', () => { + const event = generateEvent(1000, 50); + const result = utilsHelper.getTargetSelection(event); + expect(result.browser.x).toEqual(imageProperties.width); + }); + + it('should return y = 0 if y < 0', () => { + const event = generateEvent(50, -10); + const result = utilsHelper.getTargetSelection(event); + expect(result.browser.y).toEqual(0); + }); + + it('should return y = height if y > height', () => { + const event = generateEvent(50, 1000); + const result = utilsHelper.getTargetSelection(event); + expect(result.browser.y).toEqual(imageProperties.height); + }); + }); + }); + + describe('initImageDiff', () => { + let glCache; + let fileEl; + + beforeEach(() => { + window.gl = window.gl || (window.gl = {}); + glCache = window.gl; + window.gl.ImageFile = () => {}; + fileEl = document.createElement('div'); + fileEl.innerHTML = ` +
+ `; + + spyOn(ImageDiff.prototype, 'init').and.callFake(() => {}); + spyOn(ReplacedImageDiff.prototype, 'init').and.callFake(() => {}); + }); + + afterEach(() => { + window.gl = glCache; + }); + + it('should initialize gl.ImageFile', () => { + spyOn(window.gl, 'ImageFile'); + + utilsHelper.initImageDiff(fileEl, false, false); + expect(gl.ImageFile).toHaveBeenCalled(); + }); + + it('should initialize ImageDiff if js-single-image', () => { + const diffFileEl = fileEl.querySelector('.diff-file'); + diffFileEl.innerHTML = ` +
+
+ `; + + const imageDiff = utilsHelper.initImageDiff(fileEl, true, false); + expect(ImageDiff.prototype.init).toHaveBeenCalled(); + expect(imageDiff.canCreateNote).toEqual(true); + expect(imageDiff.renderCommentBadge).toEqual(false); + }); + + it('should initialize ReplacedImageDiff if js-replaced-image', () => { + const diffFileEl = fileEl.querySelector('.diff-file'); + diffFileEl.innerHTML = ` +
+
+ `; + + const replacedImageDiff = utilsHelper.initImageDiff(fileEl, false, true); + expect(ReplacedImageDiff.prototype.init).toHaveBeenCalled(); + expect(replacedImageDiff.canCreateNote).toEqual(false); + expect(replacedImageDiff.renderCommentBadge).toEqual(true); + }); + }); +}); diff --git a/spec/javascripts/image_diff/image_badge_spec.js b/spec/javascripts/image_diff/image_badge_spec.js new file mode 100644 index 00000000000..87f98fc0926 --- /dev/null +++ b/spec/javascripts/image_diff/image_badge_spec.js @@ -0,0 +1,84 @@ +import ImageBadge from '~/image_diff/image_badge'; +import imageDiffHelper from '~/image_diff/helpers/index'; +import * as mockData from './mock_data'; + +describe('ImageBadge', () => { + const { noteId, discussionId, imageMeta } = mockData; + const options = { + noteId, + discussionId, + }; + + it('should save actual property', () => { + const imageBadge = new ImageBadge(Object.assign({}, options, { + actual: imageMeta, + })); + + const { actual } = imageBadge; + expect(actual.x).toEqual(imageMeta.x); + expect(actual.y).toEqual(imageMeta.y); + expect(actual.width).toEqual(imageMeta.width); + expect(actual.height).toEqual(imageMeta.height); + }); + + it('should save browser property', () => { + const imageBadge = new ImageBadge(Object.assign({}, options, { + browser: imageMeta, + })); + + const { browser } = imageBadge; + expect(browser.x).toEqual(imageMeta.x); + expect(browser.y).toEqual(imageMeta.y); + expect(browser.width).toEqual(imageMeta.width); + expect(browser.height).toEqual(imageMeta.height); + }); + + it('should save noteId', () => { + const imageBadge = new ImageBadge(options); + expect(imageBadge.noteId).toEqual(noteId); + }); + + it('should save discussionId', () => { + const imageBadge = new ImageBadge(options); + expect(imageBadge.discussionId).toEqual(discussionId); + }); + + describe('default values', () => { + let imageBadge; + + beforeEach(() => { + imageBadge = new ImageBadge(options); + }); + + it('should return defaultimageMeta if actual property is not provided', () => { + const { actual } = imageBadge; + expect(actual.x).toEqual(0); + expect(actual.y).toEqual(0); + expect(actual.width).toEqual(0); + expect(actual.height).toEqual(0); + }); + + it('should return defaultimageMeta if browser property is not provided', () => { + const { browser } = imageBadge; + expect(browser.x).toEqual(0); + expect(browser.y).toEqual(0); + expect(browser.width).toEqual(0); + expect(browser.height).toEqual(0); + }); + }); + + describe('imageEl property is provided and not browser property', () => { + beforeEach(() => { + spyOn(imageDiffHelper, 'resizeCoordinatesToImageElement').and.returnValue(true); + }); + + it('should generate browser property', () => { + const imageBadge = new ImageBadge(Object.assign({}, options, { + imageEl: document.createElement('img'), + })); + + expect(imageDiffHelper.resizeCoordinatesToImageElement).toHaveBeenCalled(); + expect(imageBadge.browser).toEqual(true); + }); + }); +}); diff --git a/spec/javascripts/image_diff/image_diff_spec.js b/spec/javascripts/image_diff/image_diff_spec.js new file mode 100644 index 00000000000..346282328c7 --- /dev/null +++ b/spec/javascripts/image_diff/image_diff_spec.js @@ -0,0 +1,361 @@ +import ImageDiff from '~/image_diff/image_diff'; +import * as imageUtility from '~/lib/utils/image_utility'; +import imageDiffHelper from '~/image_diff/helpers/index'; +import * as mockData from './mock_data'; + +describe('ImageDiff', () => { + let element; + let imageDiff; + + beforeEach(() => { + setFixtures(` +
+
+
+ +
+
1
+
2
+
3
+
+
+
+
+
+
+
+
+
+
+
+
+
+ `); + element = document.getElementById('element'); + }); + + describe('constructor', () => { + beforeEach(() => { + imageDiff = new ImageDiff(element, { + canCreateNote: true, + renderCommentBadge: true, + }); + }); + + it('should set el', () => { + expect(imageDiff.el).toEqual(element); + }); + + it('should set canCreateNote', () => { + expect(imageDiff.canCreateNote).toEqual(true); + }); + + it('should set renderCommentBadge', () => { + expect(imageDiff.renderCommentBadge).toEqual(true); + }); + + it('should set $noteContainer', () => { + expect(imageDiff.$noteContainer[0]).toEqual(element.querySelector('.note-container')); + }); + + describe('default', () => { + beforeEach(() => { + imageDiff = new ImageDiff(element); + }); + + it('should set canCreateNote as false', () => { + expect(imageDiff.canCreateNote).toEqual(false); + }); + + it('should set renderCommentBadge as false', () => { + expect(imageDiff.renderCommentBadge).toEqual(false); + }); + }); + }); + + describe('init', () => { + beforeEach(() => { + spyOn(ImageDiff.prototype, 'bindEvents').and.callFake(() => {}); + imageDiff = new ImageDiff(element); + imageDiff.init(); + }); + + it('should set imageFrameEl', () => { + expect(imageDiff.imageFrameEl).toEqual(element.querySelector('.diff-file .js-image-frame')); + }); + + it('should set imageEl', () => { + expect(imageDiff.imageEl).toEqual(element.querySelector('.diff-file .js-image-frame img')); + }); + + it('should call bindEvents', () => { + expect(imageDiff.bindEvents).toHaveBeenCalled(); + }); + }); + + describe('bindEvents', () => { + let imageEl; + + beforeEach(() => { + spyOn(imageDiffHelper, 'toggleCollapsed').and.callFake(() => {}); + spyOn(imageDiffHelper, 'commentIndicatorOnClick').and.callFake(() => {}); + spyOn(imageDiffHelper, 'removeCommentIndicator').and.callFake(() => {}); + spyOn(ImageDiff.prototype, 'imageClicked').and.callFake(() => {}); + spyOn(ImageDiff.prototype, 'addBadge').and.callFake(() => {}); + spyOn(ImageDiff.prototype, 'removeBadge').and.callFake(() => {}); + spyOn(ImageDiff.prototype, 'renderBadges').and.callFake(() => {}); + imageEl = element.querySelector('.diff-file .js-image-frame img'); + }); + + describe('default', () => { + beforeEach(() => { + spyOn(imageUtility, 'isImageLoaded').and.returnValue(false); + imageDiff = new ImageDiff(element); + imageDiff.imageEl = imageEl; + imageDiff.bindEvents(); + }); + + it('should register click event delegation to js-diff-notes-toggle', () => { + element.querySelector('.js-diff-notes-toggle').click(); + expect(imageDiffHelper.toggleCollapsed).toHaveBeenCalled(); + }); + + it('should register click event delegation to comment-indicator', () => { + element.querySelector('.comment-indicator').click(); + expect(imageDiffHelper.commentIndicatorOnClick).toHaveBeenCalled(); + }); + }); + + describe('image loaded', () => { + beforeEach(() => { + spyOn(imageUtility, 'isImageLoaded').and.returnValue(true); + imageDiff = new ImageDiff(element); + imageDiff.imageEl = imageEl; + }); + + it('should renderBadges', () => {}); + }); + + describe('image not loaded', () => { + beforeEach(() => { + spyOn(imageUtility, 'isImageLoaded').and.returnValue(false); + imageDiff = new ImageDiff(element); + imageDiff.imageEl = imageEl; + imageDiff.bindEvents(); + }); + + it('should registers load eventListener', () => { + const loadEvent = new Event('load'); + imageEl.dispatchEvent(loadEvent); + expect(imageDiff.renderBadges).toHaveBeenCalled(); + }); + }); + + describe('canCreateNote', () => { + beforeEach(() => { + spyOn(imageUtility, 'isImageLoaded').and.returnValue(false); + imageDiff = new ImageDiff(element, { + canCreateNote: true, + }); + imageDiff.imageEl = imageEl; + imageDiff.bindEvents(); + }); + + it('should register click.imageDiff event', () => { + const event = new CustomEvent('click.imageDiff'); + element.dispatchEvent(event); + expect(imageDiff.imageClicked).toHaveBeenCalled(); + }); + + it('should register blur.imageDiff event', () => { + const event = new CustomEvent('blur.imageDiff'); + element.dispatchEvent(event); + expect(imageDiffHelper.removeCommentIndicator).toHaveBeenCalled(); + }); + + it('should register addBadge.imageDiff event', () => { + const event = new CustomEvent('addBadge.imageDiff'); + element.dispatchEvent(event); + expect(imageDiff.addBadge).toHaveBeenCalled(); + }); + + it('should register removeBadge.imageDiff event', () => { + const event = new CustomEvent('removeBadge.imageDiff'); + element.dispatchEvent(event); + expect(imageDiff.removeBadge).toHaveBeenCalled(); + }); + }); + + describe('canCreateNote is false', () => { + beforeEach(() => { + spyOn(imageUtility, 'isImageLoaded').and.returnValue(false); + imageDiff = new ImageDiff(element); + imageDiff.imageEl = imageEl; + imageDiff.bindEvents(); + }); + + it('should not register click.imageDiff event', () => { + const event = new CustomEvent('click.imageDiff'); + element.dispatchEvent(event); + expect(imageDiff.imageClicked).not.toHaveBeenCalled(); + }); + }); + }); + + describe('imageClicked', () => { + beforeEach(() => { + spyOn(imageDiffHelper, 'getTargetSelection').and.returnValue({ + actual: {}, + browser: {}, + }); + spyOn(imageDiffHelper, 'setPositionDataAttribute').and.callFake(() => {}); + spyOn(imageDiffHelper, 'showCommentIndicator').and.callFake(() => {}); + imageDiff = new ImageDiff(element); + imageDiff.imageClicked({ + detail: { + currentTarget: {}, + }, + }); + }); + + it('should call getTargetSelection', () => { + expect(imageDiffHelper.getTargetSelection).toHaveBeenCalled(); + }); + + it('should call setPositionDataAttribute', () => { + expect(imageDiffHelper.setPositionDataAttribute).toHaveBeenCalled(); + }); + + it('should call showCommentIndicator', () => { + expect(imageDiffHelper.showCommentIndicator).toHaveBeenCalled(); + }); + }); + + describe('renderBadges', () => { + beforeEach(() => { + spyOn(ImageDiff.prototype, 'renderBadge').and.callFake(() => {}); + imageDiff = new ImageDiff(element); + imageDiff.renderBadges(); + }); + + it('should call renderBadge for each discussionEl', () => { + const discussionEls = element.querySelectorAll('.note-container .discussion-notes .notes'); + expect(imageDiff.renderBadge.calls.count()).toEqual(discussionEls.length); + }); + }); + + describe('renderBadge', () => { + let discussionEls; + + beforeEach(() => { + spyOn(imageDiffHelper, 'addImageBadge').and.callFake(() => {}); + spyOn(imageDiffHelper, 'addImageCommentBadge').and.callFake(() => {}); + spyOn(imageDiffHelper, 'generateBadgeFromDiscussionDOM').and.returnValue({ + browser: {}, + noteId: 'noteId', + }); + discussionEls = element.querySelectorAll('.note-container .discussion-notes .notes'); + imageDiff = new ImageDiff(element); + imageDiff.renderBadge(discussionEls[0], 0); + }); + + it('should populate imageBadges', () => { + expect(imageDiff.imageBadges.length).toEqual(1); + }); + + describe('renderCommentBadge', () => { + beforeEach(() => { + imageDiff.renderCommentBadge = true; + imageDiff.renderBadge(discussionEls[0], 0); + }); + + it('should call addImageCommentBadge', () => { + expect(imageDiffHelper.addImageCommentBadge).toHaveBeenCalled(); + }); + }); + + describe('renderCommentBadge is false', () => { + it('should call addImageBadge', () => { + expect(imageDiffHelper.addImageBadge).toHaveBeenCalled(); + }); + }); + }); + + describe('addBadge', () => { + beforeEach(() => { + spyOn(imageDiffHelper, 'addImageBadge').and.callFake(() => {}); + spyOn(imageDiffHelper, 'addAvatarBadge').and.callFake(() => {}); + spyOn(imageDiffHelper, 'updateDiscussionBadgeNumber').and.callFake(() => {}); + imageDiff = new ImageDiff(element); + imageDiff.imageFrameEl = element.querySelector('.diff-file .js-image-frame'); + imageDiff.addBadge({ + detail: { + x: 0, + y: 1, + width: 25, + height: 50, + noteId: 'noteId', + discussionId: 'discussionId', + }, + }); + }); + + it('should add imageBadge to imageBadges', () => { + expect(imageDiff.imageBadges.length).toEqual(1); + }); + + it('should call addImageBadge', () => { + expect(imageDiffHelper.addImageBadge).toHaveBeenCalled(); + }); + + it('should call addAvatarBadge', () => { + expect(imageDiffHelper.addAvatarBadge).toHaveBeenCalled(); + }); + + it('should call updateDiscussionBadgeNumber', () => { + expect(imageDiffHelper.updateDiscussionBadgeNumber).toHaveBeenCalled(); + }); + }); + + describe('removeBadge', () => { + beforeEach(() => { + const { imageMeta } = mockData; + + spyOn(imageDiffHelper, 'updateDiscussionBadgeNumber').and.callFake(() => {}); + spyOn(imageDiffHelper, 'updateDiscussionAvatarBadgeNumber').and.callFake(() => {}); + imageDiff = new ImageDiff(element); + imageDiff.imageBadges = [imageMeta, imageMeta, imageMeta]; + imageDiff.imageFrameEl = element.querySelector('.diff-file .js-image-frame'); + imageDiff.removeBadge({ + detail: { + badgeNumber: 2, + }, + }); + }); + + describe('cascade badge count', () => { + it('should update next imageBadgeEl value', () => { + const imageBadgeEls = imageDiff.imageFrameEl.querySelectorAll('.badge'); + expect(imageBadgeEls[0].innerText).toEqual('1'); + expect(imageBadgeEls[1].innerText).toEqual('2'); + expect(imageBadgeEls.length).toEqual(2); + }); + + it('should call updateDiscussionBadgeNumber', () => { + expect(imageDiffHelper.updateDiscussionBadgeNumber).toHaveBeenCalled(); + }); + + it('should call updateDiscussionAvatarBadgeNumber', () => { + expect(imageDiffHelper.updateDiscussionAvatarBadgeNumber).toHaveBeenCalled(); + }); + }); + + it('should remove badge from imageBadges', () => { + expect(imageDiff.imageBadges.length).toEqual(2); + }); + + it('should remove imageBadgeEl', () => { + expect(imageDiff.imageFrameEl.querySelector('#badge-2')).toBeNull(); + }); + }); +}); diff --git a/spec/javascripts/image_diff/init_discussion_tab_spec.js b/spec/javascripts/image_diff/init_discussion_tab_spec.js new file mode 100644 index 00000000000..7c447d6f70d --- /dev/null +++ b/spec/javascripts/image_diff/init_discussion_tab_spec.js @@ -0,0 +1,37 @@ +import initDiscussionTab from '~/image_diff/init_discussion_tab'; +import imageDiffHelper from '~/image_diff/helpers/index'; + +describe('initDiscussionTab', () => { + beforeEach(() => { + setFixtures(` +
+
+
+
+ `); + }); + + it('should pass canCreateNote as false to initImageDiff', (done) => { + spyOn(imageDiffHelper, 'initImageDiff').and.callFake((diffFileEl, canCreateNote) => { + expect(canCreateNote).toEqual(false); + done(); + }); + + initDiscussionTab(); + }); + + it('should pass renderCommentBadge as true to initImageDiff', (done) => { + spyOn(imageDiffHelper, 'initImageDiff').and.callFake((diffFileEl, canCreateNote, renderCommentBadge) => { + expect(renderCommentBadge).toEqual(true); + done(); + }); + + initDiscussionTab(); + }); + + it('should call initImageDiff for each diffFileEls', () => { + spyOn(imageDiffHelper, 'initImageDiff').and.callFake(() => {}); + initDiscussionTab(); + expect(imageDiffHelper.initImageDiff.calls.count()).toEqual(2); + }); +}); diff --git a/spec/javascripts/image_diff/mock_data.js b/spec/javascripts/image_diff/mock_data.js new file mode 100644 index 00000000000..a0d1732dd0a --- /dev/null +++ b/spec/javascripts/image_diff/mock_data.js @@ -0,0 +1,28 @@ +export const noteId = 'noteId'; +export const discussionId = 'discussionId'; +export const badgeText = 'badgeText'; +export const badgeNumber = 5; + +export const coordinate = { + x: 100, + y: 100, +}; + +export const image = { + width: 100, + height: 100, +}; + +export const imageProperties = { + width: image.width, + height: image.height, + naturalWidth: image.width * 2, + naturalHeight: image.height * 2, +}; + +export const imageMeta = { + x: coordinate.x, + y: coordinate.y, + width: imageProperties.naturalWidth, + height: imageProperties.naturalHeight, +}; diff --git a/spec/javascripts/image_diff/replaced_image_diff_spec.js b/spec/javascripts/image_diff/replaced_image_diff_spec.js new file mode 100644 index 00000000000..5f8cd7c531a --- /dev/null +++ b/spec/javascripts/image_diff/replaced_image_diff_spec.js @@ -0,0 +1,312 @@ +import ReplacedImageDiff from '~/image_diff/replaced_image_diff'; +import ImageDiff from '~/image_diff/image_diff'; +import { viewTypes } from '~/image_diff/view_types'; +import imageDiffHelper from '~/image_diff/helpers/index'; + +describe('ReplacedImageDiff', () => { + let element; + let replacedImageDiff; + + beforeEach(() => { + setFixtures(` +
+
+
+ +
+
+
+
+ +
+
+
+
+ +
+
+
+
2-up
+
Swipe
+
Onion skin
+
+
+ `); + element = document.getElementById('element'); + }); + + function setupImageFrameEls() { + replacedImageDiff.imageFrameEls = []; + replacedImageDiff.imageFrameEls[viewTypes.TWO_UP] = element.querySelector('.two-up .js-image-frame'); + replacedImageDiff.imageFrameEls[viewTypes.SWIPE] = element.querySelector('.swipe .js-image-frame'); + replacedImageDiff.imageFrameEls[viewTypes.ONION_SKIN] = element.querySelector('.onion-skin .js-image-frame'); + } + + function setupViewModesEls() { + replacedImageDiff.viewModesEls = []; + replacedImageDiff.viewModesEls[viewTypes.TWO_UP] = element.querySelector('.view-modes-menu .two-up'); + replacedImageDiff.viewModesEls[viewTypes.SWIPE] = element.querySelector('.view-modes-menu .swipe'); + replacedImageDiff.viewModesEls[viewTypes.ONION_SKIN] = element.querySelector('.view-modes-menu .onion-skin'); + } + + function setupImageEls() { + replacedImageDiff.imageEls = []; + replacedImageDiff.imageEls[viewTypes.TWO_UP] = element.querySelector('.two-up img'); + replacedImageDiff.imageEls[viewTypes.SWIPE] = element.querySelector('.swipe img'); + replacedImageDiff.imageEls[viewTypes.ONION_SKIN] = element.querySelector('.onion-skin img'); + } + + it('should extend ImageDiff', () => { + replacedImageDiff = new ReplacedImageDiff(element); + expect(replacedImageDiff instanceof ImageDiff).toEqual(true); + }); + + describe('init', () => { + beforeEach(() => { + spyOn(ReplacedImageDiff.prototype, 'bindEvents').and.callFake(() => {}); + spyOn(ReplacedImageDiff.prototype, 'generateImageEls').and.callFake(() => {}); + + replacedImageDiff = new ReplacedImageDiff(element); + replacedImageDiff.init(); + }); + + it('should set imageFrameEls', () => { + const { imageFrameEls } = replacedImageDiff; + expect(imageFrameEls).toBeDefined(); + expect(imageFrameEls[viewTypes.TWO_UP]).toEqual(element.querySelector('.two-up .js-image-frame')); + expect(imageFrameEls[viewTypes.SWIPE]).toEqual(element.querySelector('.swipe .js-image-frame')); + expect(imageFrameEls[viewTypes.ONION_SKIN]).toEqual(element.querySelector('.onion-skin .js-image-frame')); + }); + + it('should set viewModesEls', () => { + const { viewModesEls } = replacedImageDiff; + expect(viewModesEls).toBeDefined(); + expect(viewModesEls[viewTypes.TWO_UP]).toEqual(element.querySelector('.view-modes-menu .two-up')); + expect(viewModesEls[viewTypes.SWIPE]).toEqual(element.querySelector('.view-modes-menu .swipe')); + expect(viewModesEls[viewTypes.ONION_SKIN]).toEqual(element.querySelector('.view-modes-menu .onion-skin')); + }); + + it('should generateImageEls', () => { + expect(ReplacedImageDiff.prototype.generateImageEls).toHaveBeenCalled(); + }); + + it('should bindEvents', () => { + expect(ReplacedImageDiff.prototype.bindEvents).toHaveBeenCalled(); + }); + + describe('currentView', () => { + it('should set currentView', () => { + replacedImageDiff.init(viewTypes.ONION_SKIN); + expect(replacedImageDiff.currentView).toEqual(viewTypes.ONION_SKIN); + }); + + it('should default to viewTypes.TWO_UP', () => { + expect(replacedImageDiff.currentView).toEqual(viewTypes.TWO_UP); + }); + }); + }); + + describe('generateImageEls', () => { + beforeEach(() => { + spyOn(ReplacedImageDiff.prototype, 'bindEvents').and.callFake(() => {}); + + replacedImageDiff = new ReplacedImageDiff(element, { + canCreateNote: false, + renderCommentBadge: false, + }); + + setupImageFrameEls(); + }); + + it('should set imageEls', () => { + replacedImageDiff.generateImageEls(); + const { imageEls } = replacedImageDiff; + expect(imageEls).toBeDefined(); + expect(imageEls[viewTypes.TWO_UP]).toEqual(element.querySelector('.two-up img')); + expect(imageEls[viewTypes.SWIPE]).toEqual(element.querySelector('.swipe img')); + expect(imageEls[viewTypes.ONION_SKIN]).toEqual(element.querySelector('.onion-skin img')); + }); + }); + + describe('bindEvents', () => { + beforeEach(() => { + spyOn(ImageDiff.prototype, 'bindEvents').and.callFake(() => {}); + replacedImageDiff = new ReplacedImageDiff(element); + + setupViewModesEls(); + }); + + it('should call super.bindEvents', () => { + replacedImageDiff.bindEvents(); + expect(ImageDiff.prototype.bindEvents).toHaveBeenCalled(); + }); + + it('should register click eventlistener to 2-up view mode', (done) => { + spyOn(ReplacedImageDiff.prototype, 'changeView').and.callFake((viewMode) => { + expect(viewMode).toEqual(viewTypes.TWO_UP); + done(); + }); + + replacedImageDiff.bindEvents(); + replacedImageDiff.viewModesEls[viewTypes.TWO_UP].click(); + }); + + it('should register click eventlistener to swipe view mode', (done) => { + spyOn(ReplacedImageDiff.prototype, 'changeView').and.callFake((viewMode) => { + expect(viewMode).toEqual(viewTypes.SWIPE); + done(); + }); + + replacedImageDiff.bindEvents(); + replacedImageDiff.viewModesEls[viewTypes.SWIPE].click(); + }); + + it('should register click eventlistener to onion skin view mode', (done) => { + spyOn(ReplacedImageDiff.prototype, 'changeView').and.callFake((viewMode) => { + expect(viewMode).toEqual(viewTypes.SWIPE); + done(); + }); + + replacedImageDiff.bindEvents(); + replacedImageDiff.viewModesEls[viewTypes.SWIPE].click(); + }); + }); + + describe('getters', () => { + describe('imageEl', () => { + beforeEach(() => { + replacedImageDiff = new ReplacedImageDiff(element); + replacedImageDiff.currentView = viewTypes.TWO_UP; + setupImageEls(); + }); + + it('should return imageEl based on currentView', () => { + expect(replacedImageDiff.imageEl).toEqual(element.querySelector('.two-up img')); + + replacedImageDiff.currentView = viewTypes.SWIPE; + expect(replacedImageDiff.imageEl).toEqual(element.querySelector('.swipe img')); + }); + }); + + describe('imageFrameEl', () => { + beforeEach(() => { + replacedImageDiff = new ReplacedImageDiff(element); + replacedImageDiff.currentView = viewTypes.TWO_UP; + setupImageFrameEls(); + }); + + it('should return imageFrameEl based on currentView', () => { + expect(replacedImageDiff.imageFrameEl).toEqual(element.querySelector('.two-up .js-image-frame')); + + replacedImageDiff.currentView = viewTypes.ONION_SKIN; + expect(replacedImageDiff.imageFrameEl).toEqual(element.querySelector('.onion-skin .js-image-frame')); + }); + }); + }); + + describe('changeView', () => { + beforeEach(() => { + replacedImageDiff = new ReplacedImageDiff(element); + spyOn(imageDiffHelper, 'removeCommentIndicator').and.returnValue({ + removed: false, + }); + setupImageFrameEls(); + }); + + describe('invalid viewType', () => { + beforeEach(() => { + replacedImageDiff.changeView('some-view-name'); + }); + + it('should not call removeCommentIndicator', () => { + expect(imageDiffHelper.removeCommentIndicator).not.toHaveBeenCalled(); + }); + }); + + describe('valid viewType', () => { + beforeEach(() => { + jasmine.clock().install(); + spyOn(ReplacedImageDiff.prototype, 'renderNewView').and.callFake(() => {}); + replacedImageDiff.changeView(viewTypes.ONION_SKIN); + }); + + afterEach(() => { + jasmine.clock().uninstall(); + }); + + it('should call removeCommentIndicator', () => { + expect(imageDiffHelper.removeCommentIndicator).toHaveBeenCalled(); + }); + + it('should update currentView to newView', () => { + expect(replacedImageDiff.currentView).toEqual(viewTypes.ONION_SKIN); + }); + + it('should clear imageBadges', () => { + expect(replacedImageDiff.imageBadges.length).toEqual(0); + }); + + it('should call renderNewView', () => { + jasmine.clock().tick(251); + expect(replacedImageDiff.renderNewView).toHaveBeenCalled(); + }); + }); + }); + + describe('renderNewView', () => { + beforeEach(() => { + replacedImageDiff = new ReplacedImageDiff(element); + }); + + it('should call renderBadges', () => { + spyOn(ReplacedImageDiff.prototype, 'renderBadges').and.callFake(() => {}); + + replacedImageDiff.renderNewView({ + removed: false, + }); + + expect(replacedImageDiff.renderBadges).toHaveBeenCalled(); + }); + + describe('removeIndicator', () => { + const indicator = { + removed: true, + x: 0, + y: 1, + image: { + width: 50, + height: 100, + }, + }; + + beforeEach(() => { + setupImageEls(); + setupImageFrameEls(); + }); + + it('should pass showCommentIndicator normalized indicator values', (done) => { + spyOn(imageDiffHelper, 'showCommentIndicator').and.callFake(() => {}); + spyOn(imageDiffHelper, 'resizeCoordinatesToImageElement').and.callFake((imageEl, meta) => { + expect(meta.x).toEqual(indicator.x); + expect(meta.y).toEqual(indicator.y); + expect(meta.width).toEqual(indicator.image.width); + expect(meta.height).toEqual(indicator.image.height); + done(); + }); + replacedImageDiff.renderNewView(indicator); + }); + + it('should call showCommentIndicator', (done) => { + const normalized = { + normalized: true, + }; + spyOn(imageDiffHelper, 'resizeCoordinatesToImageElement').and.returnValue(normalized); + spyOn(imageDiffHelper, 'showCommentIndicator').and.callFake((imageFrameEl, normalizedIndicator) => { + expect(normalizedIndicator).toEqual(normalized); + done(); + }); + replacedImageDiff.renderNewView(indicator); + }); + }); + }); +}); diff --git a/spec/javascripts/image_diff/view_types_spec.js b/spec/javascripts/image_diff/view_types_spec.js new file mode 100644 index 00000000000..e9639f46497 --- /dev/null +++ b/spec/javascripts/image_diff/view_types_spec.js @@ -0,0 +1,24 @@ +import { viewTypes, isValidViewType } from '~/image_diff/view_types'; + +describe('viewTypes', () => { + describe('isValidViewType', () => { + it('should return true for TWO_UP', () => { + expect(isValidViewType(viewTypes.TWO_UP)).toEqual(true); + }); + + it('should return true for SWIPE', () => { + expect(isValidViewType(viewTypes.SWIPE)).toEqual(true); + }); + + it('should return true for ONION_SKIN', () => { + expect(isValidViewType(viewTypes.ONION_SKIN)).toEqual(true); + }); + + it('should return false for non view types', () => { + expect(isValidViewType('some-view-type')).toEqual(false); + expect(isValidViewType(null)).toEqual(false); + expect(isValidViewType(undefined)).toEqual(false); + expect(isValidViewType('')).toEqual(false); + }); + }); +}); diff --git a/spec/javascripts/lib/utils/image_utility_spec.js b/spec/javascripts/lib/utils/image_utility_spec.js new file mode 100644 index 00000000000..75addfcc833 --- /dev/null +++ b/spec/javascripts/lib/utils/image_utility_spec.js @@ -0,0 +1,32 @@ +import * as imageUtility from '~/lib/utils/image_utility'; + +describe('imageUtility', () => { + describe('isImageLoaded', () => { + it('should return false when image.complete is false', () => { + const element = { + complete: false, + naturalHeight: 100, + }; + + expect(imageUtility.isImageLoaded(element)).toEqual(false); + }); + + it('should return false when naturalHeight = 0', () => { + const element = { + complete: true, + naturalHeight: 0, + }; + + expect(imageUtility.isImageLoaded(element)).toEqual(false); + }); + + it('should return true when image.complete and naturalHeight != 0', () => { + const element = { + complete: true, + naturalHeight: 100, + }; + + expect(imageUtility.isImageLoaded(element)).toEqual(true); + }); + }); +}); diff --git a/spec/lib/gitlab/diff/formatters/image_formatter_spec.rb b/spec/lib/gitlab/diff/formatters/image_formatter_spec.rb new file mode 100644 index 00000000000..2f99febe04e --- /dev/null +++ b/spec/lib/gitlab/diff/formatters/image_formatter_spec.rb @@ -0,0 +1,20 @@ +require 'spec_helper' + +describe Gitlab::Diff::Formatters::ImageFormatter do + it_behaves_like "position formatter" do + let(:base_attrs) do + { + base_sha: 123, + start_sha: 456, + head_sha: 789, + old_path: 'old_image.png', + new_path: 'new_image.png', + position_type: 'image' + } + end + + let(:attrs) do + base_attrs.merge(width: 100, height: 100, x: 1, y: 2) + end + end +end diff --git a/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb new file mode 100644 index 00000000000..897dc917f6a --- /dev/null +++ b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb @@ -0,0 +1,42 @@ +require 'spec_helper' + +describe Gitlab::Diff::Formatters::TextFormatter do + let!(:base) do + { + base_sha: 123, + start_sha: 456, + head_sha: 789, + old_path: 'old_path.txt', + new_path: 'new_path.txt' + } + end + + let!(:complete) do + base.merge(old_line: 1, new_line: 2) + end + + it_behaves_like "position formatter" do + let(:base_attrs) { base } + + let(:attrs) { complete } + end + + # Specific text formatter examples + let!(:formatter) { described_class.new(attrs) } + + describe '#line_age' do + subject { formatter.line_age } + + context ' when there is only new_line' do + let(:attrs) { base.merge(new_line: 1) } + + it { is_expected.to eq('new') } + end + + context ' when there is only old_line' do + let(:attrs) { base.merge(old_line: 1) } + + it { is_expected.to eq('old') } + end + end +end diff --git a/spec/lib/gitlab/diff/position_spec.rb b/spec/lib/gitlab/diff/position_spec.rb index 7798736a4dc..9bf54fdecc4 100644 --- a/spec/lib/gitlab/diff/position_spec.rb +++ b/spec/lib/gitlab/diff/position_spec.rb @@ -5,7 +5,7 @@ describe Gitlab::Diff::Position do let(:project) { create(:project, :repository) } - describe "position for an added file" do + describe "position for an added text file" do let(:commit) { project.commit("2ea1f3dec713d940208fb5ce4a38765ecb5d3f73") } subject do @@ -47,6 +47,31 @@ describe Gitlab::Diff::Position do end end + describe "position for an added image file" do + let(:commit) { project.commit("33f3729a45c02fc67d00adb1b8bca394b0e761d9") } + + subject do + described_class.new( + old_path: "files/images/6049019_460s.jpg", + new_path: "files/images/6049019_460s.jpg", + width: 100, + height: 100, + x: 1, + y: 100, + diff_refs: commit.diff_refs, + position_type: "image" + ) + end + + it "returns the correct diff file" do + diff_file = subject.diff_file(project.repository) + + expect(diff_file.new_file?).to be true + expect(diff_file.new_path).to eq(subject.new_path) + expect(diff_file.diff_refs).to eq(subject.diff_refs) + end + end + describe "position for a changed file" do let(:commit) { project.commit("570e7b2abdd848b95f2f578043fc23bd6f6fd24d") } @@ -468,26 +493,54 @@ describe Gitlab::Diff::Position do end describe "#to_json" do - let(:hash) do - { - old_path: "files/ruby/popen.rb", - new_path: "files/ruby/popen.rb", - old_line: nil, - new_line: 14, - base_sha: nil, - head_sha: nil, - start_sha: nil - } + shared_examples "diff position json" do + it "returns the position as JSON" do + expect(JSON.parse(diff_position.to_json)).to eq(hash.stringify_keys) + end + + it "works when nested under another hash" do + expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => hash.stringify_keys) + end end - let(:diff_position) { described_class.new(hash) } + context "for text positon" do + let(:hash) do + { + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + base_sha: nil, + head_sha: nil, + start_sha: nil, + position_type: "text" + } + end - it "returns the position as JSON" do - expect(JSON.parse(diff_position.to_json)).to eq(hash.stringify_keys) + let(:diff_position) { described_class.new(hash) } + + it_behaves_like "diff position json" end - it "works when nested under another hash" do - expect(JSON.parse(JSON.generate(pos: diff_position))).to eq('pos' => hash.stringify_keys) + context "for image positon" do + let(:hash) do + { + old_path: "files/any.img", + new_path: "files/any.img", + base_sha: nil, + head_sha: nil, + start_sha: nil, + width: 100, + height: 100, + x: 1, + y: 100, + position_type: "image" + } + end + + let(:diff_position) { described_class.new(hash) } + + it_behaves_like "diff position json" end end end diff --git a/spec/lib/gitlab/diff/position_tracer_spec.rb b/spec/lib/gitlab/diff/position_tracer_spec.rb index 4fa30d8df8b..e5138705443 100644 --- a/spec/lib/gitlab/diff/position_tracer_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer_spec.rb @@ -71,6 +71,10 @@ describe Gitlab::Diff::PositionTracer do Gitlab::Diff::DiffRefs.new(base_sha: base_commit.id, head_sha: head_commit.id) end + def text_position_attrs + [:old_line, :new_line] + end + def position(attrs = {}) attrs.reverse_merge!( diff_refs: old_diff_refs @@ -91,7 +95,11 @@ describe Gitlab::Diff::PositionTracer do expect(new_position.diff_refs).to eq(new_diff_refs) attrs.each do |attr, value| - expect(new_position.send(attr)).to eq(value) + if text_position_attrs.include?(attr) + expect(new_position.formatter.send(attr)).to eq(value) + else + expect(new_position.send(attr)).to eq(value) + end end end end @@ -110,7 +118,11 @@ describe Gitlab::Diff::PositionTracer do expect(change_position.diff_refs).to eq(change_diff_refs) attrs.each do |attr, value| - expect(change_position.send(attr)).to eq(value) + if text_position_attrs.include?(attr) + expect(change_position.formatter.send(attr)).to eq(value) + else + expect(change_position.send(attr)).to eq(value) + end end end end diff --git a/spec/models/diff_note_spec.rb b/spec/models/diff_note_spec.rb index 4aa9ec789a3..eb0a3e9e0d3 100644 --- a/spec/models/diff_note_spec.rb +++ b/spec/models/diff_note_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe DiffNote do include RepoHelpers - let(:merge_request) { create(:merge_request) } + let!(:merge_request) { create(:merge_request) } let(:project) { merge_request.project } let(:commit) { project.commit(sample_commit.id) } @@ -98,14 +98,14 @@ describe DiffNote do diff_line = subject.diff_line expect(diff_line.added?).to be true - expect(diff_line.new_line).to eq(position.new_line) + expect(diff_line.new_line).to eq(position.formatter.new_line) expect(diff_line.text).to eq("+ vars = {") end end describe "#line_code" do it "returns the correct line code" do - line_code = Gitlab::Diff::LineCode.generate(position.file_path, position.new_line, 15) + line_code = Gitlab::Diff::LineCode.generate(position.file_path, position.formatter.new_line, 15) expect(subject.line_code).to eq(line_code) end @@ -255,4 +255,38 @@ describe DiffNote do end end end + + describe "image diff notes" do + let(:path) { "files/images/any_image.png" } + + let!(:position) do + Gitlab::Diff::Position.new( + old_path: path, + new_path: path, + width: 10, + height: 10, + x: 1, + y: 1, + diff_refs: merge_request.diff_refs, + position_type: "image" + ) + end + + describe "validations" do + subject { build(:diff_note_on_merge_request, project: project, position: position, noteable: merge_request) } + + it { is_expected.not_to validate_presence_of(:line_code) } + + it "does not validate diff line" do + diff_line = subject.diff_line + + expect(diff_line).to be nil + expect(subject).to be_valid + end + end + + it "returns true for on_image?" do + expect(subject.on_image?).to be_truthy + end + end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index b214074fdce..1ecb50586c7 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -314,6 +314,56 @@ describe Note do expect(subject[active_diff_note1.line_code].first.id).to eq(active_diff_note1.discussion_id) expect(subject[active_diff_note3.line_code].first.id).to eq(active_diff_note3.discussion_id) end + + context 'with image discussions' do + let(:merge_request2) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, title: "Added images and changes") } + let(:image_path) { "files/images/ee_repo_logo.png" } + let(:text_path) { "bar/branch-test.txt" } + let!(:image_note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request2, position: image_position) } + let!(:text_note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request2, position: text_position) } + + let(:image_position) do + Gitlab::Diff::Position.new( + old_path: image_path, + new_path: image_path, + width: 100, + height: 100, + x: 1, + y: 1, + position_type: "image", + diff_refs: merge_request2.diff_refs + ) + end + + let(:text_position) do + Gitlab::Diff::Position.new( + old_path: text_path, + new_path: text_path, + old_line: nil, + new_line: 2, + position_type: "text", + diff_refs: merge_request2.diff_refs + ) + end + + it "groups image discussions by file identifier" do + diff_discussion = DiffDiscussion.new([image_note]) + + discussions = merge_request2.notes.grouped_diff_discussions + + expect(discussions.size).to eq(2) + expect(discussions[image_note.diff_file.new_path]).to include(diff_discussion) + end + + it "groups text discussions by line code" do + diff_discussion = DiffDiscussion.new([text_note]) + + discussions = merge_request2.notes.grouped_diff_discussions + + expect(discussions.size).to eq(2) + expect(discussions[text_note.line_code]).to include(diff_discussion) + end + end end context 'diff discussions for older diff refs' do diff --git a/spec/services/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb index 82b156f5ebe..2b84206318f 100644 --- a/spec/services/discussions/update_diff_position_service_spec.rb +++ b/spec/services/discussions/update_diff_position_service_spec.rb @@ -164,8 +164,8 @@ describe Discussions::UpdateDiffPositionService do change_position = discussion.change_position expect(change_position.start_sha).to eq(old_diff_refs.head_sha) expect(change_position.head_sha).to eq(new_diff_refs.head_sha) - expect(change_position.old_line).to eq(9) - expect(change_position.new_line).to be_nil + expect(change_position.formatter.old_line).to eq(9) + expect(change_position.formatter.new_line).to be_nil end it 'creates a system discussion' do @@ -184,7 +184,7 @@ describe Discussions::UpdateDiffPositionService do expect(discussion.original_position).to eq(old_position) expect(discussion.position).not_to eq(old_position) - expect(discussion.position.new_line).to eq(22) + expect(discussion.position.formatter.new_line).to eq(22) end context 'when the resolve_outdated_diff_discussions setting is set' do diff --git a/spec/support/features/discussion_comments_shared_example.rb b/spec/support/features/discussion_comments_shared_example.rb index 81cb94ab8c4..9f05cabf7ae 100644 --- a/spec/support/features/discussion_comments_shared_example.rb +++ b/spec/support/features/discussion_comments_shared_example.rb @@ -71,7 +71,7 @@ shared_examples 'discussion comments' do |resource_name| expect(page).not_to have_selector menu_selector find(toggle_selector).click - find('body').click + find('body').trigger 'click' expect(page).not_to have_selector menu_selector end diff --git a/spec/support/shared_examples/position_formatters.rb b/spec/support/shared_examples/position_formatters.rb new file mode 100644 index 00000000000..ffc9456dbc7 --- /dev/null +++ b/spec/support/shared_examples/position_formatters.rb @@ -0,0 +1,43 @@ +shared_examples_for "position formatter" do + let(:formatter) { described_class.new(attrs) } + + describe '#key' do + let(:key) { [123, 456, 789, Digest::SHA1.hexdigest(formatter.old_path), Digest::SHA1.hexdigest(formatter.new_path), 1, 2] } + + subject { formatter.key } + + it { is_expected.to eq(key) } + end + + describe '#complete?' do + subject { formatter.complete? } + + context 'when there are missing key attributes' do + it { is_expected.to be_truthy } + end + + context 'when old_line and new_line are nil' do + let(:attrs) { base_attrs } + + it { is_expected.to be_falsy } + end + end + + describe '#to_h' do + let(:formatter_hash) do + attrs.merge(position_type: base_attrs[:position_type] || 'text' ) + end + + subject { formatter.to_h } + + it { is_expected.to eq(formatter_hash) } + end + + describe '#==' do + subject { formatter } + + let(:other_formatter) { described_class.new(attrs) } + + it { is_expected.to eq(other_formatter) } + end +end diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index 79395f4c564..a27bfdee3d2 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -46,7 +46,8 @@ module TestEnv 'v1.1.0' => 'b83d6e3', 'add-ipython-files' => '93ee732', 'add-pdf-file' => 'e774ebd', - 'add-pdf-text-binary' => '79faa7b' + 'add-pdf-text-binary' => '79faa7b', + 'add_images_and_changes' => '010d106' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily