From f7df9ddb52be8a03b8fbd8c9a870f3e3af577562 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 18 Oct 2018 08:50:38 +0100 Subject: Re-implemented image commenting on diffs This re-implements image commenting in merge request diffs. This feature was previously lost when the merge request page was refactored into Vue. With this, we create an overlay component. The overlay component handles displaying the comment badges and the comment form badge. Badges are displayed based on the position attribute sent with the discussion. Comment forms for diff files are controlled through a different state property. This is so we don't tie comment forms to diff files directly creating deep nested state. Instead we create a flat array which holds the file hash & the X & Y position of the comment form. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/48956 --- app/assets/javascripts/diffs/components/app.vue | 5 +- .../javascripts/diffs/components/diff_content.vue | 70 +++++++++++- .../diffs/components/diff_discussions.vue | 54 ++++++++- .../diffs/components/image_diff_overlay.vue | 115 ++++++++++++++++++++ app/assets/javascripts/diffs/constants.js | 1 + app/assets/javascripts/diffs/store/actions.js | 19 +++- app/assets/javascripts/diffs/store/getters.js | 3 + .../javascripts/diffs/store/modules/diff_state.js | 2 + .../javascripts/diffs/store/mutation_types.js | 4 + app/assets/javascripts/diffs/store/mutations.js | 53 +++++++-- app/assets/javascripts/diffs/store/utils.js | 18 ++- app/assets/javascripts/merge_request_tabs.js | 3 - .../notes/components/diff_with_note.vue | 36 ++++-- .../notes/components/noteable_discussion.vue | 11 +- .../javascripts/notes/components/noteable_note.vue | 8 +- .../content_viewer/viewers/image_viewer.vue | 83 ++++++++------ .../components/diff_viewer/diff_viewer.vue | 9 +- .../viewers/image_diff/onion_skin_viewer.vue | 15 ++- .../viewers/image_diff/swipe_viewer.vue | 45 ++++---- .../viewers/image_diff/two_up_viewer.vue | 37 ++++--- .../diff_viewer/viewers/image_diff_viewer.vue | 79 ++++++++------ .../components/user_avatar/user_avatar_link.vue | 1 + app/assets/stylesheets/pages/diff.scss | 47 +++++--- changelogs/unreleased/mr-image-commenting.yml | 5 + lib/gitlab/diff/formatters/image_formatter.rb | 2 +- locale/gitlab.pot | 9 ++ .../user_creates_image_diff_notes_spec.rb | 9 +- .../diffs/components/diff_content_spec.js | 51 +++++++-- .../diffs/components/diff_discussions_spec.js | 78 ++++++++++++- .../diffs/components/image_diff_overlay_spec.js | 121 +++++++++++++++++++++ .../diffs/mock_data/diff_discussions.js | 17 +++ spec/javascripts/diffs/mock_data/diff_file.js | 1 + spec/javascripts/diffs/store/actions_spec.js | 2 + .../notes/components/diff_with_note_spec.js | 2 +- .../content_viewer/content_viewer_spec.js | 2 +- .../components/diff_viewer/diff_viewer_spec.js | 4 +- .../diff_viewer/viewers/image_diff_viewer_spec.js | 16 +-- 37 files changed, 832 insertions(+), 205 deletions(-) create mode 100644 app/assets/javascripts/diffs/components/image_diff_overlay.vue create mode 100644 changelogs/unreleased/mr-image-commenting.yml create mode 100644 spec/javascripts/diffs/components/image_diff_overlay_spec.js diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 59680959bb1..9c0f72f177f 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -223,7 +223,10 @@ export default { :commit="commit" /> -
+
-import { mapGetters, mapState } from 'vuex'; +import { mapActions, mapGetters, mapState } from 'vuex'; import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue'; -import { diffModes } from '~/ide/constants'; import InlineDiffView from './inline_diff_view.vue'; import ParallelDiffView from './parallel_diff_view.vue'; +import NoteForm from '../../notes/components/note_form.vue'; +import ImageDiffOverlay from './image_diff_overlay.vue'; +import DiffDiscussions from './diff_discussions.vue'; +import { IMAGE_DIFF_POSITION_TYPE } from '../constants'; +import { getDiffMode } from '../store/utils'; export default { components: { InlineDiffView, ParallelDiffView, DiffViewer, + NoteForm, + DiffDiscussions, + ImageDiffOverlay, }, props: { diffFile: { @@ -23,13 +30,36 @@ export default { endpoint: state => state.diffs.endpoint, }), ...mapGetters('diffs', ['isInlineView', 'isParallelView']), + ...mapGetters('diffs', ['getCommentFormForDiffFile']), + ...mapGetters(['getNoteableData', 'noteableType']), diffMode() { - const diffModeKey = Object.keys(diffModes).find(key => this.diffFile[`${key}File`]); - return diffModes[diffModeKey] || diffModes.replaced; + return getDiffMode(this.diffFile); }, isTextFile() { return this.diffFile.viewer.name === 'text'; }, + diffFileCommentForm() { + return this.getCommentFormForDiffFile(this.diffFile.fileHash); + }, + showNotesContainer() { + return this.diffFile.discussions.length || this.diffFileCommentForm; + }, + }, + methods: { + ...mapActions('diffs', ['saveDiffDiscussion', 'closeDiffFileCommentForm']), + handleSaveNote(note) { + this.saveDiffDiscussion({ + note, + formData: { + noteableData: this.getNoteableData, + noteableType: this.noteableType, + diffFile: this.diffFile, + positionType: IMAGE_DIFF_POSITION_TYPE, + x: this.diffFileCommentForm.x, + y: this.diffFileCommentForm.y, + }, + }); + }, }, }; @@ -56,7 +86,37 @@ export default { :new-sha="diffFile.diffRefs.headSha" :old-path="diffFile.oldPath" :old-sha="diffFile.diffRefs.baseSha" - :project-path="projectPath"/> + :file-hash="diffFile.fileHash" + :project-path="projectPath" + > + +
+ + +
+
diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index cddbe554fbd..e19207bdc95 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -1,24 +1,40 @@ @@ -26,22 +42,54 @@ export default { diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue index 38e881d17a2..cd0c1e850af 100644 --- a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/onion_skin_viewer.vue @@ -15,11 +15,6 @@ export default { type: String, required: true, }, - projectPath: { - type: String, - required: false, - default: '', - }, }, data() { return { @@ -120,7 +115,6 @@ export default { key="onionOldImg" :render-info="false" :path="oldPath" - :project-path="projectPath" @imgLoaded="onionOldImgLoaded" />
@@ -136,9 +130,14 @@ export default { key="onionNewImg" :render-info="false" :path="newPath" - :project-path="projectPath" @imgLoaded="onionNewImgLoaded" - /> + > + + +
diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue index 86366c799a2..c3cfe54eb4d 100644 --- a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff/swipe_viewer.vue @@ -16,11 +16,6 @@ export default { type: String, required: true, }, - projectPath: { - type: String, - required: false, - default: '', - }, }, data() { return { @@ -117,16 +112,14 @@ export default { 'height': swipeMaxPixelHeight, }" class="swipe-frame"> -
- -
+
-
- -
+ + + +
diff --git a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue index 1af85283277..e68a2aa73fa 100644 --- a/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue +++ b/app/assets/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer.vue @@ -8,9 +8,6 @@ import { diffModes, imageViewMode } from '../constants'; export default { components: { ImageViewer, - TwoUpViewer, - SwipeViewer, - OnionSkinViewer, }, props: { diffMode: { @@ -25,17 +22,32 @@ export default { type: String, required: true, }, - projectPath: { - type: String, - required: false, - default: '', - }, }, data() { return { mode: imageViewMode.twoup, }; }, + computed: { + imageViewComponent() { + switch (this.mode) { + case imageViewMode.twoup: + return TwoUpViewer; + case imageViewMode.swipe: + return SwipeViewer; + case imageViewMode.onion: + return OnionSkinViewer; + default: + return undefined; + } + }, + isNew() { + return this.diffMode === diffModes.new; + }, + imagePath() { + return this.isNew ? this.newPath : this.oldPath; + }, + }, methods: { changeMode(newMode) { this.mode = newMode; @@ -52,15 +64,16 @@ export default { v-if="diffMode === $options.diffModes.replaced" class="diff-viewer">
- - - + + + +
    @@ -87,23 +100,27 @@ export default {
-
-
-
-
- + class="diff-viewer" + > +
+ + + + +
diff --git a/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue b/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue index 86c7498a092..ce8efe5b574 100644 --- a/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue +++ b/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue @@ -100,5 +100,6 @@ export default { :title="tooltipText" :tooltip-placement="tooltipPlacement" >{{ username }} + diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 52c91266ff4..19bc4262e21 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -421,21 +421,13 @@ .diff-file-container { .frame.deleted { - border: 0; + border: 1px solid $deleted; background-color: inherit; - - .image_file img { - border: 1px solid $deleted; - } } .frame.added { - border: 0; + border: 1px solid $added; background-color: inherit; - - .image_file img { - border: 1px solid $added; - } } .swipe.view, @@ -481,6 +473,11 @@ bottom: -25px; } } + + .discussion-notes .discussion-notes { + margin-left: 0; + border-left: 0; + } } .file-content .diff-file { @@ -804,7 +801,7 @@ // double jagged line divider .discussion-notes + .discussion-notes::before, - .discussion-notes + .discussion-form::before { + .diff-file-discussions + .discussion-form::before { content: ''; position: relative; display: block; @@ -844,6 +841,13 @@ background-repeat: repeat; } + .diff-file-discussions + .discussion-form::before { + width: auto; + margin-left: -16px; + margin-right: -16px; + margin-bottom: 16px; + } + .notes { position: relative; } @@ -870,11 +874,13 @@ } } -.files:not([data-can-create-note]) .frame { +.files:not([data-can-create-note="true"]) .frame { cursor: auto; } -.frame.click-to-comment { +.frame, +.frame.click-to-comment, +.btn-transparent.image-diff-overlay-add-comment { position: relative; cursor: image-url('illustrations/image_comment_light_cursor.svg') $image-comment-cursor-left-offset $image-comment-cursor-top-offset, @@ -910,6 +916,7 @@ .frame .badge.badge-pill, .image-diff-avatar-link .badge.badge-pill, +.user-avatar-link .badge.badge-pill, .notes > .badge.badge-pill { position: absolute; background-color: $blue-400; @@ -944,7 +951,8 @@ } } -.image-diff-avatar-link { +.image-diff-avatar-link, +.user-avatar-link { position: relative; .badge.badge-pill, @@ -1073,3 +1081,14 @@ top: 0; } } + +.image-diff-overlay, +.image-diff-overlay-add-comment { + top: 0; + left: 0; + + &:active, + &:focus { + outline: 0; + } +} diff --git a/changelogs/unreleased/mr-image-commenting.yml b/changelogs/unreleased/mr-image-commenting.yml new file mode 100644 index 00000000000..3cc3becc795 --- /dev/null +++ b/changelogs/unreleased/mr-image-commenting.yml @@ -0,0 +1,5 @@ +--- +title: Reimplemented image commenting in merge request diffs +merge_request: +author: +type: added diff --git a/lib/gitlab/diff/formatters/image_formatter.rb b/lib/gitlab/diff/formatters/image_formatter.rb index ccd0d309972..219a4b9b348 100644 --- a/lib/gitlab/diff/formatters/image_formatter.rb +++ b/lib/gitlab/diff/formatters/image_formatter.rb @@ -21,7 +21,7 @@ module Gitlab end def complete? - x && y && width && height + x && y end def to_h diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 324e5315821..65e27d1611b 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -336,6 +336,9 @@ msgstr "" msgid "Add a table" msgstr "" +msgid "Add image comment" +msgstr "" + msgid "Add license" msgstr "" @@ -1721,12 +1724,18 @@ msgstr "" msgid "Collapse sidebar" msgstr "" +msgid "Comment" +msgstr "" + msgid "Comment & resolve discussion" msgstr "" msgid "Comment & unresolve discussion" msgstr "" +msgid "Comment form position" +msgstr "" + msgid "Comments" msgstr "" diff --git a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb index f0d38dc6a0c..d790bdc82ce 100644 --- a/spec/features/merge_request/user_creates_image_diff_notes_spec.rb +++ b/spec/features/merge_request/user_creates_image_diff_notes_spec.rb @@ -114,10 +114,9 @@ describe 'Merge request > User creates image diff notes', :js do create_image_diff_note end - # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 - xit 'shows indicator and avatar badges, and allows collapsing/expanding the discussion notes' do + it 'shows indicator and avatar badges, and allows collapsing/expanding the discussion notes' do indicator = find('.js-image-badge', match: :first) - badge = find('.image-diff-avatar-link .badge', match: :first) + badge = find('.user-avatar-link .badge', match: :first) expect(indicator).to have_content('1') expect(badge).to have_content('1') @@ -157,8 +156,7 @@ describe 'Merge request > User creates image diff notes', :js do visit project_merge_request_path(project, merge_request) end - # TODO: https://gitlab.com/gitlab-org/gitlab-ce/issues/48034 - xit 'render diff indicators within the image frame' do + 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 @@ -200,7 +198,6 @@ describe 'Merge request > User creates image diff notes', :js do def create_image_diff_note find('.js-add-image-diff-note-button', match: :first).click - page.all('.js-add-image-diff-note-button')[0].click find('.diff-content .note-textarea').native.send_keys('image diff test comment') click_button 'Comment' wait_for_requests diff --git a/spec/javascripts/diffs/components/diff_content_spec.js b/spec/javascripts/diffs/components/diff_content_spec.js index 67f7b569f47..a31e04d426b 100644 --- a/spec/javascripts/diffs/components/diff_content_spec.js +++ b/spec/javascripts/diffs/components/diff_content_spec.js @@ -1,15 +1,24 @@ import Vue from 'vue'; import DiffContentComponent from '~/diffs/components/diff_content.vue'; -import store from '~/mr_notes/stores'; +import { createStore } from '~/mr_notes/stores'; import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import { GREEN_BOX_IMAGE_URL, RED_BOX_IMAGE_URL } from 'spec/test_constants'; +import '~/behaviors/markdown/render_gfm'; import diffFileMockData from '../mock_data/diff_file'; +import discussionsMockData from '../mock_data/diff_discussions'; describe('DiffContent', () => { const Component = Vue.extend(DiffContentComponent); let vm; beforeEach(() => { + const store = createStore(); + store.state.notes.noteableData = { + current_user: { + can_create_note: false, + }, + }; + vm = mountComponentWithStore(Component, { store, props: { @@ -46,21 +55,49 @@ describe('DiffContent', () => { }); describe('image diff', () => { - beforeEach(() => { + beforeEach(done => { vm.diffFile.newPath = GREEN_BOX_IMAGE_URL; vm.diffFile.newSha = 'DEF'; vm.diffFile.oldPath = RED_BOX_IMAGE_URL; vm.diffFile.oldSha = 'ABC'; vm.diffFile.viewPath = ''; + vm.diffFile.discussions = [{ ...discussionsMockData }]; + vm.$store.state.diffs.commentForms.push({ fileHash: vm.diffFile.fileHash, x: 10, y: 20 }); + + vm.$nextTick(done); }); - it('should have image diff view in place', done => { - vm.$nextTick(() => { - expect(vm.$el.querySelectorAll('.js-diff-inline-view').length).toEqual(0); + it('should have image diff view in place', () => { + expect(vm.$el.querySelectorAll('.js-diff-inline-view').length).toEqual(0); + + expect(vm.$el.querySelectorAll('.diff-viewer .image').length).toEqual(1); + }); - expect(vm.$el.querySelectorAll('.diff-viewer .image').length).toEqual(1); + it('renders image diff overlay', () => { + expect(vm.$el.querySelector('.image-diff-overlay')).not.toBe(null); + }); - done(); + it('renders diff file discussions', () => { + expect(vm.$el.querySelectorAll('.discussion .note.timeline-entry').length).toEqual(5); + }); + + describe('handleSaveNote', () => { + it('dispatches handleSaveNote', () => { + spyOn(vm.$store, 'dispatch').and.stub(); + + vm.handleSaveNote('test'); + + expect(vm.$store.dispatch).toHaveBeenCalledWith('diffs/saveDiffDiscussion', { + note: 'test', + formData: { + noteableData: jasmine.anything(), + noteableType: jasmine.anything(), + diffFile: vm.diffFile, + positionType: 'image', + x: 10, + y: 20, + }, + }); }); }); }); diff --git a/spec/javascripts/diffs/components/diff_discussions_spec.js b/spec/javascripts/diffs/components/diff_discussions_spec.js index 270f363825f..0bc9da5ad0f 100644 --- a/spec/javascripts/diffs/components/diff_discussions_spec.js +++ b/spec/javascripts/diffs/components/diff_discussions_spec.js @@ -1,24 +1,90 @@ import Vue from 'vue'; import DiffDiscussions from '~/diffs/components/diff_discussions.vue'; -import store from '~/mr_notes/stores'; +import { createStore } from '~/mr_notes/stores'; import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +import '~/behaviors/markdown/render_gfm'; import discussionsMockData from '../mock_data/diff_discussions'; describe('DiffDiscussions', () => { - let component; + let vm; const getDiscussionsMockData = () => [Object.assign({}, discussionsMockData)]; - beforeEach(() => { - component = createComponentWithStore(Vue.extend(DiffDiscussions), store, { + function createComponent(props = {}) { + const store = createStore(); + + vm = createComponentWithStore(Vue.extend(DiffDiscussions), store, { discussions: getDiscussionsMockData(), + ...props, }).$mount(); + } + + afterEach(() => { + vm.$destroy(); }); describe('template', () => { it('should have notes list', () => { - const { $el } = component; + createComponent(); + + expect(vm.$el.querySelectorAll('.discussion .note.timeline-entry').length).toEqual(5); + }); + }); + + describe('image commenting', () => { + it('renders collapsible discussion button', () => { + createComponent({ shouldCollapseDiscussions: true }); + + expect(vm.$el.querySelector('.js-diff-notes-toggle')).not.toBe(null); + expect(vm.$el.querySelector('.js-diff-notes-toggle svg')).not.toBe(null); + expect(vm.$el.querySelector('.js-diff-notes-toggle').classList).toContain( + 'diff-notes-collapse', + ); + }); + + it('dispatches toggleDiscussion when clicking collapse button', () => { + createComponent({ shouldCollapseDiscussions: true }); + + spyOn(vm.$store, 'dispatch').and.stub(); + + vm.$el.querySelector('.js-diff-notes-toggle').click(); + + expect(vm.$store.dispatch).toHaveBeenCalledWith('toggleDiscussion', { + discussionId: vm.discussions[0].id, + }); + }); + + it('renders expand button when discussion is collapsed', done => { + createComponent({ shouldCollapseDiscussions: true }); + + vm.discussions[0].expanded = false; + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.js-diff-notes-toggle').textContent.trim()).toBe('1'); + expect(vm.$el.querySelector('.js-diff-notes-toggle').className).toContain( + 'btn-transparent badge badge-pill', + ); + + done(); + }); + }); + + it('hides discussion when collapsed', done => { + createComponent({ shouldCollapseDiscussions: true }); + + vm.discussions[0].expanded = false; + + vm.$nextTick(() => { + expect(vm.$el.querySelector('.note-discussion').style.display).toBe('none'); + + done(); + }); + }); + + it('renders badge on avatar', () => { + createComponent({ renderAvatarBadge: true, discussions: [{ ...discussionsMockData }] }); - expect($el.querySelectorAll('.discussion .note.timeline-entry').length).toEqual(5); + expect(vm.$el.querySelector('.user-avatar-link .badge-pill')).not.toBe(null); + expect(vm.$el.querySelector('.user-avatar-link .badge-pill').textContent.trim()).toBe('1'); }); }); }); diff --git a/spec/javascripts/diffs/components/image_diff_overlay_spec.js b/spec/javascripts/diffs/components/image_diff_overlay_spec.js new file mode 100644 index 00000000000..4ab5ebd6c01 --- /dev/null +++ b/spec/javascripts/diffs/components/image_diff_overlay_spec.js @@ -0,0 +1,121 @@ +import Vue from 'vue'; +import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue'; +import { createStore } from '~/mr_notes/stores'; +import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +import { imageDiffDiscussions } from '../mock_data/diff_discussions'; + +describe('Diffs image diff overlay component', () => { + let Component; + let vm; + + function createComponent(props = {}, extendStore = () => {}) { + const store = createStore(); + + extendStore(store); + + vm = mountComponentWithStore(Component, { + store, + props: { discussions: [...imageDiffDiscussions], fileHash: 'ABC', ...props }, + }); + } + + beforeAll(() => { + Component = Vue.extend(ImageDiffOverlay); + }); + + afterEach(() => { + vm.$destroy(); + }); + + it('renders comment badges', () => { + createComponent(); + + expect(vm.$el.querySelectorAll('.js-image-badge').length).toBe(2); + }); + + it('renders index of discussion in badge', () => { + createComponent(); + + expect(vm.$el.querySelectorAll('.js-image-badge')[0].textContent.trim()).toBe('1'); + expect(vm.$el.querySelectorAll('.js-image-badge')[1].textContent.trim()).toBe('2'); + }); + + it('renders icon when showCommentIcon is true', () => { + createComponent({ showCommentIcon: true }); + + expect(vm.$el.querySelector('.js-image-badge svg')).not.toBe(null); + }); + + it('sets badge comment positions', () => { + createComponent(); + + expect(vm.$el.querySelectorAll('.js-image-badge')[0].style.left).toBe('10px'); + expect(vm.$el.querySelectorAll('.js-image-badge')[0].style.top).toBe('10px'); + + expect(vm.$el.querySelectorAll('.js-image-badge')[1].style.left).toBe('5px'); + expect(vm.$el.querySelectorAll('.js-image-badge')[1].style.top).toBe('5px'); + }); + + it('renders single badge for discussion object', () => { + createComponent({ + discussions: { + ...imageDiffDiscussions[0], + }, + }); + + expect(vm.$el.querySelectorAll('.js-image-badge').length).toBe(1); + }); + + it('dispatches openDiffFileCommentForm when clcking overlay', () => { + createComponent({ canComment: true }); + + spyOn(vm.$store, 'dispatch').and.stub(); + + vm.$el.querySelector('.js-add-image-diff-note-button').click(); + + expect(vm.$store.dispatch).toHaveBeenCalledWith('diffs/openDiffFileCommentForm', { + fileHash: 'ABC', + x: 0, + y: 0, + }); + }); + + describe('toggle discussion', () => { + it('disables buttons when shouldToggleDiscussion is false', () => { + createComponent({ shouldToggleDiscussion: false }); + + expect(vm.$el.querySelector('.js-image-badge').hasAttribute('disabled')).toBe(true); + }); + + it('dispatches toggleDiscussion when clicking image badge', () => { + createComponent(); + + spyOn(vm.$store, 'dispatch').and.stub(); + + vm.$el.querySelector('.js-image-badge').click(); + + expect(vm.$store.dispatch).toHaveBeenCalledWith('toggleDiscussion', { discussionId: '1' }); + }); + }); + + describe('comment form', () => { + beforeEach(() => { + createComponent({}, store => { + store.state.diffs.commentForms.push({ + fileHash: 'ABC', + x: 20, + y: 10, + }); + }); + }); + + it('renders comment form badge', () => { + expect(vm.$el.querySelector('.comment-indicator')).not.toBe(null); + }); + + it('sets comment form badge position', () => { + expect(vm.$el.querySelector('.comment-indicator').style.left).toBe('20px'); + expect(vm.$el.querySelector('.comment-indicator').style.top).toBe('10px'); + }); + }); +}); diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 0ad214ea4a4..908a27417b6 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -492,3 +492,20 @@ export default { image_diff_html: '
\n
\n
\n
\nCHANGELOG\n
\n

\n22.3 KB\n|\nW:\n\n|\nH:\n\n

\n
\n
\n
\nCHANGELOG\n
\n\n

\n22.3 KB\n|\nW:\n\n|\nH:\n\n

\n
\n
\n
\n
\n
\nCHANGELOG\n
\n
\n
\nCHANGELOG\n
\n\n
\n\n\n\n\n
\n
\n
\n
\n
\nCHANGELOG\n
\n
\nCHANGELOG\n
\n\n
\n
\n
\n
\n
\n
\n
\n
\n
\n
\n
\n\n
\n', }; + +export const imageDiffDiscussions = [ + { + id: '1', + position: { + x: 10, + y: 10, + }, + }, + { + id: '2', + position: { + x: 5, + y: 5, + }, + }, +]; diff --git a/spec/javascripts/diffs/mock_data/diff_file.js b/spec/javascripts/diffs/mock_data/diff_file.js index d7bc0dbe431..be194ab414f 100644 --- a/spec/javascripts/diffs/mock_data/diff_file.js +++ b/spec/javascripts/diffs/mock_data/diff_file.js @@ -237,4 +237,5 @@ export default { }, }, ], + discussions: [], }; diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index bb623953710..17d0f31bdd3 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -218,6 +218,7 @@ describe('DiffsStoreActions', () => { ], }; const singleDiscussion = { + id: '1', fileHash: 'ABC', line_code: 'ABC_1_1', }; @@ -230,6 +231,7 @@ describe('DiffsStoreActions', () => { { type: types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, payload: { + id: '1', fileHash: 'ABC', lineCode: 'ABC_1_1', }, diff --git a/spec/javascripts/notes/components/diff_with_note_spec.js b/spec/javascripts/notes/components/diff_with_note_spec.js index 239d7950907..0c16103714a 100644 --- a/spec/javascripts/notes/components/diff_with_note_spec.js +++ b/spec/javascripts/notes/components/diff_with_note_spec.js @@ -1,7 +1,7 @@ import Vue from 'vue'; import DiffWithNote from '~/notes/components/diff_with_note.vue'; import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; -import createStore from '~/notes/stores'; +import { createStore } from '~/mr_notes/stores'; import { mountComponentWithStore } from 'spec/helpers'; const discussionFixture = 'merge_requests/diff_discussion.json'; diff --git a/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js b/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js index e2c34508b0d..4da8c6196b1 100644 --- a/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js +++ b/spec/javascripts/vue_shared/components/content_viewer/content_viewer_spec.js @@ -47,7 +47,7 @@ describe('ContentViewer', () => { }); setTimeout(() => { - expect(vm.$el.querySelector('.image_file img').getAttribute('src')).toBe(GREEN_BOX_IMAGE_URL); + expect(vm.$el.querySelector('img').getAttribute('src')).toBe(GREEN_BOX_IMAGE_URL); done(); }); diff --git a/spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js b/spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js index fcd231ec693..67a3a2e08bc 100644 --- a/spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js +++ b/spec/javascripts/vue_shared/components/diff_viewer/diff_viewer_spec.js @@ -30,11 +30,11 @@ describe('DiffViewer', () => { }); setTimeout(() => { - expect(vm.$el.querySelector('.deleted .image_file img').getAttribute('src')).toBe( + expect(vm.$el.querySelector('.deleted img').getAttribute('src')).toBe( `//raw/DEF/${RED_BOX_IMAGE_URL}`, ); - expect(vm.$el.querySelector('.added .image_file img').getAttribute('src')).toBe( + expect(vm.$el.querySelector('.added img').getAttribute('src')).toBe( `//raw/ABC/${GREEN_BOX_IMAGE_URL}`, ); diff --git a/spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js b/spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js index 380effdb669..2d3e178d249 100644 --- a/spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js +++ b/spec/javascripts/vue_shared/components/diff_viewer/viewers/image_diff_viewer_spec.js @@ -52,13 +52,9 @@ describe('ImageDiffViewer', () => { }); setTimeout(() => { - expect(vm.$el.querySelector('.added .image_file img').getAttribute('src')).toBe( - GREEN_BOX_IMAGE_URL, - ); + expect(vm.$el.querySelector('.added img').getAttribute('src')).toBe(GREEN_BOX_IMAGE_URL); - expect(vm.$el.querySelector('.deleted .image_file img').getAttribute('src')).toBe( - RED_BOX_IMAGE_URL, - ); + expect(vm.$el.querySelector('.deleted img').getAttribute('src')).toBe(RED_BOX_IMAGE_URL); expect(vm.$el.querySelector('.view-modes-menu li.active').textContent.trim()).toBe('2-up'); expect(vm.$el.querySelector('.view-modes-menu li:nth-child(2)').textContent.trim()).toBe( @@ -81,9 +77,7 @@ describe('ImageDiffViewer', () => { }); setTimeout(() => { - expect(vm.$el.querySelector('.added .image_file img').getAttribute('src')).toBe( - GREEN_BOX_IMAGE_URL, - ); + expect(vm.$el.querySelector('.added img').getAttribute('src')).toBe(GREEN_BOX_IMAGE_URL); done(); }); @@ -97,9 +91,7 @@ describe('ImageDiffViewer', () => { }); setTimeout(() => { - expect(vm.$el.querySelector('.deleted .image_file img').getAttribute('src')).toBe( - RED_BOX_IMAGE_URL, - ); + expect(vm.$el.querySelector('.deleted img').getAttribute('src')).toBe(RED_BOX_IMAGE_URL); done(); }); -- cgit v1.2.1 From 35faecb06b4b921d12a62344976918e88cd73fca Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 6 Nov 2018 09:40:03 +0000 Subject: Restored width & height properties --- .../javascripts/diffs/components/diff_content.vue | 2 ++ .../diffs/components/image_diff_overlay.vue | 28 +++++++++++++++-- app/assets/javascripts/diffs/store/utils.js | 2 ++ lib/gitlab/diff/formatters/image_formatter.rb | 2 +- .../diffs/components/diff_content_spec.js | 10 ++++++- .../diffs/components/image_diff_overlay_spec.js | 35 ++++++++++++++++++---- .../diffs/mock_data/diff_discussions.js | 4 +++ 7 files changed, 74 insertions(+), 9 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_content.vue b/app/assets/javascripts/diffs/components/diff_content.vue index f677999a268..547742a5ff4 100644 --- a/app/assets/javascripts/diffs/components/diff_content.vue +++ b/app/assets/javascripts/diffs/components/diff_content.vue @@ -57,6 +57,8 @@ export default { positionType: IMAGE_DIFF_POSITION_TYPE, x: this.diffFileCommentForm.x, y: this.diffFileCommentForm.y, + width: this.diffFileCommentForm.width, + height: this.diffFileCommentForm.height, }, }); }, diff --git a/app/assets/javascripts/diffs/components/image_diff_overlay.vue b/app/assets/javascripts/diffs/components/image_diff_overlay.vue index 4b99f38d293..ae1b0a52901 100644 --- a/app/assets/javascripts/diffs/components/image_diff_overlay.vue +++ b/app/assets/javascripts/diffs/components/image_diff_overlay.vue @@ -50,15 +50,39 @@ export default { methods: { ...mapActions(['toggleDiscussion']), ...mapActions('diffs', ['openDiffFileCommentForm']), + getImageDimensions() { + return { + width: this.$parent.width, + height: this.$parent.height, + }; + }, + getPositionForObject(meta) { + const { x, y, width, height } = meta; + const imageWidth = this.getImageDimensions().width; + const imageHeight = this.getImageDimensions().height; + const widthRatio = imageWidth / width; + const heightRatio = imageHeight / height; + + return { + x: Math.round(x * widthRatio), + y: Math.round(y * heightRatio), + }; + }, getPosition(discussion) { + const { x, y } = this.getPositionForObject(discussion.position); + return { - left: `${discussion.position.x}px`, - top: `${discussion.position.y}px`, + left: `${x}px`, + top: `${y}px`, }; }, clickedImage(x, y) { + const { width, height } = this.getImageDimensions(); + this.openDiffFileCommentForm({ fileHash: this.fileHash, + width, + height, x, y, }); diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 178745cffc2..a935b9b1ffa 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -49,6 +49,8 @@ export function getFormData(params) { new_line: noteTargetLine ? noteTargetLine.newLine : null, x: params.x, y: params.y, + width: params.width, + height: params.height, }); const postData = { diff --git a/lib/gitlab/diff/formatters/image_formatter.rb b/lib/gitlab/diff/formatters/image_formatter.rb index 219a4b9b348..ccd0d309972 100644 --- a/lib/gitlab/diff/formatters/image_formatter.rb +++ b/lib/gitlab/diff/formatters/image_formatter.rb @@ -21,7 +21,7 @@ module Gitlab end def complete? - x && y + x && y && width && height end def to_h diff --git a/spec/javascripts/diffs/components/diff_content_spec.js b/spec/javascripts/diffs/components/diff_content_spec.js index a31e04d426b..36bd042f3c4 100644 --- a/spec/javascripts/diffs/components/diff_content_spec.js +++ b/spec/javascripts/diffs/components/diff_content_spec.js @@ -62,7 +62,13 @@ describe('DiffContent', () => { vm.diffFile.oldSha = 'ABC'; vm.diffFile.viewPath = ''; vm.diffFile.discussions = [{ ...discussionsMockData }]; - vm.$store.state.diffs.commentForms.push({ fileHash: vm.diffFile.fileHash, x: 10, y: 20 }); + vm.$store.state.diffs.commentForms.push({ + fileHash: vm.diffFile.fileHash, + x: 10, + y: 20, + width: 100, + height: 200, + }); vm.$nextTick(done); }); @@ -96,6 +102,8 @@ describe('DiffContent', () => { positionType: 'image', x: 10, y: 20, + width: 100, + height: 200, }, }); }); diff --git a/spec/javascripts/diffs/components/image_diff_overlay_spec.js b/spec/javascripts/diffs/components/image_diff_overlay_spec.js index 4ab5ebd6c01..d76ab745fe1 100644 --- a/spec/javascripts/diffs/components/image_diff_overlay_spec.js +++ b/spec/javascripts/diffs/components/image_diff_overlay_spec.js @@ -1,10 +1,14 @@ import Vue from 'vue'; import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue'; import { createStore } from '~/mr_notes/stores'; -import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; +import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import { imageDiffDiscussions } from '../mock_data/diff_discussions'; describe('Diffs image diff overlay component', () => { + const dimensions = { + width: 100, + height: 200, + }; let Component; let vm; @@ -13,9 +17,10 @@ describe('Diffs image diff overlay component', () => { extendStore(store); - vm = mountComponentWithStore(Component, { - store, - props: { discussions: [...imageDiffDiscussions], fileHash: 'ABC', ...props }, + vm = createComponentWithStore(Component, store, { + discussions: [...imageDiffDiscussions], + fileHash: 'ABC', + ...props, }); } @@ -29,12 +34,16 @@ describe('Diffs image diff overlay component', () => { it('renders comment badges', () => { createComponent(); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); expect(vm.$el.querySelectorAll('.js-image-badge').length).toBe(2); }); it('renders index of discussion in badge', () => { createComponent(); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); expect(vm.$el.querySelectorAll('.js-image-badge')[0].textContent.trim()).toBe('1'); expect(vm.$el.querySelectorAll('.js-image-badge')[1].textContent.trim()).toBe('2'); @@ -42,12 +51,16 @@ describe('Diffs image diff overlay component', () => { it('renders icon when showCommentIcon is true', () => { createComponent({ showCommentIcon: true }); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); expect(vm.$el.querySelector('.js-image-badge svg')).not.toBe(null); }); it('sets badge comment positions', () => { createComponent(); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); expect(vm.$el.querySelectorAll('.js-image-badge')[0].style.left).toBe('10px'); expect(vm.$el.querySelectorAll('.js-image-badge')[0].style.top).toBe('10px'); @@ -62,12 +75,16 @@ describe('Diffs image diff overlay component', () => { ...imageDiffDiscussions[0], }, }); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); expect(vm.$el.querySelectorAll('.js-image-badge').length).toBe(1); }); - it('dispatches openDiffFileCommentForm when clcking overlay', () => { + it('dispatches openDiffFileCommentForm when clicking overlay', () => { createComponent({ canComment: true }); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); spyOn(vm.$store, 'dispatch').and.stub(); @@ -77,18 +94,24 @@ describe('Diffs image diff overlay component', () => { fileHash: 'ABC', x: 0, y: 0, + width: 100, + height: 200, }); }); describe('toggle discussion', () => { it('disables buttons when shouldToggleDiscussion is false', () => { createComponent({ shouldToggleDiscussion: false }); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); expect(vm.$el.querySelector('.js-image-badge').hasAttribute('disabled')).toBe(true); }); it('dispatches toggleDiscussion when clicking image badge', () => { createComponent(); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); spyOn(vm.$store, 'dispatch').and.stub(); @@ -107,6 +130,8 @@ describe('Diffs image diff overlay component', () => { y: 10, }); }); + spyOn(vm, 'getImageDimensions').and.returnValue(dimensions); + vm.$mount(); }); it('renders comment form badge', () => { diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 908a27417b6..5ffe5a366ba 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -499,6 +499,8 @@ export const imageDiffDiscussions = [ position: { x: 10, y: 10, + width: 100, + height: 200, }, }, { @@ -506,6 +508,8 @@ export const imageDiffDiscussions = [ position: { x: 5, y: 5, + width: 100, + height: 200, }, }, ]; -- cgit v1.2.1 From 0485c04764638b4ec198097ac3efd69d6b6d3aad Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 7 Nov 2018 08:25:02 +0000 Subject: Fixed user avatar link render empty space --- .../javascripts/vue_shared/components/user_avatar/user_avatar_link.vue | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue b/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue index ce8efe5b574..dd6f96e2609 100644 --- a/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue +++ b/app/assets/javascripts/vue_shared/components/user_avatar/user_avatar_link.vue @@ -99,7 +99,6 @@ export default { v-tooltip :title="tooltipText" :tooltip-placement="tooltipPlacement" - >{{ username }} - + >{{ username }} -- cgit v1.2.1