diff options
author | Tim Zallmann <tzallmann@gitlab.com> | 2018-07-18 18:12:31 +0300 |
---|---|---|
committer | Tim Zallmann <tzallmann@gitlab.com> | 2018-07-18 18:12:31 +0300 |
commit | 5ba542b1094bd8fd95d48d2ac834fd8605cf8fee (patch) | |
tree | 4800ef87c593189476995f89b98b19f645e92a0f | |
parent | 32c831ea2b8425c71c790b67fb8f2170f3d9955a (diff) | |
parent | 9e24e93e7759bab0868d7ad212e5018ae0f37259 (diff) |
Merge branch '_acet-fix-mr-autosave' into 'master'
Fix autosave issues for MR discussions
Closes #48876 and #48877
See merge request gitlab-org/gitlab-ce!20569
-rw-r--r-- | app/assets/javascripts/autosave.js | 4 | ||||
-rw-r--r-- | app/assets/javascripts/diffs/components/diff_line_note_form.vue | 31 | ||||
-rw-r--r-- | app/assets/javascripts/notes/components/note_form.vue | 2 | ||||
-rw-r--r-- | app/assets/javascripts/notes/components/noteable_discussion.vue | 34 | ||||
-rw-r--r-- | app/assets/javascripts/notes/mixins/autosave.js | 17 | ||||
-rw-r--r-- | changelogs/unreleased/_acet-fix-mr-autosave.yml | 5 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/javascripts/autosave_spec.js | 10 | ||||
-rw-r--r-- | spec/javascripts/diffs/components/diff_line_note_form_spec.js | 41 | ||||
-rw-r--r-- | spec/javascripts/notes/components/noteable_discussion_spec.js | 9 |
10 files changed, 106 insertions, 50 deletions
diff --git a/app/assets/javascripts/autosave.js b/app/assets/javascripts/autosave.js index fa00a3cf386..e8c59fab609 100644 --- a/app/assets/javascripts/autosave.js +++ b/app/assets/javascripts/autosave.js @@ -53,4 +53,8 @@ export default class Autosave { return window.localStorage.removeItem(this.key); } + + dispose() { + this.field.off('input'); + } } diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue index 32f9516d332..cbe4551d06b 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -1,17 +1,17 @@ <script> -import $ from 'jquery'; import { mapState, mapGetters, mapActions } from 'vuex'; import createFlash from '~/flash'; import { s__ } from '~/locale'; import noteForm from '../../notes/components/note_form.vue'; import { getNoteFormData } from '../store/utils'; -import Autosave from '../../autosave'; -import { DIFF_NOTE_TYPE, NOTE_TYPE } from '../constants'; +import autosave from '../../notes/mixins/autosave'; +import { DIFF_NOTE_TYPE } from '../constants'; export default { components: { noteForm, }, + mixins: [autosave], props: { diffFileHash: { type: String, @@ -41,28 +41,35 @@ export default { }, mounted() { if (this.isLoggedIn) { - const noteableData = this.getNoteableData; const keys = [ - NOTE_TYPE, - this.noteableType, - noteableData.id, - noteableData.diff_head_sha, + this.noteableData.diff_head_sha, DIFF_NOTE_TYPE, - noteableData.source_project_id, + this.noteableData.source_project_id, this.line.lineCode, ]; - this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); + this.initAutoSave(this.noteableData, keys); } }, methods: { ...mapActions('diffs', ['cancelCommentForm']), ...mapActions(['saveNote', 'refetchDiscussionById']), - handleCancelCommentForm() { - this.autosave.reset(); + handleCancelCommentForm(shouldConfirm, isDirty) { + if (shouldConfirm && isDirty) { + const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); + + // eslint-disable-next-line no-alert + if (!window.confirm(msg)) { + return; + } + } + this.cancelCommentForm({ lineCode: this.line.lineCode, }); + this.$nextTick(() => { + this.resetAutoSave(); + }); }, handleSaveNote(note) { const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash); diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index 26482a02e00..abcd4422d7c 100644 --- a/app/assets/javascripts/notes/components/note_form.vue +++ b/app/assets/javascripts/notes/components/note_form.vue @@ -7,7 +7,7 @@ import issuableStateMixin from '../mixins/issuable_state'; import resolvable from '../mixins/resolvable'; export default { - name: 'IssueNoteForm', + name: 'NoteForm', components: { issueWarning, markdownField, diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index bee635398b3..2f1a68731c7 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -6,6 +6,7 @@ import nextDiscussionsSvg from 'icons/_next_discussion.svg'; import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils'; import { truncateSha } from '~/lib/utils/text_utility'; import systemNote from '~/vue_shared/components/notes/system_note.vue'; +import { s__ } from '~/locale'; import Flash from '../../flash'; import { SYSTEM_NOTE } from '../constants'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; @@ -144,19 +145,17 @@ export default { return this.isDiffDiscussion ? '' : 'card discussion-wrapper'; }, }, - mounted() { - if (this.isReplying) { - this.initAutoSave(this.transformedDiscussion); - } - }, - updated() { - if (this.isReplying) { - if (!this.autosave) { - this.initAutoSave(this.transformedDiscussion); + watch: { + isReplying() { + if (this.isReplying) { + this.$nextTick(() => { + // Pass an extra key to separate reply and note edit forms + this.initAutoSave(this.transformedDiscussion, ['Reply']); + }); } else { - this.setAutoSave(); + this.disposeAutoSave(); } - } + }, }, created() { this.resolveDiscussionsSvg = resolveDiscussionsSvg; @@ -194,16 +193,18 @@ export default { showReplyForm() { this.isReplying = true; }, - cancelReplyForm(shouldConfirm) { - if (shouldConfirm && this.$refs.noteForm.isDirty) { + cancelReplyForm(shouldConfirm, isDirty) { + if (shouldConfirm && isDirty) { + const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); + // eslint-disable-next-line no-alert - if (!window.confirm('Are you sure you want to cancel creating this comment?')) { + if (!window.confirm(msg)) { return; } } - this.resetAutoSave(); this.isReplying = false; + this.resetAutoSave(); }, saveReply(noteText, form, callback) { const postData = { @@ -420,7 +421,8 @@ Please check your network connection and try again.`; :is-editing="false" save-button-title="Comment" @handleFormUpdate="saveReply" - @cancelForm="cancelReplyForm" /> + @cancelForm="cancelReplyForm" + /> <note-signed-out-widget v-if="!canReply" /> </div> </div> diff --git a/app/assets/javascripts/notes/mixins/autosave.js b/app/assets/javascripts/notes/mixins/autosave.js index 36cc8d5d056..4f45f912479 100644 --- a/app/assets/javascripts/notes/mixins/autosave.js +++ b/app/assets/javascripts/notes/mixins/autosave.js @@ -4,12 +4,18 @@ import { capitalizeFirstCharacter } from '../../lib/utils/text_utility'; export default { methods: { - initAutoSave(noteable) { - this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), [ + initAutoSave(noteable, extraKeys = []) { + let keys = [ 'Note', - capitalizeFirstCharacter(noteable.noteable_type), + capitalizeFirstCharacter(noteable.noteable_type || noteable.noteableType), noteable.id, - ]); + ]; + + if (extraKeys) { + keys = keys.concat(extraKeys); + } + + this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); }, resetAutoSave() { this.autosave.reset(); @@ -17,5 +23,8 @@ export default { setAutoSave() { this.autosave.save(); }, + disposeAutoSave() { + this.autosave.dispose(); + }, }, }; diff --git a/changelogs/unreleased/_acet-fix-mr-autosave.yml b/changelogs/unreleased/_acet-fix-mr-autosave.yml new file mode 100644 index 00000000000..f87b32f68e2 --- /dev/null +++ b/changelogs/unreleased/_acet-fix-mr-autosave.yml @@ -0,0 +1,5 @@ +--- +title: Fix autosave and ESC confirmation issues for MR discussions +merge_request: 20569 +author: +type: fixed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 562760552e0..8ba05827682 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3523,6 +3523,9 @@ msgstr "" msgid "Note: Consider asking your GitLab administrator to configure %{github_integration_link}, which will allow login via GitHub and allow importing repositories without generating a Personal Access Token." msgstr "" +msgid "Notes|Are you sure you want to cancel creating this comment?" +msgstr "" + msgid "Notification events" msgstr "" diff --git a/spec/javascripts/autosave_spec.js b/spec/javascripts/autosave_spec.js index 38ae5b7e00c..dcb1c781591 100644 --- a/spec/javascripts/autosave_spec.js +++ b/spec/javascripts/autosave_spec.js @@ -59,12 +59,10 @@ describe('Autosave', () => { Autosave.prototype.restore.call(autosave); - expect( - field.trigger, - ).toHaveBeenCalled(); + expect(field.trigger).toHaveBeenCalled(); }); - it('triggers native event', (done) => { + it('triggers native event', done => { autosave.field.get(0).addEventListener('change', () => { done(); }); @@ -81,9 +79,7 @@ describe('Autosave', () => { it('does not trigger event', () => { spyOn(field, 'trigger').and.callThrough(); - expect( - field.trigger, - ).not.toHaveBeenCalled(); + expect(field.trigger).not.toHaveBeenCalled(); }); }); }); diff --git a/spec/javascripts/diffs/components/diff_line_note_form_spec.js b/spec/javascripts/diffs/components/diff_line_note_form_spec.js index 4600aaea70b..6fe5fdaf7f9 100644 --- a/spec/javascripts/diffs/components/diff_line_note_form_spec.js +++ b/spec/javascripts/diffs/components/diff_line_note_form_spec.js @@ -3,6 +3,7 @@ import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue'; import store from '~/mr_notes/stores'; import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import diffFileMockData from '../mock_data/diff_file'; +import { noteableDataMock } from '../../notes/mock_data'; describe('DiffLineNoteForm', () => { let component; @@ -21,10 +22,9 @@ describe('DiffLineNoteForm', () => { noteTargetLine: diffLines[0], }); - Object.defineProperty(component, 'isLoggedIn', { - get() { - return true; - }, + Object.defineProperties(component, { + noteableData: { value: noteableDataMock }, + isLoggedIn: { value: true }, }); component.$mount(); @@ -32,12 +32,37 @@ describe('DiffLineNoteForm', () => { describe('methods', () => { describe('handleCancelCommentForm', () => { - it('should call cancelCommentForm with lineCode', () => { + it('should ask for confirmation when shouldConfirm and isDirty passed as truthy', () => { + spyOn(window, 'confirm').and.returnValue(false); + + component.handleCancelCommentForm(true, true); + expect(window.confirm).toHaveBeenCalled(); + }); + + it('should ask for confirmation when one of the params false', () => { + spyOn(window, 'confirm').and.returnValue(false); + + component.handleCancelCommentForm(true, false); + expect(window.confirm).not.toHaveBeenCalled(); + + component.handleCancelCommentForm(false, true); + expect(window.confirm).not.toHaveBeenCalled(); + }); + + it('should call cancelCommentForm with lineCode', done => { + spyOn(window, 'confirm'); spyOn(component, 'cancelCommentForm'); + spyOn(component, 'resetAutoSave'); component.handleCancelCommentForm(); - expect(component.cancelCommentForm).toHaveBeenCalledWith({ - lineCode: diffLines[0].lineCode, + expect(window.confirm).not.toHaveBeenCalled(); + component.$nextTick(() => { + expect(component.cancelCommentForm).toHaveBeenCalledWith({ + lineCode: diffLines[0].lineCode, + }); + expect(component.resetAutoSave).toHaveBeenCalled(); + + done(); }); }); }); @@ -66,7 +91,7 @@ describe('DiffLineNoteForm', () => { describe('mounted', () => { it('should init autosave', () => { - const key = 'autosave/Note/issue///DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1'; + const key = 'autosave/Note/Issue/98//DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1'; expect(component.autosave).toBeDefined(); expect(component.autosave.key).toEqual(key); diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js index 7da931fd9cb..f3f50aed232 100644 --- a/spec/javascripts/notes/components/noteable_discussion_spec.js +++ b/spec/javascripts/notes/components/noteable_discussion_spec.js @@ -46,10 +46,15 @@ describe('noteable_discussion component', () => { it('should toggle reply form', done => { vm.$el.querySelector('.js-vue-discussion-reply').click(); + Vue.nextTick(() => { - expect(vm.$refs.noteForm).not.toBeNull(); expect(vm.isReplying).toEqual(true); - done(); + + // There is a watcher for `isReplying` which will init autosave in the next tick + Vue.nextTick(() => { + expect(vm.$refs.noteForm).not.toBeNull(); + done(); + }); }); }); |