Merge branch 'kp-fix-vue-labels-dropdown-bugs' into 'master'

Improve label selection performance in Vue Labels dropdown

See merge request gitlab-org/gitlab!31327

app/assets/javascripts/vue_shared/components/sidebar/labels_select_vue/dropdown_button.vue

@@ -27,9 +27,9 @@ export default {
     class="labels-select-dropdown-button js-dropdown-button w-100 text-left"
     @click="handleButtonClick"
   >
-    <span class="dropdown-toggle-text" :class="{ 'flex-fill': isDropdownVariantStandalone }">{{
-      dropdownButtonText
-    }}</span>
+    <span class="dropdown-toggle-text flex-fill">
+      {{ dropdownButtonText }}
+    </span>
     <gl-icon name="chevron-down" class="pull-right" />
   </gl-button>
 </template>

app/assets/javascripts/vue_shared/components/sidebar/labels_select_vue/dropdown_contents_labels_view.vue

 <script>
 import { mapState, mapGetters, mapActions } from 'vuex';
-import { GlLoadingIcon, GlButton, GlIcon, GlSearchBoxByType, GlLink } from '@gitlab/ui';
+import { GlLoadingIcon, GlButton, GlSearchBoxByType, GlLink } from '@gitlab/ui';
 
 import { UP_KEY_CODE, DOWN_KEY_CODE, ENTER_KEY_CODE, ESC_KEY_CODE } from '~/lib/utils/keycodes';
 
+import LabelItem from './label_item.vue';
+
 export default {
   components: {
     GlLoadingIcon,
     GlButton,
-    GlIcon,
     GlSearchBoxByType,
     GlLink,
+    LabelItem,
   },
   data() {
     return {
@@ -60,11 +62,6 @@ export default {
       'updateSelectedLabels',
       'toggleDropdownContents',
     ]),
-    getDropdownLabelBoxStyle(label) {
-      return {
-        backgroundColor: label.color,
-      };
-    },
     isLabelSelected(label) {
       return this.selectedLabelsList.includes(label.id);
     },
@@ -138,24 +135,19 @@ export default {
         @click="toggleDropdownContents"
       />
     </div>
-    <div class="dropdown-input">
+    <div class="dropdown-input" @click.stop="() => {}">
       <gl-search-box-by-type v-model="searchKey" :autofocus="true" />
     </div>
-    <div v-if="!labelsFetchInProgress" ref="labelsListContainer" class="dropdown-content">
+    <div v-show="!labelsFetchInProgress" ref="labelsListContainer" class="dropdown-content">
       <ul class="list-unstyled mb-0">
         <li v-for="(label, index) in visibleLabels" :key="label.id" class="d-block text-left">
-          <gl-link
-            class="d-flex align-items-baseline text-break-word label-item"
-            :class="{ 'is-focused': index === currentHighlightItem }"
-            @click="handleLabelClick(label)"
-          >
-            <gl-icon v-show="label.set" name="mobile-issue-close" class="mr-2 align-self-center" />
-            <span v-show="!label.set" class="mr-3 pr-2"></span>
-            <span class="dropdown-label-box" :style="getDropdownLabelBoxStyle(label)"></span>
-            <span>{{ label.title }}</span>
-          </gl-link>
+          <label-item
+            :label="label"
+            :highlight="index === currentHighlightItem"
+            @clickLabel="handleLabelClick(label)"
+          />
         </li>
-        <li v-if="!visibleLabels.length" class="p-2 text-center">
+        <li v-show="!visibleLabels.length" class="p-2 text-center">
           {{ __('No matching results') }}
         </li>
       </ul>
@@ -170,9 +162,9 @@ export default {
           >
         </li>
         <li>
-          <gl-link :href="labelsManagePath" class="d-flex flex-row text-break-word label-item">
-            {{ footerManageLabelTitle }}
-          </gl-link>
+          <gl-link :href="labelsManagePath" class="d-flex flex-row text-break-word label-item">{{
+            footerManageLabelTitle
+          }}</gl-link>
         </li>
       </ul>
     </div>

app/assets/javascripts/vue_shared/components/sidebar/labels_select_vue/label_item.vue

+<script>
+import { GlIcon, GlLink } from '@gitlab/ui';
+
+export default {
+  components: {
+    GlIcon,
+    GlLink,
+  },
+  props: {
+    label: {
+      type: Object,
+      required: true,
+    },
+    highlight: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  data() {
+    return {
+      isSet: this.label.set,
+    };
+  },
+  computed: {
+    labelBoxStyle() {
+      return {
+        backgroundColor: this.label.color,
+      };
+    },
+  },
+  methods: {
+    handleClick() {
+      this.isSet = !this.isSet;
+      this.$emit('clickLabel', this.label);
+    },
+  },
+};
+</script>
+
+<template>
+  <gl-link
+    class="d-flex align-items-baseline text-break-word label-item"
+    :class="{ 'is-focused': highlight }"
+    @click="handleClick"
+  >
+    <gl-icon v-show="isSet" name="mobile-issue-close" class="mr-2 align-self-center" />
+    <span v-show="!isSet" data-testid="no-icon" class="mr-3 pr-2"></span>
+    <span class="dropdown-label-box" data-testid="label-color-box" :style="labelBoxStyle"></span>
+    <span>{{ label.title }}</span>
+  </gl-link>
+</template>

app/assets/javascripts/vue_shared/components/sidebar/labels_select_vue/store/mutations.js

@@ -58,29 +58,13 @@ export default {
   },
 
   [types.UPDATE_SELECTED_LABELS](state, { labels }) {
-    // Iterate over all the labels and update
-    // `set` prop value to represent their current state.
-    const labelIds = labels.map(label => label.id);
-    state.labels = state.labels.reduce((allLabels, label) => {
-      if (labelIds.includes(label.id)) {
-        allLabels.push({
-          ...label,
-          touched: true,
-          set: !label.set,
-        });
-      } else {
-        // In case multiselect is not allowed
-        // we unselect any existing selected label
-        const unchangedLabel = state.allowMultiselect
-          ? label
-          : {
-              ...label,
-              touched: true,
-              set: false,
-            };
-        allLabels.push(unchangedLabel);
+    // Find the label to update from all the labels
+    // and change `set` prop value to represent their current state.
+    const labelId = labels.pop()?.id;
+    const candidateLabel = state.labels.find(label => labelId === label.id);
+    if (candidateLabel) {
+      candidateLabel.touched = true;
+      candidateLabel.set = !candidateLabel.set;
     }
-      return allLabels;
-    }, []);
   },
 };

ee/app/assets/javascripts/epic/components/sidebar_items/sidebar_labels.vue

@@ -101,7 +101,22 @@ export default {
       }
     },
     handleUpdateSelectedLabels(labels) {
-      this.updateEpicLabels(labels);
+      // Iterate over selection and check if labels which were
+      // either selected or removed aren't leading to same selection
+      // as current one, as then we don't want to make network call
+      // since nothing has changed.
+      const anyLabelUpdated = labels.some(label => {
+        // Find this label in existing selection.
+        const existingLabel = this.epicContext.labels.find(l => l.id === label.id);
+
+        // Check either of the two following conditions;
+        // 1. A label that's not currently applied is being applied.
+        // 2. A label that's already applied is being removed.
+        return (!existingLabel && label.set) || (existingLabel && !label.set);
+      });
+
+      // Only proceed with action if there are any label updates to be done.
+      if (anyLabelUpdated) this.updateEpicLabels(labels);
     },
   },
 };

ee/spec/frontend/epic/components/sidebar_items/sidebar_labels_spec.js

@@ -117,6 +117,62 @@ describe('SidebarLabelsComponent', () => {
         expect(wrapper.vm.epicContext.labels[0].id).toBe(mockLabels[0].id);
       });
     });
+
+    describe('handleUpdateSelectedLabels', () => {
+      const updatingLabel = {
+        id: 1,
+        title: 'Foo Label',
+        description: 'Foobar',
+        color: '#BADA55',
+        text_color: '#FFFFFF',
+      };
+
+      beforeEach(() => {
+        jest.spyOn(wrapper.vm, 'updateEpicLabels').mockImplementation();
+      });
+
+      it('calls action `updateEpicLabels` when there is a label to apply', () => {
+        store.state.labels = mockLabels;
+        const appliedLabel = {
+          ...updatingLabel,
+          set: true,
+        };
+
+        wrapper.vm.handleUpdateSelectedLabels([appliedLabel]);
+
+        expect(wrapper.vm.updateEpicLabels).toHaveBeenCalledWith(
+          expect.arrayContaining([appliedLabel]),
+        );
+      });
+
+      it('calls action `updateEpicLabels` when there is a label to remove', () => {
+        const removedLabel = {
+          ...updatingLabel,
+          set: false,
+        };
+
+        store.state.labels = [...mockLabels, removedLabel];
+
+        wrapper.vm.handleUpdateSelectedLabels([removedLabel]);
+
+        expect(wrapper.vm.updateEpicLabels).toHaveBeenCalledWith(
+          expect.arrayContaining([removedLabel]),
+        );
+      });
+
+      it('does not call action `updateEpicLabels` when there are no labels to apply or remove', () => {
+        const appliedLabel = {
+          ...updatingLabel,
+          set: true,
+        };
+
+        store.state.labels = [...mockLabels, appliedLabel];
+
+        wrapper.vm.handleUpdateSelectedLabels([appliedLabel]);
+
+        expect(wrapper.vm.updateEpicLabels).not.toHaveBeenCalled();
+      });
+    });
   });
 
   describe('template', () => {

spec/frontend/vue_shared/components/sidebar/labels_select_vue/dropdown_contents_labels_view_spec.js

 import Vuex from 'vuex';
 import { shallowMount, createLocalVue } from '@vue/test-utils';
 
-import { GlButton, GlLoadingIcon, GlIcon, GlSearchBoxByType, GlLink } from '@gitlab/ui';
+import { GlButton, GlLoadingIcon, GlSearchBoxByType, GlLink } from '@gitlab/ui';
 import { UP_KEY_CODE, DOWN_KEY_CODE, ENTER_KEY_CODE, ESC_KEY_CODE } from '~/lib/utils/keycodes';
 import DropdownContentsLabelsView from '~/vue_shared/components/sidebar/labels_select_vue/dropdown_contents_labels_view.vue';
+import LabelItem from '~/vue_shared/components/sidebar/labels_select_vue/label_item.vue';
 
 import defaultState from '~/vue_shared/components/sidebar/labels_select_vue/store/state';
 import mutations from '~/vue_shared/components/sidebar/labels_select_vue/store/mutations';
@@ -78,16 +79,6 @@ describe('DropdownContentsLabelsView', () => {
   });
 
   describe('methods', () => {
-    describe('getDropdownLabelBoxStyle', () => {
-      it('returns an object containing `backgroundColor` based on provided `label` param', () => {
-        expect(wrapper.vm.getDropdownLabelBoxStyle(mockRegularLabel)).toEqual(
-          expect.objectContaining({
-            backgroundColor: mockRegularLabel.color,
-          }),
-        );
-      });
-    });
-
     describe('isLabelSelected', () => {
       it('returns true when provided `label` param is one of the selected labels', () => {
         expect(wrapper.vm.isLabelSelected(mockRegularLabel)).toBe(true);
@@ -234,16 +225,7 @@ describe('DropdownContentsLabelsView', () => {
     });
 
     it('renders label elements for all labels', () => {
-      const labelsEl = wrapper.findAll('.dropdown-content li');
-      const labelItemEl = labelsEl.at(0).find(GlLink);
-
-      expect(labelsEl.length).toBe(mockLabels.length);
-      expect(labelItemEl.exists()).toBe(true);
-      expect(labelItemEl.find(GlIcon).props('name')).toBe('mobile-issue-close');
-      expect(labelItemEl.find('.dropdown-label-box').attributes('style')).toBe(
-        'background-color: rgb(186, 218, 85);',
-      );
-      expect(labelItemEl.find(GlLink).text()).toContain(mockLabels[0].title);
+      expect(wrapper.findAll(LabelItem)).toHaveLength(mockLabels.length);
     });
 
     it('renders label element with "is-focused" when value of `currentHighlightItem` is more than -1', () => {
@@ -253,9 +235,9 @@ describe('DropdownContentsLabelsView', () => {
 
       return wrapper.vm.$nextTick(() => {
         const labelsEl = wrapper.findAll('.dropdown-content li');
-        const labelItemEl = labelsEl.at(0).find(GlLink);
+        const labelItemEl = labelsEl.at(0).find(LabelItem);
 
-        expect(labelItemEl.attributes('class')).toContain('is-focused');
+        expect(labelItemEl.props('highlight')).toBe(true);
       });
     });
 
@@ -267,7 +249,7 @@ describe('DropdownContentsLabelsView', () => {
       return wrapper.vm.$nextTick(() => {
         const noMatchEl = wrapper.find('.dropdown-content li');
 
-        expect(noMatchEl.exists()).toBe(true);
+        expect(noMatchEl.isVisible()).toBe(true);
         expect(noMatchEl.text()).toContain('No matching results');
       });
     });

spec/frontend/vue_shared/components/sidebar/labels_select_vue/label_item_spec.js

+import { shallowMount } from '@vue/test-utils';
+
+import { GlIcon, GlLink } from '@gitlab/ui';
+import LabelItem from '~/vue_shared/components/sidebar/labels_select_vue/label_item.vue';
+import { mockRegularLabel } from './mock_data';
+
+const createComponent = ({ label = mockRegularLabel, highlight = true } = {}) =>
+  shallowMount(LabelItem, {
+    propsData: {
+      label,
+      highlight,
+    },
+  });
+
+describe('LabelItem', () => {
+  let wrapper;
+
+  beforeEach(() => {
+    wrapper = createComponent();
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+  });
+
+  describe('computed', () => {
+    describe('labelBoxStyle', () => {
+      it('returns an object containing `backgroundColor` based on `label` prop', () => {
+        expect(wrapper.vm.labelBoxStyle).toEqual(
+          expect.objectContaining({
+            backgroundColor: mockRegularLabel.color,
+          }),
+        );
+      });
+    });
+  });
+
+  describe('methods', () => {
+    describe('handleClick', () => {
+      it('sets value of `isSet` data prop to opposite of its current value', () => {
+        wrapper.setData({
+          isSet: true,
+        });
+
+        wrapper.vm.handleClick();
+        expect(wrapper.vm.isSet).toBe(false);
+        wrapper.vm.handleClick();
+        expect(wrapper.vm.isSet).toBe(true);
+      });
+
+      it('emits event `clickLabel` on component with `label` prop as param', () => {
+        wrapper.vm.handleClick();
+
+        expect(wrapper.emitted('clickLabel')).toBeTruthy();
+        expect(wrapper.emitted('clickLabel')[0]).toEqual([mockRegularLabel]);
+      });
+    });
+  });
+
+  describe('template', () => {
+    it('renders gl-link component', () => {
+      expect(wrapper.find(GlLink).exists()).toBe(true);
+    });
+
+    it('renders gl-link component with class `is-focused` when `highlight` prop is true', () => {
+      wrapper.setProps({
+        highlight: true,
+      });
+
+      return wrapper.vm.$nextTick(() => {
+        expect(wrapper.find(GlLink).classes()).toContain('is-focused');
+      });
+    });
+
+    it('renders visible gl-icon component when `isSet` prop is true', () => {
+      wrapper.setData({
+        isSet: true,
+      });
+
+      return wrapper.vm.$nextTick(() => {
+        const iconEl = wrapper.find(GlIcon);
+
+        expect(iconEl.isVisible()).toBe(true);
+        expect(iconEl.props('name')).toBe('mobile-issue-close');
+      });
+    });
+
+    it('renders visible span element as placeholder instead of gl-icon when `isSet` prop is false', () => {
+      wrapper.setData({
+        isSet: false,
+      });
+
+      return wrapper.vm.$nextTick(() => {
+        const placeholderEl = wrapper.find('[data-testid="no-icon"]');
+
+        expect(placeholderEl.isVisible()).toBe(true);
+      });
+    });
+
+    it('renders label color element', () => {
+      const colorEl = wrapper.find('[data-testid="label-color-box"]');
+
+      expect(colorEl.exists()).toBe(true);
+      expect(colorEl.attributes('style')).toBe('background-color: rgb(186, 218, 85);');
+    });
+
+    it('renders label title', () => {
+      expect(wrapper.text()).toContain(mockRegularLabel.title);
+    });
+  });
+});

spec/frontend/vue_shared/components/sidebar/labels_select_vue/store/mutations_spec.js

@@ -155,29 +155,12 @@ describe('LabelsSelect Mutations', () => {
   describe(`${types.UPDATE_SELECTED_LABELS}`, () => {
     const labels = [{ id: 1 }, { id: 2 }, { id: 3 }, { id: 4 }];
 
-    it('updates `state.labels` to include `touched` and `set` props based on provided `labels` param when `state.allowMultiselect` is `true`', () => {
-      const updatedLabelIds = [2, 4];
-      const state = {
-        labels,
-        allowMultiselect: true,
-      };
-      mutations[types.UPDATE_SELECTED_LABELS](state, { labels });
-
-      state.labels.forEach(label => {
-        if (updatedLabelIds.includes(label.id)) {
-          expect(label.touched).toBe(true);
-          expect(label.set).toBe(true);
-        }
-      });
-    });
-
-    it('updates `state.labels` to include `touched` and `set` props based on provided `labels` param when `state.allowMultiselect` is `false`', () => {
+    it('updates `state.labels` to include `touched` and `set` props based on provided `labels` param', () => {
       const updatedLabelIds = [2];
       const state = {
         labels,
-        allowMultiselect: false,
       };
-      mutations[types.UPDATE_SELECTED_LABELS](state, { labels });
+      mutations[types.UPDATE_SELECTED_LABELS](state, { labels: [{ id: 2 }] });
 
       state.labels.forEach(label => {
         if (updatedLabelIds.includes(label.id)) {