Swimlanes - Reorder lists

Feedback

app/assets/javascripts/boards/components/board_list_header.vue

@@ -139,24 +139,28 @@ export default {
       eventHub.$emit(`toggle-issue-form-${this.list.id}`);
     },
     toggleExpanded() {
-      if (this.list.isExpandable) {
-        this.list.isExpanded = !this.list.isExpanded;
+      this.list.isExpanded = !this.list.isExpanded;
 
-        if (AccessorUtilities.isLocalStorageAccessSafe() && !this.isLoggedIn) {
-          localStorage.setItem(`${this.uniqueKey}.expanded`, this.list.isExpanded);
-        }
-
-        if (this.isLoggedIn) {
-          if (this.glFeatures.boardsWithSwimlanes && this.isSwimlanesHeader) {
-            this.updateList({ listId: this.list.id, collapsed: !this.list.isExpanded });
-          } else {
-            this.list.update();
-          }
-        }
+      if (!this.isLoggedIn) {
+        this.addToLocalStorage();
+      } else {
+        this.updateListFunction();
+      }
 
-        // When expanding/collapsing, the tooltip on the caret button sometimes stays open.
-        // Close all tooltips manually to prevent dangling tooltips.
-        this.$root.$emit('bv::hide::tooltip');
+      // When expanding/collapsing, the tooltip on the caret button sometimes stays open.
+      // Close all tooltips manually to prevent dangling tooltips.
+      this.$root.$emit('bv::hide::tooltip');
+    },
+    addToLocalStorage() {
+      if (AccessorUtilities.isLocalStorageAccessSafe()) {
+        localStorage.setItem(`${this.uniqueKey}.expanded`, this.list.isExpanded);
+      }
+    },
+    updateListFunction() {
+      if (this.glFeatures.boardsWithSwimlanes && this.isSwimlanesHeader) {
+        this.updateList({ listId: this.list.id, collapsed: !this.list.isExpanded });
+      } else {
+        this.list.update();
       }
     },
   },

app/assets/javascripts/boards/stores/actions.js

@@ -148,23 +148,25 @@ export default {
     notImplemented();
   },
 
-  moveList: ({ state, commit, dispatch }, { listId, position, moveNewIndexListUp }) => {
+  moveList: ({ state, commit, dispatch }, { listId, newIndex, adjustmentValue }) => {
     const { boardLists } = state;
+    const backupList = [...boardLists];
     const movedList = boardLists.find(({ id }) => id === listId);
-    const listAtNewIndex = boardLists[position + 1];
-    movedList.position = position;
-    listAtNewIndex.position = moveNewIndexListUp
-      ? listAtNewIndex.position + 1
-      : listAtNewIndex.position - 1;
+
+    const newPosition = newIndex - 1;
+    const listAtNewIndex = boardLists[newIndex];
+
+    movedList.position = newPosition;
+    listAtNewIndex.position += adjustmentValue;
     commit(types.MOVE_LIST, {
       movedList,
       listAtNewIndex,
     });
 
-    dispatch('updateList', { listId, position });
+    dispatch('updateList', { listId, position: newPosition, backupList });
   },
 
-  updateList: ({ commit }, { listId, position, collapsed }) => {
+  updateList: ({ commit }, { listId, position, collapsed, backupList }) => {
     gqlClient
       .mutate({
         mutation: updateBoardListMutation,
@@ -176,11 +178,11 @@ export default {
       })
       .then(({ data }) => {
         if (data?.updateBoardList?.errors.length) {
-          commit(types.UPDATE_LIST_FAILURE);
+          commit(types.UPDATE_LIST_FAILURE, backupList);
         }
       })
       .catch(() => {
-        commit(types.UPDATE_LIST_FAILURE);
+        commit(types.UPDATE_LIST_FAILURE, backupList);
       });
   },
 

app/assets/javascripts/boards/stores/mutations.js

@@ -48,16 +48,15 @@ export default {
 
   [mutationTypes.MOVE_LIST]: (state, { movedList, listAtNewIndex }) => {
     const { boardLists } = state;
-    state.boardListsPreviousState = boardLists;
     const movedListIndex = state.boardLists.findIndex(l => l.id === movedList.id);
     Vue.set(boardLists, movedListIndex, movedList);
     Vue.set(boardLists, movedListIndex.position + 1, listAtNewIndex);
-    state.boardLists = sortBy(boardLists, 'position');
+    Vue.set(state, 'boardLists', sortBy(boardLists, 'position'));
   },
 
-  [mutationTypes.UPDATE_LIST_FAILURE]: state => {
-    state.boardLists = state.boardListsPreviousState;
+  [mutationTypes.UPDATE_LIST_FAILURE]: (state, backupList) => {
     state.error = __('An error occurred while updating the list. Please try again.');
+    Vue.set(state, 'boardLists', backupList);
   },
 
   [mutationTypes.REQUEST_REMOVE_LIST]: () => {

app/assets/javascripts/boards/stores/state.js

@@ -9,7 +9,6 @@ export default () => ({
   activeId: inactiveId,
   sidebarType: '',
   boardLists: [],
-  boardListsPreviousState: [],
   issuesByListId: {},
   issues: {},
   isLoadingIssues: false,

app/assets/stylesheets/pages/boards.scss

@@ -116,7 +116,6 @@
 
     .board-title {
       flex-direction: column;
-      height: 100%;
     }
 
     .board-title-caret {

ee/app/assets/javascripts/boards/components/epics_swimlanes.vue

@@ -3,7 +3,7 @@ import { mapActions, mapGetters, mapState } from 'vuex';
 import { GlIcon, GlTooltipDirective } from '@gitlab/ui';
 import Draggable from 'vuedraggable';
 import BoardListHeader from 'ee_else_ce/boards/components/board_list_header.vue';
-import { draggableTag } from '../constants';
+import { DRAGGABLE_TAG } from '../constants';
 import defaultSortableConfig from '~/sortable/sortable_config';
 import { n__ } from '~/locale';
 import EpicLane from './epic_lane.vue';
@@ -57,14 +57,14 @@ export default {
       return n__(`%d unassigned issue`, `%d unassigned issues`, this.unassignedIssuesCount);
     },
     treeRootWrapper() {
-      return this.canAdminList ? Draggable : draggableTag;
+      return this.canAdminList ? Draggable : DRAGGABLE_TAG;
     },
     treeRootOptions() {
       const options = {
         ...defaultSortableConfig,
         fallbackOnBody: false,
         group: 'board-swimlanes',
-        tag: draggableTag,
+        tag: DRAGGABLE_TAG,
         draggable: '.is-draggable',
         'ghost-class': 'swimlane-header-drag-active',
         value: this.lists,
@@ -81,17 +81,11 @@ export default {
     handleDragOnEnd(params) {
       const { newIndex, oldIndex, item } = params;
       const { listId } = item.dataset;
-      const newPosition = newIndex - 1;
-      let moveNewIndexListUp = false;
-
-      if (newIndex < oldIndex) {
-        moveNewIndexListUp = true;
-      }
 
       this.moveList({
         listId,
-        position: newPosition,
-        moveNewIndexListUp,
+        newIndex,
+        adjustmentValue: newIndex < oldIndex ? 1 : -1,
       });
     },
   },
@@ -107,6 +101,7 @@ export default {
       :is="treeRootWrapper"
       v-bind="treeRootOptions"
       class="board-swimlanes-headers gl-display-table gl-sticky gl-pt-5 gl-bg-white gl-top-0 gl-z-index-3"
+      data-testid="board-swimlanes-headers"
       @end="handleDragOnEnd"
     >
       <div

ee/app/assets/javascripts/boards/constants.js

-export const draggableTag = 'div';
+export const DRAGGABLE_TAG = 'div';
 
 export default {
-  draggableTag,
+  DRAGGABLE_TAG,
 };

ee/spec/frontend/boards/components/epics_swimlanes_spec.js

@@ -6,7 +6,6 @@ import BoardListHeader from 'ee_else_ce/boards/components/board_list_header.vue'
 import EpicLane from 'ee/boards/components/epic_lane.vue';
 import IssueLaneList from 'ee/boards/components/issues_lane_list.vue';
 import getters from 'ee/boards/stores/getters';
-import { draggableTag } from 'ee/boards/constants';
 import { GlIcon } from '@gitlab/ui';
 import { mockListsWithModel, mockEpics, mockIssuesByListId, issues } from '../mock_data';
 
@@ -52,44 +51,25 @@ describe('EpicsSwimlanes', () => {
   });
 
   describe('computed', () => {
-    beforeEach(() => {
-      createComponent();
-    });
-
     describe('treeRootWrapper', () => {
-      it('should return Draggable reference when canAdminList prop is true', () => {
-        wrapper.setProps({ canAdminList: true });
-
-        expect(wrapper.vm.treeRootWrapper).toBe(Draggable);
+      describe('when canAdminList prop is true', () => {
+        beforeEach(() => {
+          createComponent({ canAdminList: true });
+        });
+
+        it('should return Draggable reference when canAdminList prop is true', () => {
+          expect(wrapper.find(Draggable).exists()).toBe(true);
+        });
       });
 
-      it('should return string "div" when canAdminList prop is false', () => {
-        expect(wrapper.vm.treeRootWrapper).toBe(draggableTag);
-      });
-    });
-
-    describe('treeRootOptions', () => {
-      it('should return object containing Vue.Draggable config extended from `defaultSortableConfig` when canAdminList prop is true', () => {
-        wrapper.setProps({ canAdminList: true });
-
-        expect(wrapper.vm.treeRootOptions).toEqual(
-          expect.objectContaining({
-            animation: 200,
-            forceFallback: true,
-            fallbackClass: 'is-dragging',
-            fallbackOnBody: false,
-            ghostClass: 'is-ghost',
-            group: 'board-swimlanes',
-            tag: draggableTag,
-            draggable: '.is-draggable',
-            'ghost-class': 'swimlane-header-drag-active',
-            value: mockListsWithModel,
-          }),
-        );
-      });
+      describe('when canAdminList prop is false', () => {
+        beforeEach(() => {
+          createComponent();
+        });
 
-      it('should return an empty object when canAdminList prop is false', () => {
-        expect(wrapper.vm.treeRootOptions).toEqual(expect.objectContaining({}));
+        it('should not return Draggable reference when canAdminList prop is false', () => {
+          expect(wrapper.find(Draggable).exists()).toBe(false);
+        });
       });
     });
   });

spec/frontend/boards/stores/actions_spec.js

@@ -172,7 +172,7 @@ describe('moveList', () => {
 
     testAction(
       actions.moveList,
-      { listId: 'gid://gitlab/List/1', position: 0, moveNewIndexListUp: true },
+      { listId: 'gid://gitlab/List/1', newIndex: 1, adjustmentValue: 1 },
       state,
       [
         {
@@ -180,7 +180,12 @@ describe('moveList', () => {
           payload: { movedList: mockListsWithModel[0], listAtNewIndex: mockListsWithModel[1] },
         },
       ],
-      [{ type: 'updateList', payload: { listId: 'gid://gitlab/List/1', position: 0 } }],
+      [
+        {
+          type: 'updateList',
+          payload: { listId: 'gid://gitlab/List/1', position: 0, backupList: mockListsWithModel },
+        },
+      ],
       done,
     );
   });

spec/frontend/boards/stores/mutations_spec.js

@@ -93,7 +93,7 @@ describe('Board Store Mutations', () => {
   });
 
   describe('MOVE_LIST', () => {
-    it('updates boardLists state with reordered lists and saves previous order', () => {
+    it('updates boardLists state with reordered lists', () => {
       state = {
         ...state,
         boardLists: mockListsWithModel,
@@ -105,7 +105,6 @@ describe('Board Store Mutations', () => {
       });
 
       expect(state.boardLists).toEqual([mockListsWithModel[1], mockListsWithModel[0]]);
-      expect(state.boardListsPreviousState).toEqual(mockListsWithModel);
     });
   });
 
@@ -114,11 +113,10 @@ describe('Board Store Mutations', () => {
       state = {
         ...state,
         boardLists: [mockListsWithModel[1], mockListsWithModel[0]],
-        boardListsPreviousState: mockListsWithModel,
         error: undefined,
       };
 
-      mutations.UPDATE_LIST_FAILURE(state);
+      mutations.UPDATE_LIST_FAILURE(state, mockListsWithModel);
 
       expect(state.boardLists).toEqual(mockListsWithModel);
       expect(state.error).toEqual('An error occurred while updating the list. Please try again.');