Repository: ProcessMaker/modeler (js)

Issue:
Extract intermediateMailIcon mixin into function
A mixin, `src/mixins/intermediateMailIcon.vue`, defines properties of the shape on the Vue component. To improve testing and readability, this mixin can instead be turned into a function.

The problem with mixins is that they introduce side effects that are hard to work with. This mixin specifically modifies the component state (changes properties on `this`), but it's not obvious from the component. And if more mixins are added in the future, the state they modify and introduce may interfere each other, and tracking down the mixin which introduced the problem could be difficult.

The other issue with this mixin is that in one of the places where it's included, `intermediateMessageCatchEvent.vue`, the properties that are added by the mixin are modified in the component. These properties of the mixin should instead be arguments to a function.

Pull request description:
Change mixin to function
Fixes #985.

Patch:
diff --git a/src/ModelerApp.vue b/src/ModelerApp.vue
index 5ea6b791..c0b8cd0c 100644
--- a/src/ModelerApp.vue
+++ b/src/ModelerApp.vue
@@ -1,7 +1,7 @@
 <template>
   <b-container id="modeler-app" class="h-100 container position-relative p-0">
 
-    <b-card no-body class="h-100">
+    <b-card no-body class="h-100 rounded-0">
       <b-card-header class="d-flex align-items-center header">
         <b-card-text class="m-0 font-weight-bolder">
           {{ $t('ProcessMaker Modeler') }}
diff --git a/src/components/controls/controls.vue b/src/components/controls/controls.vue
index e99b79ca..30233b38 100644
--- a/src/components/controls/controls.vue
+++ b/src/components/controls/controls.vue
@@ -4,7 +4,9 @@
     :class="[{ 'ignore-pointer': canvasDragPosition, 'controls-column-compressed' : compressed }]"
     data-test="controls-column"
   >
-    <b-card no-body class="controls" :style="{ height: parentHeight }">
+    <b-card no-body class="controls rounded-0 border-top-0 border-bottom-0 border-left-0"
+      :style="{ height: parentHeight }"
+    >
       <b-input-group size="sm" v-show="!compressed">
         <b-input-group-prepend>
           <span class="input-group-text border-left-0 border-top-0 rounded-0"><i class="fas fa-filter" /></span>
diff --git a/src/components/modeler/Modeler.vue b/src/components/modeler/Modeler.vue
index 0aa62206..2c053161 100644
--- a/src/components/modeler/Modeler.vue
+++ b/src/components/modeler/Modeler.vue
@@ -122,6 +122,7 @@ import initAnchor from '@/mixins/linkManager.js';
 import { addIdToNodeAndSetUpDiagramReference, addNodeToProcess, getTargetProcess, isBoundaryEvent } from '@/components/nodeManager';
 import ensureShapeIsNotCovered from '@/components/shapeStackUtils';
 import ToolBar from '@/components/toolbar/ToolBar';
+import moveShapeByKeypress from '@/components/modeler/moveWithArrowKeys';
 
 const version = '1.0';
 
@@ -690,6 +691,14 @@ export default {
     this.linter = new Linter(linterConfig);
   },
   mounted() {
+    document.addEventListener('keydown', event => {
+      moveShapeByKeypress(
+        event.key,
+        store.getters.highlightedShape,
+        this.pushToUndoStack,
+      );
+    });
+
     this.graph = new dia.Graph();
     store.commit('setGraph', this.graph);
     this.graph.set('interactiveFunc', cellView => {
diff --git a/src/components/modeler/moveWithArrowKeys.js b/src/components/modeler/moveWithArrowKeys.js
new file mode 100644
index 00000000..1b656dee
--- /dev/null
+++ b/src/components/modeler/moveWithArrowKeys.js
@@ -0,0 +1,49 @@
+import PaperManager from '@/components/paperManager';
+
+export const moveAmount = PaperManager.gridSize / 2;
+
+const translationAmount = new Map();
+translationAmount.set('Up', [0, -moveAmount]);
+translationAmount.set('Down', [0, moveAmount]);
+translationAmount.set('Left', [-moveAmount, 0]);
+translationAmount.set('Right', [moveAmount, 0]);
+
+const invalidTypes = [
+  'PoolLane',
+  'processmaker.components.nodes.boundaryEvent.Shape',
+];
+
+export default function moveShapeByKeypress(key, shape, onAfterMove = () => {}) {
+  if (!shape || invalidTypes.includes(shape.get('type'))) {
+    return;
+  }
+
+  const match = key.match(/^(?:Arrow)?(Up|Down|Left|Right)$/);
+  const keyCode = match && match[1];
+
+  if (!keyCode) {
+    return;
+  }
+
+  const [tx, ty] = translationAmount.get(keyCode) || [0, 0];
+  shape.translate(tx, ty, { movedWithArrowKeys: true });
+
+  expandPoolToContainElement(shape);
+
+  onAfterMove();
+}
+
+function expandPoolToContainElement(shape) {
+  const pool = shape.getParentCell();
+  if (!pool || pool.get('type') !== 'processmaker.modeler.bpmn.pool') {
+    return;
+  }
+
+  pool.component.expandToFitElement(shape, pool);
+
+  if (!pool.component.laneSet) {
+    return;
+  }
+
+  pool.component.updateLaneChildren();
+}
diff --git a/src/components/nodes/boundaryMessageEvent/boundaryMessageEvent.vue b/src/components/nodes/boundaryMessageEvent/boundaryMessageEvent.vue
index 35eca756..9b1e358e 100644
--- a/src/components/nodes/boundaryMessageEvent/boundaryMessageEvent.vue
+++ b/src/components/nodes/boundaryMessageEvent/boundaryMessageEvent.vue
@@ -1,15 +1,18 @@
 <script>
 import BoundaryEvent from '@/components/nodes/boundaryEvent/boundaryEvent';
-import intermediateMailIcon from '@/mixins/intermediateMailIcon';
+import getIntermediateMailIconShapeAttributes from '@/mixins/intermediateMailIcon';
 
 export default {
   extends: BoundaryEvent,
-  mixins: [intermediateMailIcon],
   methods: {
     isValidBoundaryEventTarget(model) {
       const component = model.component;
       return component && component.node.definition.$type === 'bpmn:CallActivity';
     },
   },
+  mounted() {
+    const shapeAttributes = getIntermediateMailIconShapeAttributes();
+    this.shape.attr(shapeAttributes);
+  },
 };
 </script>
diff --git a/src/components/nodes/intermediateMessageCatchEvent/intermediateMessageCatchEvent.vue b/src/components/nodes/intermediateMessageCatchEvent/intermediateMessageCatchEvent.vue
index faa92941..94ee2488 100644
--- a/src/components/nodes/intermediateMessageCatchEvent/intermediateMessageCatchEvent.vue
+++ b/src/components/nodes/intermediateMessageCatchEvent/intermediateMessageCatchEvent.vue
@@ -1,23 +1,12 @@
 <script>
 import IntermediateEvent from '@/components/nodes/intermediateEvent/intermediateEvent';
-import intermediateMailIcon from '@/mixins/intermediateMailIcon';
+import getIntermediateMailIconShapeAttributes from '@/mixins/intermediateMailIcon';
 
 export default {
   extends: IntermediateEvent,
-  mixins: [intermediateMailIcon],
   mounted() {
-    this.shape.attr({
-      body: {
-        stroke: '#FBBE02',
-        fill: '#FFF4D1',
-        strokeWidth: 2,
-      },
-      body2: {
-        stroke: '#FBBE02',
-        fill: '#FFF4D1',
-        strokeWidth: 2,
-      },
-    });
+    const shapeAttributes = getIntermediateMailIconShapeAttributes('#fbbe02', 2, '#fff4d1');
+    this.shape.attr(shapeAttributes);
   },
 };
 </script>
diff --git a/src/components/statusBar/Statusbar.vue b/src/components/statusBar/Statusbar.vue
index b4929356..ca17622f 100644
--- a/src/components/statusBar/Statusbar.vue
+++ b/src/components/statusBar/Statusbar.vue
@@ -1,10 +1,10 @@
 <template>
-  <div class="statusbar d-flex align-items-center justify-content-between pr-3 border border-bottom-0">
+  <div class="statusbar d-flex align-items-center justify-content-between pr-3 border-top">
     <div>
-      <slot name="secondary"/>
+      <slot name="secondary" />
     </div>
     <div>
-      <slot/>
+      <slot />
     </div>
   </div>
 </template>
diff --git a/src/mixins/hideLabelOnDrag.js b/src/mixins/hideLabelOnDrag.js
index 366c1748..d8807d66 100644
--- a/src/mixins/hideLabelOnDrag.js
+++ b/src/mixins/hideLabelOnDrag.js
@@ -1,8 +1,8 @@
 export default {
   props: ['highlighted'],
   methods: {
-    hideLabel() {
-      if (!this.highlighted) {
+    hideLabel(element, newPosition, options) {
+      if (!this.highlighted || options.movedWithArrowKeys) {
         return;
       }
 
diff --git a/src/mixins/intermediateMailIcon.js b/src/mixins/intermediateMailIcon.js
new file mode 100644
index 00000000..7d625142
--- /dev/null
+++ b/src/mixins/intermediateMailIcon.js
@@ -0,0 +1,23 @@
+import intermediateMailSymbol from '@/assets/intermediate-mail.svg';
+
+export default function getIntermediateMailIconShapeAttributes(stroke = '#000', strokeWidth = 1, fill = '#fff') {
+  return {
+    image: {
+      xlinkHref: intermediateMailSymbol,
+      width: 20,
+      height: 20,
+      y: 3,
+      x: 3,
+    },
+    body: {
+      stroke,
+      strokeWidth,
+      fill,
+    },
+    body2: {
+      stroke,
+      strokeWidth,
+      fill,
+    },
+  };
+}
diff --git a/src/mixins/intermediateMailIcon.vue b/src/mixins/intermediateMailIcon.vue
deleted file mode 100644
index 02a5fd0a..00000000
--- a/src/mixins/intermediateMailIcon.vue
+++ /dev/null
@@ -1,27 +0,0 @@
-<script>
-import intermediateMailSymbol from '@/assets/intermediate-mail.svg';
-
-export default {
-  mounted() {
-    this.shape.attr('image/xlink:href', intermediateMailSymbol);
-    this.shape.attr({
-      body: {
-        stroke: '#000',
-        strokeWidth: 1,
-        fill: '#fff',
-      },
-      body2: {
-        stroke: '#000',
-        strokeWidth: 1,
-        fill: '#fff',
-      },
-      image: {
-        width: 20,
-        height: 20,
-        y: 3,
-        x: 3,
-      },
-    });
-  },
-};
-</script>
\ No newline at end of file
diff --git a/src/store.js b/src/store.js
index a6c4a2d8..d2ecf212 100644
--- a/src/store.js
+++ b/src/store.js
@@ -29,6 +29,7 @@ export default new Vuex.Store({
     nodeShape: state => node => {
       return state.graph.getCells().find(cell => cell.component && cell.component.node === node);
     },
+    highlightedShape: (state, getters) => getters.nodeShape(getters.highlightedNode),
     rootElements: state => state.rootElements,
     autoValidate: state => state.autoValidate,
     globalProcesses: state => state.globalProcesses,


Test patch:
diff --git a/tests/unit/moveWithArrowKeys.spec.js b/tests/unit/moveWithArrowKeys.spec.js
new file mode 100644
index 00000000..55b3c0b6
--- /dev/null
+++ b/tests/unit/moveWithArrowKeys.spec.js
@@ -0,0 +1,51 @@
+import moveShapeByKeypress, { moveAmount } from '@/components/modeler/moveWithArrowKeys';
+
+const shapeFactory = (type = 'foo') => ({
+  translate: jest.fn(),
+  get: jest.fn(() => type),
+  getParentCell: () => null,
+});
+
+describe('moveWithArrowKeys', () => {
+  it('should not translate shape if correct keys are not pressed', () => {
+    const shape = shapeFactory();
+    const onAfterMove = jest.fn();
+    moveShapeByKeypress('Enter', shape);
+
+    expect(shape.translate).not.toHaveBeenCalled();
+    expect(onAfterMove).not.toHaveBeenCalled();
+  });
+
+  it.each`
+  keyPress | expected
+  ${'Up'} | ${[0, -moveAmount]}
+  ${'Down'} | ${[0, moveAmount]}
+  ${'Left'} | ${[-moveAmount, 0]}
+  ${'Right'} | ${[moveAmount, 0]}
+  ${'ArrowUp'} | ${[0, -moveAmount]}
+  ${'ArrowDown'} | ${[0, moveAmount]}
+  ${'ArrowLeft'} | ${[-moveAmount, 0]}
+  ${'ArrowRight'} | ${[moveAmount, 0]}
+  `('should move shape $keyPress direction when the corresponding key is pressed',
+  ({ keyPress, expected }) => {
+    const shape = shapeFactory();
+    const onAfterMove = jest.fn();
+    moveShapeByKeypress(keyPress, shape, onAfterMove);
+
+    expect(shape.translate).toHaveBeenCalledWith(...expected, { movedWithArrowKeys: true });
+    expect(onAfterMove).toHaveBeenCalledTimes(1);
+  });
+
+  it.each([
+    ['PoolLane'],
+    ['processmaker.components.nodes.boundaryEvent.Shape'],
+  ],
+  )('Does not attempt to move an invalid shape', (invalidShapeType) => {
+    const shape = shapeFactory(invalidShapeType);
+    const onAfterMove = jest.fn();
+    moveShapeByKeypress('Up', shape, onAfterMove);
+
+    expect(shape.translate).not.toHaveBeenCalled();
+    expect(onAfterMove).not.toHaveBeenCalled();
+  });
+});