Merge branch 'moveable-new-comment-pin' into 'master'

Design view: moveable "new comment" pin

See merge request gitlab-org/gitlab!24769

doc/user/project/issues/design_management.md

@@ -130,7 +130,7 @@ Once selected, click the **Delete selected** button to confirm the deletion:
 
 ![Delete multiple designs](img/delete_multiple_designs_v12_4.png)
 
-NOTE: **Note:**
+**Note:**
 Only the latest version of the designs can be deleted.
 Deleted designs are not permanently lost; they can be
 viewed by browsing previous versions.
@@ -144,6 +144,9 @@ which you can start a new discussion:
 
 ![Starting a new discussion on design](img/adding_note_to_design_1.png)
 
+From GitLab 12.8 on, when you are starting a new discussion, you can adjust the badge's position by
+dragging it around the image.
+
 Different discussions have different badge numbers:
 
 ![Discussions on design annotations](img/adding_note_to_design_2.png)

ee/app/assets/javascripts/design_management/components/design_note_pin.vue

@@ -28,12 +28,9 @@ export default {
       return this.label === null;
     },
     pinStyle() {
-      const style = this.position;
-      if (this.repositioning) {
-        style.cursor = 'move';
-      }
-
-      return style;
+      return this.repositioning
+        ? Object.assign({}, this.position, { cursor: 'move' })
+        : this.position;
     },
     pinLabel() {
       return this.isNewNote

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

@@ -26,6 +26,12 @@ export default {
       default: null,
     },
   },
+  data() {
+    return {
+      movingNoteNewPosition: null,
+      movingNoteStartPosition: null,
+    };
+  },
   computed: {
     overlayStyle() {
       return {
@@ -34,38 +40,121 @@ export default {
         ...this.position,
       };
     },
+    isMovingCurrentComment() {
+      return Boolean(this.movingNoteStartPosition);
+    },
+    currentCommentPositionStyle() {
+      return this.isMovingCurrentComment && this.movingNoteNewPosition
+        ? this.getNotePositionStyle(this.movingNoteNewPosition)
+        : this.getNotePositionStyle(this.currentCommentForm);
+    },
   },
   methods: {
-    clickedImage(x, y) {
+    setNewNoteCoordinates({ x, y }) {
       this.$emit('openCommentForm', { x, y });
     },
-    getNotePosition(data) {
-      const { x, y, width, height } = data;
+    getNoteRelativePosition(position) {
+      const { x, y, width, height } = position;
       const widthRatio = this.dimensions.width / width;
       const heightRatio = this.dimensions.height / height;
       return {
-        left: `${Math.round(x * widthRatio)}px`,
-        top: `${Math.round(y * heightRatio)}px`,
+        left: Math.round(x * widthRatio),
+        top: Math.round(y * heightRatio),
+      };
+    },
+    getNotePositionStyle(position) {
+      const { left, top } = this.getNoteRelativePosition(position);
+      return {
+        left: `${left}px`,
+        top: `${top}px`,
+      };
+    },
+    getMovingNotePositionDelta(e) {
+      let deltaX = 0;
+      let deltaY = 0;
+
+      if (this.movingNoteStartPosition) {
+        const { clientX, clientY } = this.movingNoteStartPosition;
+        deltaX = e.clientX - clientX;
+        deltaY = e.clientY - clientY;
+      }
+
+      return {
+        deltaX,
+        deltaY,
+      };
+    },
+    isPositionInOverlay(position) {
+      const { top, left } = this.getNoteRelativePosition(position);
+      const { height, width } = this.dimensions;
+
+      return top >= 0 && top <= height && left >= 0 && left <= width;
+    },
+    onOverlayMousemove(e) {
+      if (!this.isMovingCurrentComment) return;
+
+      const { deltaX, deltaY } = this.getMovingNotePositionDelta(e);
+      const x = this.currentCommentForm.x + deltaX;
+      const y = this.currentCommentForm.y + deltaY;
+
+      const movingNoteNewPosition = {
+        x,
+        y,
+        width: this.dimensions.width,
+        height: this.dimensions.height,
+      };
+
+      if (!this.isPositionInOverlay(movingNoteNewPosition)) {
+        this.onNewNoteMouseup();
+        return;
+      }
+
+      this.movingNoteNewPosition = movingNoteNewPosition;
+    },
+    onNewNoteMousedown({ clientX, clientY }) {
+      this.movingNoteStartPosition = {
+        clientX,
+        clientY,
       };
     },
+    onNewNoteMouseup() {
+      if (!this.movingNoteNewPosition) return;
+
+      const { x, y } = this.movingNoteNewPosition;
+      this.setNewNoteCoordinates({ x, y });
+
+      this.movingNoteStartPosition = null;
+      this.movingNoteNewPosition = null;
+    },
   },
 };
 </script>
 
 <template>
-  <div class="position-absolute image-diff-overlay frame" :style="overlayStyle">
+  <div
+    class="position-absolute image-diff-overlay frame"
+    :style="overlayStyle"
+    @mousemove="onOverlayMousemove"
+    @mouseleave="onNewNoteMouseup"
+  >
     <button
       type="button"
       class="btn-transparent position-absolute image-diff-overlay-add-comment w-100 h-100 js-add-image-diff-note-button"
       data-qa-selector="design_image_button"
-      @click="clickedImage($event.offsetX, $event.offsetY)"
+      @click="setNewNoteCoordinates({ x: $event.offsetX, y: $event.offsetY })"
     ></button>
     <design-note-pin
       v-for="(note, index) in notes"
       :key="note.id"
       :label="`${index + 1}`"
-      :position="getNotePosition(note.position)"
+      :position="getNotePositionStyle(note.position)"
+    />
+    <design-note-pin
+      v-if="currentCommentForm"
+      :position="currentCommentPositionStyle"
+      :repositioning="isMovingCurrentComment"
+      @mousedown="onNewNoteMousedown"
+      @mouseup="onNewNoteMouseup"
     />
-    <design-note-pin v-if="currentCommentForm" :position="getNotePosition(currentCommentForm)" />
   </div>
 </template>

ee/app/assets/javascripts/design_management/components/design_presentation.vue

@@ -38,7 +38,7 @@ export default {
     return {
       overlayDimensions: null,
       overlayPosition: null,
-      currentAnnotationCoordinates: null,
+      currentAnnotationPosition: null,
       zoomFocalPoint: {
         x: 0,
         y: 0,
@@ -53,7 +53,7 @@ export default {
       return this.discussions.map(discussion => discussion.notes[0]);
     },
     currentCommentForm() {
-      return (this.isAnnotating && this.currentAnnotationCoordinates) || null;
+      return (this.isAnnotating && this.currentAnnotationPosition) || null;
     },
   },
   beforeDestroy() {
@@ -73,8 +73,22 @@ export default {
     presentationViewport.addEventListener('scroll', this.scrollThrottled, false);
   },
   methods: {
+    syncCurrentAnnotationPosition() {
+      if (!this.currentAnnotationPosition) return;
+
+      const widthRatio = this.overlayDimensions.width / this.currentAnnotationPosition.width;
+      const heightRatio = this.overlayDimensions.height / this.currentAnnotationPosition.height;
+      const x = this.currentAnnotationPosition.x * widthRatio;
+      const y = this.currentAnnotationPosition.y * heightRatio;
+
+      this.currentAnnotationPosition = this.getAnnotationPositon({ x, y });
+    },
     setOverlayDimensions(overlayDimensions) {
       this.overlayDimensions = overlayDimensions;
+
+      // every time we set overlay dimensions, we need to
+      // update the current annotation as well
+      this.syncCurrentAnnotationPosition();
     },
     setOverlayPosition() {
       if (!this.overlayDimensions) {
@@ -174,16 +188,19 @@ export default {
         }
       });
     },
-    openCommentForm(position) {
-      const { x, y } = position;
+    getAnnotationPositon(coordinates) {
+      const { x, y } = coordinates;
       const { width, height } = this.overlayDimensions;
-      this.currentAnnotationCoordinates = {
+      return {
         x,
         y,
         width,
         height,
       };
-      this.$emit('openCommentForm', this.currentAnnotationCoordinates);
+    },
+    openCommentForm(coordinates) {
+      this.currentAnnotationPosition = this.getAnnotationPositon(coordinates);
+      this.$emit('openCommentForm', this.currentAnnotationPosition);
     },
   },
 };

ee/app/assets/javascripts/design_management/pages/design/index.vue

@@ -240,7 +240,8 @@ export default {
           :scale="scale"
           @openCommentForm="openCommentForm"
         />
-        <div class="design-scaler-wrapper position-absolute w-100 mb-4 d-flex-center">
+
+        <div class="design-scaler-wrapper position-absolute mb-4 d-flex-center">
           <design-scaler @scale="scale = $event" />
         </div>
       </div>

ee/app/assets/stylesheets/components/design_management/design.scss

@@ -9,7 +9,8 @@
 
 .design-scaler-wrapper {
   bottom: 0;
-  left: 0;
+  left: 50%;
+  transform: translateX(-50%);
 }
 
 .design-list-item .design-event {

ee/changelogs/unreleased/moveable-new-comment-pin.yml

+---
+title: 'Design view: moveable `new comment` pin'
+merge_request: 24769
+author:
+type: added

ee/spec/frontend/design_management/components/__snapshots__/design_presentation_spec.js.snap

 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`Design management design presentation component currentCommentForm currentCommentForm is equal to current annotation coordinates when isAnnotating is true 1`] = `
+exports[`Design management design presentation component currentCommentForm currentCommentForm is equal to current annotation position when isAnnotating is true 1`] = `
 <div
   class="h-100 w-100 p-3 overflow-auto position-relative"
 >
@@ -45,7 +45,7 @@ exports[`Design management design presentation component currentCommentForm curr
 </div>
 `;
 
-exports[`Design management design presentation component currentCommentForm currentCommentForm is null when isAnnotating is true but annotation coordinates are falsey 1`] = `
+exports[`Design management design presentation component currentCommentForm currentCommentForm is null when isAnnotating is true but annotation position is falsey 1`] = `
 <div
   class="h-100 w-100 p-3 overflow-auto position-relative"
 >

ee/spec/frontend/design_management/components/design_overlay_spec.js

@@ -23,18 +23,26 @@ describe('Design overlay component', () => {
     },
   ];
 
+  const mockDimensions = { width: 100, height: 100 };
+
+  const findOverlay = () => wrapper.find('.image-diff-overlay');
   const findAllNotes = () => wrapper.findAll('.js-image-badge');
   const findCommentBadge = () => wrapper.find('.comment-indicator');
   const findFirstBadge = () => findAllNotes().at(0);
   const findSecondBadge = () => findAllNotes().at(1);
 
+  const clickAndDragBadge = (elem, fromPoint, toPoint) => {
+    elem.trigger('mousedown', { clientX: fromPoint.x, clientY: fromPoint.y });
+    return wrapper.vm.$nextTick().then(() => {
+      elem.trigger('mousemove', { clientX: toPoint.x, clientY: toPoint.y });
+      return wrapper.vm.$nextTick();
+    });
+  };
+
   function createComponent(props = {}) {
     wrapper = mount(DesignOverlay, {
       propsData: {
-        dimensions: {
-          width: 100,
-          height: 100,
-        },
+        dimensions: mockDimensions,
         position: {
           top: '0',
           left: '0',
@@ -52,16 +60,24 @@ describe('Design overlay component', () => {
     );
   });
 
-  it('should emit a correct event when clicking on overlay', () => {
+  it('should emit `openCommentForm` when clicking on overlay', () => {
     createComponent();
-    wrapper.find('.image-diff-overlay-add-comment').trigger('click', { offsetX: 10, offsetY: 10 });
+    const newCoordinates = {
+      x: 10,
+      y: 10,
+    };
+    wrapper
+      .find('.image-diff-overlay-add-comment')
+      .trigger('click', { offsetX: newCoordinates.x, offsetY: newCoordinates.y });
 
     return wrapper.vm.$nextTick().then(() => {
-      expect(wrapper.emitted('openCommentForm')).toEqual([[{ x: 10, y: 10 }]]);
+      expect(wrapper.emitted('openCommentForm')).toEqual([
+        [{ x: newCoordinates.x, y: newCoordinates.y }],
+      ]);
     });
   });
 
-  describe('when has notes', () => {
+  describe('with notes', () => {
     beforeEach(() => {
       createComponent({
         notes,
@@ -74,24 +90,8 @@ describe('Design overlay component', () => {
 
     it('should have a correct style for each note badge', () => {
       expect(findFirstBadge().attributes().style).toBe('left: 10px; top: 15px;');
-
       expect(findSecondBadge().attributes().style).toBe('left: 50px; top: 50px;');
     });
-  });
-
-  it('should render a new comment badge when there is a new form', () => {
-    createComponent({
-      currentCommentForm: {
-        height: 100,
-        width: 100,
-        x: 25,
-        y: 25,
-      },
-    });
-
-    expect(findCommentBadge().exists()).toBe(true);
-    expect(findCommentBadge().attributes().style).toBe('left: 25px; top: 25px;');
-  });
 
     it('should recalculate badges positions on window resize', () => {
       createComponent({
@@ -115,4 +115,144 @@ describe('Design overlay component', () => {
         expect(findFirstBadge().attributes().style).toBe('left: 20px; top: 30px;');
       });
     });
+  });
+
+  describe('with a new form', () => {
+    it('should render a new comment badge', () => {
+      createComponent({
+        currentCommentForm: {
+          ...notes[0].position,
+        },
+      });
+
+      expect(findCommentBadge().exists()).toBe(true);
+      expect(findCommentBadge().attributes().style).toBe('left: 10px; top: 15px;');
+    });
+
+    describe('when moving the comment badge', () => {
+      it('should update badge style to reflect new position', () => {
+        const { position } = notes[0];
+
+        createComponent({
+          currentCommentForm: {
+            ...position,
+          },
+        });
+
+        return clickAndDragBadge(
+          findCommentBadge(),
+          { x: position.x, y: position.y },
+          { x: 20, y: 20 },
+        ).then(() => {
+          expect(findCommentBadge().attributes().style).toBe(
+            'left: 20px; top: 20px; cursor: move;',
+          );
+        });
+      });
+
+      it('should update badge style when note-moving action ends', () => {
+        const { position } = notes[0];
+        createComponent({
+          currentCommentForm: {
+            ...position,
+          },
+        });
+
+        const commentBadge = findCommentBadge();
+        const toPoint = { x: 20, y: 20 };
+
+        return clickAndDragBadge(commentBadge, { x: position.x, y: position.y }, toPoint)
+          .then(() => {
+            commentBadge.trigger('mouseup');
+            // simulates the currentCommentForm being updated in index.vue component, and
+            // propagated back down to this prop
+            wrapper.setProps({
+              currentCommentForm: { height: position.height, width: position.width, ...toPoint },
+            });
+            return wrapper.vm.$nextTick();
+          })
+          .then(() => {
+            expect(commentBadge.attributes().style).toBe('left: 20px; top: 20px;');
+          });
+      });
+
+      it.each`
+        element            | getElementFunc      | event
+        ${'overlay'}       | ${findOverlay}      | ${'mouseleave'}
+        ${'comment badge'} | ${findCommentBadge} | ${'mouseup'}
+      `(
+        'should emit `openCommentForm` event when $event fired on $element element',
+        ({ getElementFunc, event }) => {
+          createComponent({
+            notes,
+            currentCommentForm: {
+              ...notes[0].position,
+            },
+          });
+
+          const newCoordinates = { x: 20, y: 20 };
+          wrapper.setData({
+            movingNoteStartPosition: {
+              ...notes[0].position,
+            },
+            movingNoteNewPosition: {
+              ...notes[0].position,
+              ...newCoordinates,
+            },
+          });
+
+          getElementFunc().trigger(event);
+          return wrapper.vm.$nextTick().then(() => {
+            expect(wrapper.emitted('openCommentForm')).toEqual([[newCoordinates]]);
+          });
+        },
+      );
+    });
+  });
+
+  describe('getMovingNotePositionDelta', () => {
+    it('should calculate delta correctly from state', () => {
+      createComponent();
+
+      wrapper.setData({
+        movingNoteStartPosition: {
+          clientX: 10,
+          clientY: 20,
+        },
+      });
+
+      const mockMouseEvent = {
+        clientX: 30,
+        clientY: 10,
+      };
+
+      expect(wrapper.vm.getMovingNotePositionDelta(mockMouseEvent)).toEqual({
+        deltaX: 20,
+        deltaY: -10,
+      });
+    });
+  });
+
+  describe('isPositionInOverlay', () => {
+    createComponent({ dimensions: mockDimensions });
+
+    it.each`
+      test                        | coordinates           | expectedResult
+      ${'within overlay bounds'}  | ${{ x: 50, y: 50 }}   | ${true}
+      ${'outside overlay bounds'} | ${{ x: 101, y: 101 }} | ${false}
+    `('returns [$expectedResult] when position is $test', ({ coordinates, expectedResult }) => {
+      const position = { ...mockDimensions, ...coordinates };
+
+      expect(wrapper.vm.isPositionInOverlay(position)).toBe(expectedResult);
+    });
+  });
+
+  describe('getNoteRelativePosition', () => {
+    it('calculates position correctly', () => {
+      createComponent({ dimensions: mockDimensions });
+      const position = { x: 50, y: 50, width: 200, height: 200 };
+
+      expect(wrapper.vm.getNoteRelativePosition(position)).toEqual({ left: 25, top: 25 });
+    });
+  });
 });

ee/spec/frontend/design_management/components/design_presentation_spec.js

@@ -91,7 +91,7 @@ describe('Design management design presentation component', () => {
       });
     });
 
-    it('currentCommentForm is null when isAnnotating is true but annotation coordinates are falsey', () => {
+    it('currentCommentForm is null when isAnnotating is true but annotation position is falsey', () => {
       createComponent(
         {
           image: 'test.jpg',
@@ -107,7 +107,7 @@ describe('Design management design presentation component', () => {
       });
     });
 
-    it('currentCommentForm is equal to current annotation coordinates when isAnnotating is true', () => {
+    it('currentCommentForm is equal to current annotation position when isAnnotating is true', () => {
       createComponent(
         {
           image: 'test.jpg',
@@ -116,7 +116,7 @@ describe('Design management design presentation component', () => {
         },
         {
           ...mockOverlayData,
-          currentAnnotationCoordinates: {
+          currentAnnotationPosition: {
             x: 1,
             y: 1,
             width: 100,

ee/spec/frontend/design_management/pages/design/__snapshots__/index_spec.js.snap

@@ -23,7 +23,7 @@ exports[`Design management design index page renders design index 1`] = `
     />
      
     <div
-      class="design-scaler-wrapper position-absolute w-100 mb-4 d-flex-center"
+      class="design-scaler-wrapper position-absolute mb-4 d-flex-center"
     >
       <design-scaler-stub />
     </div>
@@ -117,7 +117,7 @@ exports[`Design management design index page with error GlAlert is rendered in c
     />
      
     <div
-      class="design-scaler-wrapper position-absolute w-100 mb-4 d-flex-center"
+      class="design-scaler-wrapper position-absolute mb-4 d-flex-center"
     >
       <design-scaler-stub />
     </div>