Merge branch 'fix-scroll-to-note-designs' into 'master'

Fix scroll to note designs

See merge request gitlab-org/gitlab!41062

app/assets/javascripts/design_management/components/design_notes/design_discussion.vue

@@ -105,8 +105,8 @@ export default {
         atVersion: this.designsVersion,
       };
     },
-    isDiscussionHighlighted() {
-      return this.discussion.notes[0].id === this.activeDiscussion.id;
+    isDiscussionActive() {
+      return this.discussion.notes.some(({ id }) => id === this.activeDiscussion.id);
     },
     resolveCheckboxText() {
       return this.discussion.resolved
@@ -134,18 +134,6 @@ export default {
     isFormVisible() {
       return this.isFormRendered && this.discussionWithOpenForm === this.discussion.id;
     },
-    shouldScrollToDiscussion(activeDiscussion) {
-      const ALLOWED_ACTIVE_DISCUSSION_SOURCES = [
-        ACTIVE_DISCUSSION_SOURCE_TYPES.pin,
-        ACTIVE_DISCUSSION_SOURCE_TYPES.url,
-      ];
-      const { id: activeDiscussionId, source: activeDiscussionSource } = activeDiscussion;
-
-      return (
-        ALLOWED_ACTIVE_DISCUSSION_SOURCES.includes(activeDiscussionSource) &&
-        activeDiscussionId === this.discussion.notes[0].id
-      );
-    },
   },
   methods: {
     addDiscussionComment(
@@ -199,6 +187,14 @@ export default {
           this.isResolving = false;
         });
     },
+    shouldScrollToDiscussion(activeDiscussion) {
+      const ALLOWED_ACTIVE_DISCUSSION_SOURCES = [
+        ACTIVE_DISCUSSION_SOURCE_TYPES.pin,
+        ACTIVE_DISCUSSION_SOURCE_TYPES.url,
+      ];
+      const { source } = activeDiscussion;
+      return ALLOWED_ACTIVE_DISCUSSION_SOURCES.includes(source) && this.isDiscussionActive;
+    },
   },
   createNoteMutation,
 };
@@ -221,7 +217,7 @@ export default {
         :note="firstNote"
         :markdown-preview-path="markdownPreviewPath"
         :is-resolving="isResolving"
-        :class="{ 'gl-bg-blue-50': isDiscussionHighlighted }"
+        :class="{ 'gl-bg-blue-50': isDiscussionActive }"
         @error="$emit('updateNoteError', $event)"
       >
         <template v-if="discussion.resolvable" #resolveDiscussion>
@@ -265,7 +261,7 @@ export default {
         :note="note"
         :markdown-preview-path="markdownPreviewPath"
         :is-resolving="isResolving"
-        :class="{ 'gl-bg-blue-50': isDiscussionHighlighted }"
+        :class="{ 'gl-bg-blue-50': isDiscussionActive }"
         @error="$emit('updateNoteError', $event)"
       />
       <li v-show="isReplyPlaceholderVisible" class="reply-wrapper">

app/assets/javascripts/design_management/components/design_notes/design_note.vue

@@ -7,7 +7,7 @@ import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link
 import TimelineEntryItem from '~/vue_shared/components/notes/timeline_entry_item.vue';
 import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue';
 import DesignReplyForm from './design_reply_form.vue';
-import { findNoteId } from '../../utils/design_management_utils';
+import { findNoteId, extractDesignNoteId } from '../../utils/design_management_utils';
 import { hasErrors } from '../../utils/cache_update';
 
 export default {
@@ -47,7 +47,7 @@ export default {
       return findNoteId(this.note.id);
     },
     isNoteLinked() {
-      return this.$route.hash === `#note_${this.noteAnchorId}`;
+      return extractDesignNoteId(this.$route.hash) === this.noteAnchorId;
     },
     mutationPayload() {
       return {
@@ -59,13 +59,6 @@ export default {
       return !this.isEditing && this.note.userPermissions.adminNote;
     },
   },
-  mounted() {
-    this.$nextTick(() => {
-      if (this.isNoteLinked) {
-        this.$el.scrollIntoView({ behavior: 'smooth', inline: 'start' });
-      }
-    });
-  },
   methods: {
     hideForm() {
       this.isEditing = false;

app/assets/javascripts/design_management/components/design_overlay.vue

@@ -237,7 +237,12 @@ export default {
       });
     },
     isNoteInactive(note) {
-      return this.activeDiscussion.id && this.activeDiscussion.id !== note.id;
+      const discussionNotes = note.discussion.notes.nodes || [];
+
+      return (
+        this.activeDiscussion.id &&
+        !discussionNotes.some(({ id }) => id === this.activeDiscussion.id)
+      );
     },
     designPinClass(note) {
       return { inactive: this.isNoteInactive(note), resolved: note.resolved };

app/assets/javascripts/design_management/graphql/fragments/design_note.fragment.graphql

@@ -25,5 +25,10 @@ fragment DesignNote on Note {
   }
   discussion {
     id
+    notes {
+      nodes {
+        id
+      }
+    }
   }
 }

changelogs/unreleased/fix-scroll-to-note-designs.yml

+---
+title: Highlight design discussion if any comment in discussion is linked
+merge_request: 41062
+author:
+type: fixed

spec/frontend/design_management/components/design_notes/design_discussion_spec.js

@@ -8,8 +8,9 @@ import createNoteMutation from '~/design_management/graphql/mutations/create_not
 import toggleResolveDiscussionMutation from '~/design_management/graphql/mutations/toggle_resolve_discussion.mutation.graphql';
 import ReplyPlaceholder from '~/notes/components/discussion_reply_placeholder.vue';
 import ToggleRepliesWidget from '~/design_management/components/design_notes/toggle_replies_widget.vue';
+import mockDiscussion from '../../mock_data/discussion';
 
-const discussion = {
+const defaultMockDiscussion = {
   id: '0',
   resolved: false,
   resolvable: true,
@@ -49,7 +50,7 @@ describe('Design discussions component', () => {
     wrapper = mount(DesignDiscussion, {
       propsData: {
         resolvedDiscussionsExpanded: true,
-        discussion,
+        discussion: defaultMockDiscussion,
         noteableId: 'noteable-id',
         designId: 'design-id',
         discussionIndex: 1,
@@ -82,7 +83,7 @@ describe('Design discussions component', () => {
     beforeEach(() => {
       createComponent({
         discussion: {
-          ...discussion,
+          ...defaultMockDiscussion,
           resolvable: false,
         },
       });
@@ -125,7 +126,7 @@ describe('Design discussions component', () => {
 
     it('renders a checkbox with Resolve thread text in reply form', () => {
       findReplyPlaceholder().vm.$emit('onClick');
-      wrapper.setProps({ discussionWithOpenForm: discussion.id });
+      wrapper.setProps({ discussionWithOpenForm: defaultMockDiscussion.id });
 
       return wrapper.vm.$nextTick().then(() => {
         expect(findResolveCheckbox().text()).toBe('Resolve thread');
@@ -141,7 +142,7 @@ describe('Design discussions component', () => {
     beforeEach(() => {
       createComponent({
         discussion: {
-          ...discussion,
+          ...defaultMockDiscussion,
           resolved: true,
           resolvedBy: notes[0].author,
           resolvedAt: '2020-05-08T07:10:45Z',
@@ -206,7 +207,7 @@ describe('Design discussions component', () => {
 
       it('renders a checkbox with Unresolve thread text in reply form', () => {
         findReplyPlaceholder().vm.$emit('onClick');
-        wrapper.setProps({ discussionWithOpenForm: discussion.id });
+        wrapper.setProps({ discussionWithOpenForm: defaultMockDiscussion.id });
 
         return wrapper.vm.$nextTick().then(() => {
           expect(findResolveCheckbox().text()).toBe('Unresolve thread');
@@ -218,7 +219,7 @@ describe('Design discussions component', () => {
   it('hides reply placeholder and opens form on placeholder click', () => {
     createComponent();
     findReplyPlaceholder().vm.$emit('onClick');
-    wrapper.setProps({ discussionWithOpenForm: discussion.id });
+    wrapper.setProps({ discussionWithOpenForm: defaultMockDiscussion.id });
 
     return wrapper.vm.$nextTick().then(() => {
       expect(findReplyPlaceholder().exists()).toBe(false);
@@ -228,7 +229,7 @@ describe('Design discussions component', () => {
 
   it('calls mutation on submitting form and closes the form', () => {
     createComponent(
-      { discussionWithOpenForm: discussion.id },
+      { discussionWithOpenForm: defaultMockDiscussion.id },
       { discussionComment: 'test', isFormRendered: true },
     );
 
@@ -246,7 +247,7 @@ describe('Design discussions component', () => {
 
   it('clears the discussion comment on closing comment form', () => {
     createComponent(
-      { discussionWithOpenForm: discussion.id },
+      { discussionWithOpenForm: defaultMockDiscussion.id },
       { discussionComment: 'test', isFormRendered: true },
     );
 
@@ -263,19 +264,26 @@ describe('Design discussions component', () => {
       });
   });
 
-  it('applies correct class to design notes when discussion is highlighted', () => {
-    createComponent(
-      {},
-      {
-        activeDiscussion: {
-          id: notes[0].id,
-          source: 'pin',
-        },
-      },
-    );
+  describe('when any note from a discussion is active', () => {
+    it.each([notes[0], notes[0].discussion.notes.nodes[1]])(
+      'applies correct class to all notes in the active discussion',
+      note => {
+        createComponent(
+          { discussion: mockDiscussion },
+          {
+            activeDiscussion: {
+              id: note.id,
+              source: 'pin',
+            },
+          },
+        );
 
-    expect(wrapper.findAll(DesignNote).wrappers.every(note => note.classes('gl-bg-blue-50'))).toBe(
-      true,
+        expect(
+          wrapper
+            .findAll(DesignNote)
+            .wrappers.every(designNote => designNote.classes('gl-bg-blue-50')),
+        ).toBe(true);
+      },
     );
   });
 
@@ -285,7 +293,7 @@ describe('Design discussions component', () => {
     expect(mutate).toHaveBeenCalledWith({
       mutation: toggleResolveDiscussionMutation,
       variables: {
-        id: discussion.id,
+        id: defaultMockDiscussion.id,
         resolve: true,
       },
     });
@@ -296,7 +304,7 @@ describe('Design discussions component', () => {
 
   it('calls toggleResolveDiscussion mutation after adding a note if checkbox was checked', () => {
     createComponent(
-      { discussionWithOpenForm: discussion.id },
+      { discussionWithOpenForm: defaultMockDiscussion.id },
       { discussionComment: 'test', isFormRendered: true },
     );
     findResolveButton().trigger('click');
@@ -306,7 +314,7 @@ describe('Design discussions component', () => {
       expect(mutate).toHaveBeenCalledWith({
         mutation: toggleResolveDiscussionMutation,
         variables: {
-          id: discussion.id,
+          id: defaultMockDiscussion.id,
           resolve: true,
         },
       });

spec/frontend/design_management/components/design_notes/design_note_spec.js

@@ -86,16 +86,6 @@ describe('Design note component', () => {
     expect(wrapper.find(TimeAgoTooltip).exists()).toBe(true);
   });
 
-  it('should trigger a scrollIntoView method', () => {
-    createComponent({
-      note,
-    });
-
-    return wrapper.vm.$nextTick().then(() => {
-      expect(scrollIntoViewMock).toHaveBeenCalled();
-    });
-  });
-
   it('should not render edit icon when user does not have a permission', () => {
     createComponent({
       note,

spec/frontend/design_management/components/design_overlay_spec.js

@@ -13,8 +13,9 @@ describe('Design overlay component', () => {
 
   const findAllNotes = () => wrapper.findAll('.js-image-badge');
   const findCommentBadge = () => wrapper.find('.comment-indicator');
-  const findFirstBadge = () => findAllNotes().at(0);
-  const findSecondBadge = () => findAllNotes().at(1);
+  const findBadgeAtIndex = noteIndex => findAllNotes().at(noteIndex);
+  const findFirstBadge = () => findBadgeAtIndex(0);
+  const findSecondBadge = () => findBadgeAtIndex(1);
 
   const clickAndDragBadge = (elem, fromPoint, toPoint) => {
     elem.trigger('mousedown', { clientX: fromPoint.x, clientY: fromPoint.y });
@@ -104,16 +105,43 @@ describe('Design overlay component', () => {
         expect(findSecondBadge().classes()).toContain('resolved');
       });
 
-      it('when there is an active discussion, should apply inactive class to all pins besides the active one', () => {
-        wrapper.setData({
-          activeDiscussion: {
-            id: notes[0].id,
-            source: 'discussion',
-          },
+      describe('when no discussion is active', () => {
+        it('should not apply inactive class to any pins', () => {
+          expect(
+            findAllNotes(0).wrappers.every(designNote => designNote.classes('gl-bg-blue-50')),
+          ).toBe(false);
         });
+      });
+
+      describe('when a discussion is active', () => {
+        it.each([notes[0].discussion.notes.nodes[1], notes[0].discussion.notes.nodes[0]])(
+          'should not apply inactive class to the pin for the active discussion',
+          note => {
+            wrapper.setData({
+              activeDiscussion: {
+                id: note.id,
+                source: 'discussion',
+              },
+            });
 
-        return wrapper.vm.$nextTick().then(() => {
-          expect(findSecondBadge().classes()).toContain('inactive');
+            return wrapper.vm.$nextTick().then(() => {
+              expect(findBadgeAtIndex(0).classes()).not.toContain('inactive');
+            });
+          },
+        );
+
+        it('should apply inactive class to all pins besides the active one', () => {
+          wrapper.setData({
+            activeDiscussion: {
+              id: notes[0].id,
+              source: 'discussion',
+            },
+          });
+
+          return wrapper.vm.$nextTick().then(() => {
+            expect(findSecondBadge().classes()).toContain('inactive');
+            expect(findFirstBadge().classes()).not.toContain('inactive');
+          });
         });
       });
     });

spec/frontend/design_management/mock_data/discussion.js

+export default {
+  id: 'discussion-id-1',
+  resolved: false,
+  resolvable: true,
+  notes: [
+    {
+      id: 'note-id-1',
+      index: 1,
+      position: {
+        height: 100,
+        width: 100,
+        x: 10,
+        y: 15,
+      },
+      author: {
+        name: 'John',
+        webUrl: 'link-to-john-profile',
+      },
+      createdAt: '2020-05-08T07:10:45Z',
+      userPermissions: {
+        adminNote: true,
+      },
+      resolved: false,
+    },
+    {
+      id: 'note-id-3',
+      index: 3,
+      position: {
+        height: 50,
+        width: 50,
+        x: 25,
+        y: 25,
+      },
+      author: {
+        name: 'Mary',
+        webUrl: 'link-to-mary-profile',
+      },
+      createdAt: '2020-05-08T07:10:45Z',
+      userPermissions: {
+        adminNote: true,
+      },
+      resolved: false,
+    },
+  ],
+};

spec/frontend/design_management/mock_data/notes.js

+import DISCUSSION_1 from './discussion';
+
+const DISCUSSION_2 = {
+  id: 'discussion-id-2',
+  notes: {
+    nodes: [
+      {
+        id: 'note-id-2',
+        index: 2,
+        position: {
+          height: 50,
+          width: 50,
+          x: 25,
+          y: 25,
+        },
+        author: {
+          name: 'Mary',
+          webUrl: 'link-to-mary-profile',
+        },
+        createdAt: '2020-05-08T07:10:45Z',
+        userPermissions: {
+          adminNote: true,
+        },
+        resolved: true,
+      },
+    ],
+  },
+};
+
 export default [
   {
-    id: 'note-id-1',
-    index: 1,
-    position: {
-      height: 100,
-      width: 100,
-      x: 10,
-      y: 15,
-    },
-    author: {
-      name: 'John',
-      webUrl: 'link-to-john-profile',
-    },
-    createdAt: '2020-05-08T07:10:45Z',
-    userPermissions: {
-      adminNote: true,
-    },
+    ...DISCUSSION_1.notes[0],
     discussion: {
-      id: 'discussion-id-1',
+      id: DISCUSSION_1.id,
+      notes: {
+        nodes: DISCUSSION_1.notes,
+      },
     },
-    resolved: false,
   },
   {
-    id: 'note-id-2',
-    index: 2,
-    position: {
-      height: 50,
-      width: 50,
-      x: 25,
-      y: 25,
-    },
-    author: {
-      name: 'Mary',
-      webUrl: 'link-to-mary-profile',
-    },
-    createdAt: '2020-05-08T07:10:45Z',
-    userPermissions: {
-      adminNote: true,
-    },
-    discussion: {
-      id: 'discussion-id-2',
-    },
-    resolved: true,
+    ...DISCUSSION_2.notes.nodes[0],
+    discussion: DISCUSSION_2,
   },
 ];