Vmo 3944 allow moving multiple blocks #101

safydy-r · 2021-06-15T02:13:31Z

Followed the last tech arch from miro (by Brett & Safidy) : https://miro.com/app/board/o9J_lXulmu4=/?moveToWidget=3074457360012269222&cot=14

safydy-r · 2021-06-15T02:16:01Z

src/store/builder/index.ts

    operations[operation.kind] = operation
  },

-  setBlockPositionTo(state, { position: { x, y }, block }) {


moved it as actions

Hi @safydy, can we try to separate our logic from our mutations? From what I understand, this mutation should remain in tact, and be dispatched to from the other action when we want to assign values; they can be named differently if that helps with readability, but the vuex state should be modified within mutations, and logic around those can be done in actions.

oh ... I see, thanks for requesting this. I think this makes sense because it would be dangerous to re-assign prop in actions. Even ESLint is saying Assignment to property of function parameter 'block'. (no-param-reassign)
Making a change here ... <3

bzabos

Hi @safydy ! I've left a couple notes on separation of concerns and hopefully reduce a bit of complexity. I'd love a demo sometime, let me know if you have any Qs. :)

bzabos · 2021-06-21T14:53:24Z

src/store/builder/index.ts

    operations[operation.kind] = operation
  },

-  setBlockPositionTo(state, { position: { x, y }, block }) {


Hi @safydy, can we try to separate our logic from our mutations? From what I understand, this mutation should remain in tact, and be dispatched to from the other action when we want to assign values; they can be named differently if that helps with readability, but the vuex state should be modified within mutations, and logic around those can be done in actions.

bzabos · 2021-06-21T14:55:41Z

src/store/builder/index.ts

+    if (!includes(rootState.flow.selectedBlockUuids, block.uuid)) {
+      block.vendor_metadata.io_viamo.uiData.xPosition = x
+      block.vendor_metadata.io_viamo.uiData.yPosition = y
+    } else { // the block is selected


Should the if() {} above have a return statement after block.vendor_metadata.io_viamo.uiData.yPosition = y ? It seems like it's a special case that we can bail after to avoid having to take it into account later in this function.

You're right, let's make an early return

bzabos · 2021-06-21T14:56:43Z

src/store/builder/index.ts

+      }
+
+      let shouldReversePosition = false;
+      if (translationDelta.x > 0 /* moving to the right */ ||


👌 I agree with this eslint rule, I got burned trying to understand the second line of this if( as a statement within the if!

bzabos · 2021-06-21T15:01:30Z

src/store/builder/index.ts

+
+      let shouldReversePosition = false;
+      if (translationDelta.x > 0 /* moving to the right */ ||
+        rootGetters['flow/selectedBlockAtTheFurthestLeftPosition'].vendor_metadata.io_viamo.uiData.xPosition > 0 /* left space still available */) {


Hi @safydy , thanks for putting the comments in, super helpful! One step further, can we extract some of these wordy lines into self-explaining variable names and then not need comments? The should help when reading the logic portions as English. eg.

const leftMostBlock = rootGetters['flow/selectedBlockAtTheFurthestLeftPosition'] const isMovingRight = translationDelta.x > 0 const hasMoreLeftSpaceAvailable = leftMostBlock.vendor_metadata.io_viamo.uiData.xPosition > 0 if (isMovingRight || hasMoreLeftSpaceAvailable) { ...

Since the new getters like selectedBlockAtTheFurthestLeftPosition are only used here, can we move them into this file as a modue-level functions that we call directly? This would mitigate iterating every block on the canvas on every change if we're only ever using here :)

Thanks for recommending self-explaining variable instead of comments, this way the code is easier to read

Agree with selectedBlockAtTheFurthestLeftPosition concern

bzabos · 2021-06-21T15:07:46Z

src/store/builder/index.ts

+      const otherSelectedBlocks = filter(rootGetters['flow/selectedBlocks'], currentBlock => currentBlock.uuid !== block.uuid);
+      forEach(otherSelectedBlocks, async (currentBlock: IBlock) => {
+        let newPosition = {} as IPosition
+        await dispatch('setBlockPositionFromDelta', { delta: translationDelta, block: currentBlock }).then(


Hi @safydy-r , a couple things on this one: I am feeling confused by the need to manage block and draggable separately. Can we wrap these two mutations into a single action (eg. setBlockAndSyncDraggablePositionTo({pos}) or setBlockAndSyncDraggablePositionFromDelta({delta})) then just call that everywhere? I think we want them to be the same in most circumstances, right?

Good remark, I think setBlockAndSyncDraggablePositionFromDelta would be perfect. Changing ...

bzabos · 2021-06-21T15:28:46Z

src/store/builder/index.ts

+      }
+
+      if(translationDelta.y > 0 /* moving to the top */ ||
+        rootGetters['flow/selectedBlockAtTheTopPosition'].vendor_metadata.io_viamo.uiData.yPosition - state.toolbarHeight > 0 /* top space still available */) {


This might get hard to maintain as the UI becomes more fancy. It feels fragile to try to match css and logic. Is there a way we can remove the need to track anything to do with toolbar height? My first thought is to use CSS to manage margins:

------------------------ Toolbar ------------------------ Alerts Toolbar |----------------------| Canvas |----------------------| |----------------------| |----------------------|

Using the same logic as we already use to show/hide alerts-toolbar, we can (create, then) apply .has-notification-alerts-banner class to canvas which can add a margin-top of whatever the magic number is. This mitigates mixing javascript and css, keeps pixel variations within css, and avoids having those magic numbers sprinkled throughout our formulas and vuex state.

If we use margins instead of padding, then the top of what we can see on the canvas becomes the true top of the canvas and the formulas needn't be concerned.

What do you think about this?

I do agree this toolbarHeight might introduce new complexity to the code. I will check what can we do with margin as you suggested

… errors

…ocks-alternative-2

safydy-r · 2021-06-21T21:58:07Z

src/store/builder/index.ts

+  },
+
+  isAnyTopSpaceAvailable(state, getters) {
+    return getters.selectedBlockAtTheTopPosition.vendor_metadata.io_viamo.uiData.yPosition - state.toolbarHeight > 0


@bzabos , your comment about separating pixel computation is still relevant here.
I introduced this toolbarHeight because we would like to know in JS side if there is no more space between the topBlock and the toolbar, and we know the toolbar has a dynamic height.

I tried your proposal

Using the same logic as we already use to show/hide alerts-toolbar, we can (create, then) apply .has-notification-alerts-banner class to canvas which can add a margin-top of whatever the magic number is. This mitigates mixing javascript and css, keeps pixel variations within css, and avoids having those magic numbers sprinkled throughout our formulas and vuex state.

... but I cannot resolve the challenge of removing this formula getters.selectedBlockAtTheTopPosition.vendor_metadata.io_viamo.uiData.yPosition - state.toolbarHeight > 0 from JS. Any idea ?

…ocks-alternative-2 # Conflicts: # src/components/interaction-designer/Block.vue # src/components/interaction-designer/toolbar/ErrorNotifications.vue # src/components/interaction-designer/toolbar/SelectionBanner.vue # src/components/interaction-designer/toolbar/TreeBuilderToolbar.vue # src/store/builder/index.ts # src/store/flow/block.ts # src/store/flow/flow.ts # src/store/flow/index.ts # src/views/InteractionDesigner.vue

github-actions · 2021-07-13T23:47:36Z

Found 16 violations:

Reporter: ESLint
Rule: eslint.rules.vue/order-in-components
Severity: WARN
File: src/components/interaction-designer/Block.vue L260

The "computed" property should be above the "watch" property on line 242. (vue/order-in-components)

Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/restrict-plus-operands
Severity: ERROR
File: src/store/builder/index.ts L248

Operands of '+' operation must either be both strings or both numbers. (@typescript-eslint/restrict-plus-operands)

Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/restrict-plus-operands
Severity: ERROR
File: src/store/builder/index.ts L247

Operands of '+' operation must either be both strings or both numbers. (@typescript-eslint/restrict-plus-operands)

Reporter: ESLint
Rule: eslint.rules.lodash/prefer-filter
Severity: ERROR
File: src/store/builder/index.ts L227

Prefer _.filter or _.some over an if statement inside a _.forEach (lodash/prefer-filter)

Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/strict-boolean-expressions
Severity: ERROR
File: src/store/builder/index.ts L207

Unexpected any value in conditional. An explicit comparison or type cast is required. (@typescript-eslint/strict-boolean-expressions)

Reporter: ESLint
Rule: eslint.rules.@typescript-eslint/strict-boolean-expressions
Severity: ERROR
File: src/store/builder/index.ts L198

Unexpected any value in conditional. An explicit comparison or type cast is required. (@typescript-eslint/strict-boolean-expressions)

Reporter: ESLint
Rule: eslint.rules.lodash/prop-shorthand
Severity: WARN
File: src/store/builder/index.ts L108