Merge branch 'draft-design-improvements' into 'master'

Improve draft note header design

Closes #7920 and #7905

See merge request gitlab-org/gitlab-ee!8503

app/assets/javascripts/notes/components/note_actions.vue

@@ -2,6 +2,7 @@
 import { mapGetters } from 'vuex';
 import Icon from '~/vue_shared/components/icon.vue';
 import { GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui';
+import resolvedStatusMixin from 'ee/batch_comments/mixins/resolved_status';
 
 export default {
   name: 'NoteActions',
@@ -12,6 +13,7 @@ export default {
   directives: {
     GlTooltip: GlTooltipDirective,
   },
+  mixins: [resolvedStatusMixin],
   props: {
     authorId: {
       type: Number,
@@ -93,6 +95,8 @@ export default {
       return this.getUserDataByProp('id');
     },
     resolveButtonTitle() {
+      if (this.discussionId) return this.resolvedStatusMessage;
+
       let title = 'Mark as resolved';
 
       if (this.resolvedBy) {
@@ -113,6 +117,7 @@ export default {
       this.$emit('handleResolve');
     },
   },
+  showStaysResolved: true,
 };
 </script>
 

app/assets/javascripts/notes/components/note_body.vue

 <script>
+import { mapGetters } from 'vuex';
 import $ from 'jquery';
 import noteEditedText from './note_edited_text.vue';
 import noteAwardsList from './note_awards_list.vue';
@@ -30,9 +31,15 @@ export default {
     },
   },
   computed: {
+    ...mapGetters(['getDiscussion']),
     noteBody() {
       return this.note.note;
     },
+    discussion() {
+      if (!this.note.isDraft) return {};
+
+      return this.getDiscussion(this.note.discussion_id);
+    },
   },
   mounted() {
     this.renderGFM();
@@ -56,8 +63,8 @@ export default {
     renderGFM() {
       $(this.$refs['note-body']).renderGFM();
     },
-    handleFormUpdate(note, parentElement, callback) {
-      this.$emit('handleFormUpdate', note, parentElement, callback);
+    handleFormUpdate(note, parentElement, callback, resolveDiscussion) {
+      this.$emit('handleFormUpdate', note, parentElement, callback, resolveDiscussion);
     },
     formCancelHandler(shouldConfirm, isDirty) {
       this.$emit('cancelForm', shouldConfirm, isDirty);
@@ -76,6 +83,8 @@ export default {
       :note-body="noteBody"
       :note-id="note.id"
       :markdown-version="note.cached_markdown_version"
+      :discussion="discussion"
+      :resolve-discussion="note.resolve_discussion"
       @handleFormUpdate="handleFormUpdate"
       @cancelForm="formCancelHandler"
     />

app/assets/javascripts/notes/components/note_form.vue

@@ -51,14 +51,19 @@ export default {
       required: false,
       default: '',
     },
+    resolveDiscussion: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
       updatedNoteBody: this.noteBody,
       conflictWhileEditing: false,
       isSubmitting: false,
-      isResolving: false,
-      isUnresolving: false,
+      isResolving: this.resolveDiscussion,
+      isUnresolving: !this.resolveDiscussion,
       resolveAsThread: true,
     };
   },
@@ -122,13 +127,19 @@ export default {
       const beforeSubmitDiscussionState = this.discussionResolved;
       this.isSubmitting = true;
 
-      this.$emit('handleFormUpdate', this.updatedNoteBody, this.$refs.editNoteForm, () => {
+      this.$emit(
+        'handleFormUpdate',
+        this.updatedNoteBody,
+        this.$refs.editNoteForm,
+        () => {
           this.isSubmitting = false;
 
           if (this.shouldToggleResolved(shouldResolve, beforeSubmitDiscussionState)) {
             this.resolveHandler(beforeSubmitDiscussionState);
           }
-      });
+        },
+        this.discussionResolved ? !this.isUnresolving : this.isResolving,
+      );
     },
     editMyLastNote() {
       if (this.updatedNoteBody === '') {
@@ -153,7 +164,7 @@ export default {
   <div ref="editNoteForm" class="note-edit-form current-note-edit-form js-discussion-note-form">
     <div v-if="conflictWhileEditing" class="js-conflict-edit-warning alert alert-danger">
       This comment has changed since you started editing, please review the
-      <a :href="noteHash" target="_blank" rel="noopener noreferrer"> updated comment </a> to ensure
+      <a :href="noteHash" target="_blank" rel="noopener noreferrer">updated comment</a> to ensure
       information is not lost.
     </div>
     <div class="flash-container timeline-content"></div>
@@ -178,20 +189,17 @@ export default {
           v-model="updatedNoteBody"
           :data-supports-quick-actions="!isEditing"
           name="note[note]"
-          class="note-textarea js-gfm-input js-note-text
-js-autosize markdown-area js-vue-issue-note-form js-vue-textarea qa-reply-input"
+          class="note-textarea js-gfm-input js-note-text js-autosize markdown-area js-vue-issue-note-form js-vue-textarea qa-reply-input"
           aria-label="Description"
           placeholder="Write a comment or drag your files here…"
           @keydown.meta.enter="handleUpdate();"
           @keydown.ctrl.enter="handleUpdate();"
           @keydown.up="editMyLastNote();"
           @keydown.esc="cancelHandler(true);"
-        >
-        </textarea>
+        ></textarea>
       </markdown-field>
       <div class="note-form-actions clearfix">
-        <template v-if="showBatchCommentsActions">
-          <p v-if="discussion && discussion.id">
+        <p v-if="(discussion && discussion.id) || isDraft">
           <label>
             <template v-if="discussionResolved">
               <input
@@ -207,19 +215,15 @@ js-autosize markdown-area js-vue-issue-note-form js-vue-textarea qa-reply-input"
             </template>
           </label>
         </p>
-          <div>
+        <div v-if="showBatchCommentsActions">
           <button
             :disabled="isDisabled"
             type="button"
             class="btn btn-success qa-start-review"
-              @click="handleAddToReview();"
+            @click="handleAddToReview"
           >
-              <template v-if="hasDrafts">
-                {{ __('Add to review') }}
-              </template>
-              <template v-else>
-                {{ __('Start a review') }}
-              </template>
+            <template v-if="hasDrafts">{{ __('Add to review') }}</template>
+            <template v-else>{{ __('Start a review') }}</template>
           </button>
           <button
             :disabled="isDisabled"
@@ -237,12 +241,11 @@ js-autosize markdown-area js-vue-issue-note-form js-vue-textarea qa-reply-input"
             {{ __('Cancel') }}
           </button>
         </div>
-        </template>
         <template v-else>
           <button
             :disabled="isDisabled"
             type="button"
-            class="js-vue-issue-save btn btn-success js-comment-button "
+            class="js-vue-issue-save btn btn-success js-comment-button"
             @click="handleUpdate();"
           >
             {{ saveButtonTitle }}

app/assets/javascripts/notes/components/note_header.vue

@@ -69,24 +69,23 @@ export default {
         type="button"
         @click="handleToggle"
       >
-        <i :class="toggleChevronClass" class="fa" aria-hidden="true"> </i>
+        <i :class="toggleChevronClass" class="fa" aria-hidden="true"></i>
         {{ __('Toggle discussion') }}
       </button>
     </div>
     <a v-if="hasAuthor" v-once :href="author.path">
+      <slot name="note-header-info"></slot>
       <span class="note-header-author-name">{{ author.name }}</span>
       <span v-if="author.status_tooltip_html" v-html="author.status_tooltip_html"></span>
-      <span class="note-headline-light"> @{{ author.username }} </span>
+      <span class="note-headline-light">@{{ author.username }}</span>
     </a>
-    <span v-else> {{ __('A deleted user') }} </span>
+    <span v-else>{{ __('A deleted user') }}</span>
     <span class="note-headline-light">
       <span class="note-headline-meta">
         <span class="system-note-message"> <slot></slot> </span>
         <template v-if="createdAt">
           <span class="system-note-separator">
-            <template v-if="actionText">
-              {{ actionText }}
-            </template>
+            <template v-if="actionText">{{ actionText }}</template>
           </span>
           <a
             :href="noteTimestampLink"
@@ -100,8 +99,7 @@ export default {
           class="fa fa-spinner fa-spin editing-spinner"
           aria-label="Comment is being updated"
           aria-hidden="true"
-        >
-        </i>
+        ></i>
       </span>
     </span>
   </div>

app/assets/javascripts/notes/components/noteable_note.vue

@@ -111,10 +111,11 @@ export default {
       this.$refs.noteBody.resetAutoSave();
       this.$emit('updateSuccess');
     },
-    formUpdateHandler(noteText, parentElement, callback) {
+    formUpdateHandler(noteText, parentElement, callback, resolveDiscussion) {
       this.$emit('handleUpdateNote', {
         note: this.note,
         noteText,
+        resolveDiscussion,
         callback: () => this.updateSuccess(),
       });
 
@@ -198,7 +199,9 @@ export default {
             :created-at="note.created_at"
             :note-id="note.id"
             action-text="commented"
-          />
+          >
+            <slot slot="note-header-info" name="note-header-info"> </slot>
+          </note-header>
           <note-actions
             :author-id="author.id"
             :note-id="note.id"
@@ -208,12 +211,16 @@ export default {
             :can-award-emoji="note.current_user.can_award_emoji"
             :can-delete="note.current_user.can_edit"
             :can-report-as-abuse="canReportAsAbuse"
-            :can-resolve="note.current_user.can_resolve"
+            :can-resolve="
+              note.current_user.can_resolve || (note.isDraft && note.discussion_id !== null)
+            "
             :report-abuse-path="note.report_abuse_path"
-            :resolvable="note.resolvable"
-            :is-resolved="note.resolved"
+            :resolvable="note.resolvable || note.isDraft"
+            :is-resolved="note.resolved || note.resolve_discussion"
             :is-resolving="isResolving"
             :resolved-by="note.resolved_by"
+            :discussion-id="note.isDraft && note.discussion_id"
+            :resolve-discussion="note.isDraft && note.resolve_discussion"
             @handleEdit="editHandler"
             @handleDelete="deleteHandler"
             @handleResolve="resolveHandler"

app/assets/javascripts/notes/mixins/resolvable.js

@@ -31,12 +31,16 @@ export default {
   },
   methods: {
     resolveHandler(resolvedState = false) {
+      if (this.note && this.note.isDraft) {
+        return this.$emit('toggleResolveStatus');
+      }
+
       this.isResolving = true;
       const isResolved = this.discussionResolved || resolvedState;
       const discussion = this.resolveAsThread;
       const endpoint = discussion ? this.discussion.resolve_path : `${this.note.path}/resolve`;
 
-      this.toggleResolveNote({ endpoint, isResolved, discussion })
+      return this.toggleResolveNote({ endpoint, isResolved, discussion })
         .then(() => {
           this.isResolving = false;
         })

ee/app/assets/javascripts/batch_comments/components/draft_note.vue

@@ -3,7 +3,6 @@ import { mapActions, mapGetters, mapState } from 'vuex';
 import Icon from '~/vue_shared/components/icon.vue';
 import NoteableNote from '~/notes/components/noteable_note.vue';
 import LoadingButton from '~/vue_shared/components/loading_button.vue';
-import resolvedStatusMixin from '../mixins/resolved_status';
 import PublishButton from './publish_button.vue';
 
 export default {
@@ -13,7 +12,6 @@ export default {
     Icon,
     LoadingButton,
   },
-  mixins: [resolvedStatusMixin],
   props: {
     draft: {
       type: Object,
@@ -43,6 +41,7 @@ export default {
       'updateDraft',
       'publishSingleDraft',
       'scrollToDraft',
+      'toggleResolveDiscussion',
     ]),
     update(data) {
       this.updateDraft(data);
@@ -57,18 +56,10 @@ export default {
       this.isEditingDraft = false;
     },
   },
-  showStaysResolved: true,
 };
 </script>
 <template>
-  <article :class="componentClasses" class="draft-note-component note-wrapper">
-    <header class="draft-note-header">
-      <strong class="badge draft-pending-label"> {{ __('Pending') }} </strong>
-      <p v-if="draft.discussion_id" class="draft-note-resolution">
-        <Icon :size="16" name="status_success" />
-        {{ __(resolvedStatusMessage) }}
-      </p>
-    </header>
+  <article class="draft-note-component note-wrapper">
     <ul class="notes draft-notes">
       <noteable-note
         :note="draft"
@@ -78,7 +69,12 @@ export default {
         @updateSuccess="handleNotEditing"
         @handleDeleteNote="deleteDraft"
         @handleUpdateNote="update"
-      />
+        @toggleResolveStatus="toggleResolveDiscussion(draft.id);"
+      >
+        <strong slot="note-header-info" class="badge draft-pending-label append-right-4">
+          {{ __('Pending') }}
+        </strong>
+      </noteable-note>
     </ul>
 
     <template v-if="!isEditingDraft">
@@ -88,11 +84,11 @@ export default {
         v-html="draftCommands"
       ></div>
 
-      <p class="draft-note-actions">
+      <p class="draft-note-actions d-flex">
         <publish-button
           :show-count="true"
           :should-publish="false"
-          class="btn btn-success btn-inverted"
+          class="btn btn-success btn-inverted append-right-8"
         />
         <loading-button
           :loading="isPublishingDraft(draft.id) || isPublishing"

ee/app/assets/javascripts/batch_comments/mixins/note_form.js

 import { mapGetters, mapState } from 'vuex';
 
 export default {
+  props: {
+    isDraft: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
   computed: {
     ...mapState({
       withBatchComments: state => state.batchComments && state.batchComments.withBatchComments,
@@ -21,20 +28,6 @@ export default {
 
       return resolveStatus;
     },
-    handleUpdate(resolveStatus) {
-      const beforeSubmitDiscussionState = this.discussionResolved;
-      this.isSubmitting = true;
-
-      const shouldBeResolved = this.shouldBeResolved(resolveStatus) !== beforeSubmitDiscussionState;
-
-      this.$emit('handleFormUpdate', this.updatedNoteBody, this.$refs.editNoteForm, () => {
-        this.isSubmitting = false;
-
-        if (resolveStatus || (shouldBeResolved && this.withBatchComments)) {
-          this.resolveHandler(beforeSubmitDiscussionState); // this will toggle the state
-        }
-      });
-    },
     handleAddToReview() {
       // check if draft should resolve discussion
       const shouldResolve =

ee/app/assets/javascripts/batch_comments/mixins/resolved_status.js

@@ -2,12 +2,28 @@ import { mapGetters } from 'vuex';
 import { s__ } from '~/locale';
 
 export default {
+  props: {
+    discussionId: {
+      type: String,
+      required: false,
+      default: null,
+    },
+    resolveDiscussion: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
   computed: {
     ...mapGetters(['isDiscussionResolved']),
     resolvedStatusMessage() {
       let message;
-      const discussionResolved = this.isDiscussionResolved(this.draft.discussion_id);
-      const discussionToBeResolved = this.draft.resolve_discussion;
+      const discussionResolved = this.isDiscussionResolved(
+        this.draft ? this.draft.discussion_id : this.discussionId,
+      );
+      const discussionToBeResolved = this.draft
+        ? this.draft.resolve_discussion
+        : this.resolveDiscussion;
 
       if (discussionToBeResolved && discussionResolved && !this.$options.showStaysResolved) {
         return undefined;
@@ -15,22 +31,20 @@ export default {
 
       if (discussionToBeResolved) {
         if (discussionResolved) {
-          message = s__('MergeRequests|Discussion stays resolved.');
+          message = s__('MergeRequests|Discussion stays resolved');
         } else {
-          message = s__('MergeRequests|Discussion will be resolved.');
+          message = s__('MergeRequests|Discussion will be resolved');
         }
       } else if (discussionResolved) {
-        message = s__('MergeRequests|Discussion will be unresolved.');
+        message = s__('MergeRequests|Discussion will be unresolved');
       } else if (this.$options.showStaysResolved) {
-        message = s__('MergeRequests|Discussion stays unresolved.');
+        message = s__('MergeRequests|Discussion stays unresolved');
       }
 
       return message;
     },
     componentClasses() {
-      return this.draft.resolve_discussion
-        ? 'is-resolving-discussion'
-        : 'is-unresolving-discussion';
+      return this.resolveDiscussion ? 'is-resolving-discussion' : 'is-unresolving-discussion';
     },
   },
 };

ee/app/assets/javascripts/batch_comments/services/drafts_service.js

@@ -28,7 +28,11 @@ export default {
   discard(endpoint) {
     return Vue.http.delete(endpoint);
   },
-  update(endpoint, { draftId, note }) {
-    return Vue.http.put(`${endpoint}/${draftId}`, { draft_note: { note } }, { emulateJSON: true });
+  update(endpoint, { draftId, note, resolveDiscussion }) {
+    return Vue.http.put(
+      `${endpoint}/${draftId}`,
+      { draft_note: { note, resolve_discussion: resolveDiscussion } },
+      { emulateJSON: true },
+    );
   },
 };

ee/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js

@@ -88,9 +88,13 @@ export const discardReview = ({ commit, getters }) => {
     .catch(() => commit(types.RECEIVE_DISCARD_REVIEW_ERROR));
 };
 
-export const updateDraft = ({ commit, getters }, { note, noteText, callback }) =>
+export const updateDraft = ({ commit, getters }, { note, noteText, resolveDiscussion, callback }) =>
   service
-    .update(getters.getNotesData.draftsPath, { draftId: note.id, note: noteText })
+    .update(getters.getNotesData.draftsPath, {
+      draftId: note.id,
+      note: noteText,
+      resolveDiscussion,
+    })
     .then(res => res.json())
     .then(data => commit(types.RECEIVE_DRAFT_UPDATE_SUCCESS, data))
     .then(callback)
@@ -139,5 +143,9 @@ export const expandAllDiscussions = ({ dispatch, state }) =>
       dispatch('expandDiscussion', { discussionId: draft.discussion_id }, { root: true });
     });
 
+export const toggleResolveDiscussion = ({ commit }, draftId) => {
+  commit(types.TOGGLE_RESOLVE_DISCUSSION, draftId);
+};
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

ee/app/assets/javascripts/batch_comments/stores/modules/batch_comments/mutation_types.js

@@ -19,3 +19,5 @@ export const RECEIVE_DRAFT_UPDATE_SUCCESS = 'RECEIVE_DRAFT_UPDATE_SUCCESS';
 
 export const OPEN_REVIEW_DROPDOWN = 'OPEN_REVIEW_DROPDOWN';
 export const CLOSE_REVIEW_DROPDOWN = 'CLOSE_REVIEW_DROPDOWN';
+
+export const TOGGLE_RESOLVE_DISCUSSION = 'TOGGLE_RESOLVE_DISCUSSION';

ee/app/assets/javascripts/batch_comments/stores/modules/batch_comments/mutations.js

@@ -70,4 +70,16 @@ export default {
   [types.CLOSE_REVIEW_DROPDOWN](state) {
     state.showPreviewDropdown = false;
   },
+  [types.TOGGLE_RESOLVE_DISCUSSION](state, draftId) {
+    state.drafts = state.drafts.map(draft => {
+      if (draft.id === draftId) {
+        return {
+          ...draft,
+          resolve_discussion: !draft.resolve_discussion,
+        };
+      }
+
+      return draft;
+    });
+  },
 };

ee/app/assets/stylesheets/components/batch_comments/draft_note.scss

@@ -47,37 +47,6 @@ button[disabled] {
   }
 }
 
-.draft-note-header {
-  display: flex;
-  justify-content: space-between;
-  align-items: center;
-}
-
-.draft-note-resolution {
-  padding: 0 $gl-padding;
-  line-height: 1;
-  font-size: $label-font-size;
-  color: $theme-gray-700;
-  flex-grow: 1;
-
-  svg {
-    vertical-align: text-bottom;
-    display: inline-block;
-  }
-
-  .is-resolving-discussion & {
-    svg {
-      color: $green-600;
-    }
-  }
-
-  .is-unresolving-discussion & {
-    svg {
-      color: $gray-darkest;
-    }
-  }
-}
-
 .draft-pending-label {
   background: $orange-600;
   color: $white-light;
@@ -88,14 +57,10 @@ button[disabled] {
 .diff-file {
   .notes .note {
     &.draft-note {
-      margin: $gl-padding-8 0 $gl-padding;
+      margin: 0 0 $gl-padding;
       padding: 0;
       border: 0;
 
-      .note-actions {
-        margin-top: -26px;
-      }
-
       &.is-editing {
         margin-bottom: 0;
       }

ee/spec/javascripts/batch_comments/components/draft_note_spec.js

@@ -35,58 +35,13 @@ describe('Batch comments draft note component', () => {
 
   describe('in discussion', () => {
     beforeEach(done => {
-      vm.draft.discussion_id = 123;
+      vm.draft.discussion_id = '123';
 
       vm.$nextTick(done);
     });
 
     it('renders resolution status', () => {
-      expect(vm.$el.querySelector('.draft-note-resolution')).not.toBe(null);
-    });
-
-    describe('resolvedStatusMessage', () => {
-      describe('resolve discussion', () => {
-        it('return will be resolved text', () => {
-          vm.draft.resolve_discussion = true;
-
-          expect(vm.resolvedStatusMessage).toBe('Discussion will be resolved.');
-        });
-
-        it('return will be stays resolved text', () => {
-          spyOnProperty(vm, 'isDiscussionResolved').and.returnValue(() => true);
-          vm.draft.resolve_discussion = true;
-
-          expect(vm.resolvedStatusMessage).toBe('Discussion stays resolved.');
-        });
-      });
-
-      describe('unresolve discussion', () => {
-        it('return will be stays unresolved text', () => {
-          expect(vm.resolvedStatusMessage).toBe('Discussion stays unresolved.');
-        });
-
-        it('return will be unresolved text', () => {
-          spyOnProperty(vm, 'isDiscussionResolved').and.returnValue(() => true);
-
-          vm.$forceUpdate();
-
-          expect(vm.resolvedStatusMessage).toBe('Discussion stays unresolved.');
-        });
-      });
-
-      it('adds resolving class to element', done => {
-        vm.draft.resolve_discussion = true;
-
-        vm.$nextTick(() => {
-          expect(vm.$el.classList).toContain('is-resolving-discussion');
-
-          done();
-        });
-      });
-
-      it('adds unresolving class to element', () => {
-        expect(vm.$el.classList).toContain('is-unresolving-discussion');
-      });
+      expect(vm.$el.querySelector('.line-resolve-btn')).not.toBe(null);
     });
   });
 
@@ -121,6 +76,7 @@ describe('Batch comments draft note component', () => {
           expect(vm.$store.dispatch).toHaveBeenCalledWith('batchComments/updateDraft', {
             note: draft,
             noteText: 'a',
+            resolveDiscussion: false,
             callback: jasmine.any(Function),
           });
         })

locale/gitlab.pot

@@ -5202,16 +5202,16 @@ msgstr ""
 msgid "MergeRequests|An error occurred while saving the draft comment."
 msgstr ""
 
-msgid "MergeRequests|Discussion stays resolved."
+msgid "MergeRequests|Discussion stays resolved"
 msgstr ""
 
-msgid "MergeRequests|Discussion stays unresolved."
+msgid "MergeRequests|Discussion stays unresolved"
 msgstr ""
 
-msgid "MergeRequests|Discussion will be resolved."
+msgid "MergeRequests|Discussion will be resolved"
 msgstr ""
 
-msgid "MergeRequests|Discussion will be unresolved."
+msgid "MergeRequests|Discussion will be unresolved"
 msgstr ""
 
 msgid "MergeRequests|Resolve this discussion in a new issue"