Merge branch '219742-add-general-chart-skeleton-loader-to-insights-page' into 'master'

Feat(Insights Charts): use skeleton chart loader

Closes #219742

See merge request gitlab-org/gitlab!37815

ee/app/assets/javascripts/insights/components/insights.vue

@@ -34,8 +34,18 @@ export default {
       'configLoading',
       'activeTab',
       'activePage',
-      'pageLoading',
+      'chartData',
     ]),
+    pageLoading() {
+      const requestedChartKeys = this.activePage?.charts?.map(chart => chart.title) || [];
+      const storeChartKeys = Object.keys(this.chartData);
+      const loadedCharts = storeChartKeys.filter(key => this.chartData[key].loaded);
+      const chartsLoaded =
+        Boolean(requestedChartKeys.length) &&
+        requestedChartKeys.every(key => loadedCharts.includes(key));
+      const chartsErrored = storeChartKeys.some(key => this.chartData[key].error);
+      return !chartsLoaded && !chartsErrored;
+    },
     pages() {
       const { configData, activeTab } = this;
 

ee/app/assets/javascripts/insights/components/insights_chart.vue

@@ -3,6 +3,7 @@ import { GlColumnChart, GlLineChart, GlStackedColumnChart } from '@gitlab/ui/dis
 
 import { getSvgIconPathContent } from '~/lib/utils/icon_utils';
 import ResizableChartContainer from '~/vue_shared/components/resizable_chart/resizable_chart_container.vue';
+import ChartSkeletonLoader from '~/vue_shared/components/resizable_chart/skeleton_loader.vue';
 
 import InsightsChartError from './insights_chart_error.vue';
 import { CHART_TYPES } from '../constants';
@@ -14,8 +15,9 @@ export default {
     GlColumnChart,
     GlLineChart,
     GlStackedColumnChart,
-    ResizableChartContainer,
     InsightsChartError,
+    ResizableChartContainer,
+    ChartSkeletonLoader,
   },
   props: {
     loaded: {
@@ -117,7 +119,15 @@ export default {
 };
 </script>
 <template>
-  <resizable-chart-container v-if="loaded" class="insights-chart">
+  <div v-if="error" class="insights-chart">
+    <insights-chart-error
+      :chart-name="title"
+      :title="__('This chart could not be displayed')"
+      :summary="__('Please check the configuration file for this chart')"
+      :error="error"
+    />
+  </div>
+  <resizable-chart-container v-else-if="loaded" class="insights-chart">
     <h5 class="text-center">{{ title }}</h5>
     <p v-if="description" class="text-center">{{ description }}</p>
     <gl-column-chart
@@ -153,12 +163,5 @@ export default {
       @created="onChartCreated"
     />
   </resizable-chart-container>
-  <div v-else-if="error" class="insights-chart">
-    <insights-chart-error
-      :chart-name="title"
-      :title="__('This chart could not be displayed')"
-      :summary="__('Please check the configuration file for this chart')"
-      :error="error"
-    />
-  </div>
+  <chart-skeleton-loader v-else />
 </template>

ee/app/assets/javascripts/insights/components/insights_page.vue

 <script>
-import { GlLoadingIcon } from '@gitlab/ui';
+import { GlEmptyState } from '@gitlab/ui';
 import { isUndefined } from 'lodash';
 import { mapActions, mapState } from 'vuex';
 
-import InsightsConfigWarning from './insights_config_warning.vue';
 import InsightsChart from './insights_chart.vue';
+import { __ } from '~/locale';
 
 export default {
   components: {
-    GlLoadingIcon,
+    GlEmptyState,
     InsightsChart,
-    InsightsConfigWarning,
   },
   props: {
     queryEndpoint: {
@@ -23,23 +22,27 @@ export default {
     },
   },
   computed: {
-    ...mapState('insights', ['chartData', 'pageLoading']),
+    ...mapState('insights', ['chartData']),
+    emptyState() {
+      return {
+        title: __('There are no charts configured for this page'),
+        description: __(
+          'Please check the configuration file to ensure that a collection of charts has been declared.',
+        ),
+      };
+    },
     charts() {
       return this.pageConfig.charts;
     },
     chartKeys() {
       return this.charts.map(chart => chart.title);
     },
-    storeKeys() {
-      return Object.keys(this.chartData);
-    },
     hasChartsConfigured() {
       return !isUndefined(this.charts) && this.charts.length > 0;
     },
   },
   watch: {
     pageConfig() {
-      this.setPageLoading(true);
       this.fetchCharts();
     },
   },
@@ -47,26 +50,14 @@ export default {
     this.fetchCharts();
   },
   methods: {
-    ...mapActions('insights', ['fetchChartData', 'initChartData', 'setPageLoading']),
+    ...mapActions('insights', ['fetchChartData', 'initChartData']),
     fetchCharts() {
       if (this.hasChartsConfigured) {
         this.initChartData(this.chartKeys);
 
-        const insightsRequests = this.charts.map(chart =>
-          this.fetchChartData({ endpoint: this.queryEndpoint, chart }),
-        );
-        Promise.all(insightsRequests)
-          .then(() => {
-            this.setPageLoading(!this.storePopulated());
-          })
-          .catch(() => {
-            this.setPageLoading(false);
-          });
+        this.charts.forEach(chart => this.fetchChartData({ endpoint: this.queryEndpoint, chart }));
       }
     },
-    storePopulated() {
-      return this.chartKeys.filter(key => this.storeKeys.includes(key)).length > 0;
-    },
   },
 };
 </script>
@@ -86,19 +77,12 @@ export default {
           :error="error"
         />
       </div>
-      <div v-if="pageLoading" class="insights-chart-loading text-center p-5">
-        <gl-loading-icon :inline="true" size="lg" />
-      </div>
     </div>
-    <insights-config-warning
+    <gl-empty-state
       v-else
-      :title="__('There are no charts configured for this page')"
-      :summary="
-        __(
-          'Please check the configuration file to ensure that a collection of charts has been declared.',
-        )
-      "
-      image="illustrations/monitoring/getting_started.svg"
+      :title="emptyState.title"
+      :description="emptyState.description"
+      svg-path="/assets/illustrations/monitoring/getting_started.svg"
     />
   </div>
 </template>

ee/app/assets/javascripts/insights/stores/modules/insights/actions.js

@@ -70,6 +70,4 @@ export const setActiveTab = ({ commit, state }, key) => {
   }
 };
 
-export const initChartData = ({ commit }, store) => commit(types.INIT_CHART_DATA, store);
-export const setPageLoading = ({ commit }, pageLoading) =>
-  commit(types.SET_PAGE_LOADING, pageLoading);
+export const initChartData = ({ commit }, keys) => commit(types.INIT_CHART_DATA, keys);

ee/app/assets/javascripts/insights/stores/modules/insights/mutations.js

@@ -53,7 +53,4 @@ export default {
       return acc;
     }, {});
   },
-  [types.SET_PAGE_LOADING](state, pageLoading) {
-    state.pageLoading = pageLoading;
-  },
 };

ee/app/assets/javascripts/insights/stores/modules/insights/state.js

@@ -4,5 +4,4 @@ export default () => ({
   activeTab: null,
   activePage: null,
   chartData: {},
-  pageLoading: true,
 });

ee/changelogs/unreleased/219742-add-general-chart-skeleton-loader-to-insights-page.yml

+---
+title: Use new chart loader on Insights page
+merge_request: 37815
+author:
+type: changed

ee/spec/frontend/insights/components/insights_chart_spec.js

@@ -9,6 +9,7 @@ import {
 } from 'ee_jest/insights/mock_data';
 import InsightsChart from 'ee/insights/components/insights_chart.vue';
 import InsightsChartError from 'ee/insights/components/insights_chart_error.vue';
+import ChartSkeletonLoader from '~/vue_shared/components/resizable_chart/skeleton_loader.vue';
 import { CHART_TYPES } from 'ee/insights/constants';
 
 describe('Insights chart component', () => {
@@ -24,6 +25,20 @@ describe('Insights chart component', () => {
     wrapper.destroy();
   });
 
+  describe('when chart is loading', () => {
+    it('displays the chart loader', () => {
+      wrapper = factory({
+        loaded: false,
+        type: CHART_TYPES.BAR,
+        title: chartInfo.title,
+        data: null,
+        error: '',
+      });
+
+      expect(wrapper.contains(ChartSkeletonLoader)).toBe(true);
+    });
+  });
+
   describe('when chart is loaded', () => {
     it('displays a bar chart', () => {
       wrapper = factory({

ee/spec/frontend/insights/components/insights_page_spec.js

-import Vue from 'vue';
+import Vuex from 'vuex';
+import { GlEmptyState } from '@gitlab/ui';
+import { createLocalVue, shallowMount } from '@vue/test-utils';
+import { GlColumnChart } from '@gitlab/ui/dist/charts';
+
 import InsightsPage from 'ee/insights/components/insights_page.vue';
+import InsightsChart from 'ee/insights/components/insights_chart.vue';
 import { createStore } from 'ee/insights/stores';
-import { mountComponentWithStore } from 'helpers/vue_mount_component_helper';
 import { TEST_HOST } from 'helpers/test_constants';
-import { chartInfo, pageInfo, pageInfoNoCharts } from 'ee_jest/insights/mock_data';
+import { chartInfo, pageInfo, pageInfoNoCharts, barChartData } from 'ee_jest/insights/mock_data';
+
+const localVue = createLocalVue();
+localVue.use(Vuex);
 
 describe('Insights page component', () => {
-  let component;
   let store;
-  let Component;
+  let wrapper;
+
+  const createComponent = (props = {}) => {
+    wrapper = shallowMount(InsightsPage, {
+      localVue,
+      store,
+      propsData: {
+        queryEndpoint: `${TEST_HOST}/query`,
+        pageConfig: pageInfoNoCharts,
+        ...props,
+      },
+    });
+  };
+
+  const createLoadingChartData = () => {
+    return pageInfo.charts.reduce((memo, chart) => {
+      return { ...memo, [chart.title]: {} };
+    }, {});
+  };
+
+  const createLoadedChartData = () => {
+    return pageInfo.charts.reduce((memo, chart) => {
+      return {
+        ...memo,
+        [chart.title]: {
+          loaded: true,
+          type: chart.type,
+          description: '',
+          data: barChartData,
+          error: null,
+        },
+      };
+    }, {});
+  };
+
+  const findInsightsChartData = () => wrapper.find(InsightsChart);
 
   beforeEach(() => {
     store = createStore();
     jest.spyOn(store, 'dispatch').mockImplementation(() => {});
-    Component = Vue.extend(InsightsPage);
   });
 
   afterEach(() => {
-    component.$destroy();
+    wrapper.destroy();
+    wrapper = null;
   });
 
   describe('no chart config available', () => {
-    it('shows an empty state', () => {
-      component = mountComponentWithStore(Component, {
-        store,
-        props: {
-          queryEndpoint: `${TEST_HOST}/query`,
-          pageConfig: pageInfoNoCharts,
-        },
+    beforeEach(() => {
+      createComponent();
+    });
+
+    it('does not fetch chart data when mounted', () => {
+      expect(store.dispatch).not.toHaveBeenCalled();
     });
 
-      expect(component.$el.querySelector('.js-empty-state')).not.toBe(null);
+    it('shows an empty state', () => {
+      expect(wrapper.contains(GlEmptyState)).toBe(true);
     });
   });
 
   describe('charts configured', () => {
     beforeEach(() => {
-      component = mountComponentWithStore(Component, {
-        store,
-        props: {
-          queryEndpoint: `${TEST_HOST}/query`,
-          pageConfig: pageInfo,
-        },
-      });
+      createComponent({ pageConfig: pageInfo });
     });
 
     it('fetches chart data when mounted', () => {
@@ -52,44 +87,44 @@ describe('Insights page component', () => {
       });
     });
 
-    describe('when charts loading', () => {
-      beforeEach(() => {
-        component.$store.state.insights.pageLoading = true;
+    it('does not show empty state', () => {
+      expect(wrapper.contains(GlEmptyState)).toBe(false);
     });
 
-      it('renders loading state', () => {
-        return component.$nextTick(() => {
-          expect(
-            component.$el.querySelector('.js-insights-page-container .insights-chart-loading'),
-          ).not.toBe(null);
+    describe('pageConfig changes', () => {
+      it('reflects new state', async () => {
+        wrapper.setProps({ pageConfig: pageInfoNoCharts });
+
+        await wrapper.vm.$nextTick();
+
+        expect(wrapper.contains(GlEmptyState)).toBe(true);
+      });
     });
+
+    describe('when charts loading', () => {
+      beforeEach(() => {
+        store.state.insights.chartData = createLoadingChartData();
       });
 
-      it('does display chart area', () => {
-        return component.$nextTick(() => {
-          expect(
-            component.$el.querySelector('.js-insights-page-container .insights-charts'),
-          ).not.toBe(null);
+      it('renders loading state', () => {
+        expect(findInsightsChartData().props()).toMatchObject({
+          loaded: false,
         });
       });
 
       it('does not display chart', () => {
-        return component.$nextTick(() => {
-          expect(
-            component.$el.querySelector(
-              '.js-insights-page-container .insights-charts .insights-chart',
-            ),
-          ).toBe(null);
-        });
+        expect(wrapper.contains(GlColumnChart)).toBe(false);
       });
     });
 
-    describe('pageConfig changes', () => {
-      it('reflects new state', () => {
-        component.pageConfig = pageInfoNoCharts;
+    describe('charts configured and loaded', () => {
+      beforeEach(() => {
+        store.state.insights.chartData = createLoadedChartData();
+      });
 
-        return component.$nextTick(() => {
-          expect(component.$el.querySelector('.js-empty-state')).not.toBe(null);
+      it('does not render loading state', () => {
+        expect(findInsightsChartData().props()).toMatchObject({
+          loaded: true,
         });
       });
     });

ee/spec/frontend/insights/components/insights_spec.js

@@ -56,8 +56,13 @@ describe('Insights component', () => {
 
   describe('when config loaded', () => {
     const title = 'Bugs Per Team';
+    const chart1 = { title: 'foo' };
+    const chart2 = { title: 'bar' };
+
+    describe('when charts have not been initialized', () => {
       const page = {
         title,
+        charts: [],
       };
 
       beforeEach(() => {
@@ -71,15 +76,63 @@ describe('Insights component', () => {
       it('has the correct nav tabs', () => {
         return vm.$nextTick(() => {
           expect(vm.$el.querySelector('.js-insights-dropdown')).not.toBe(null);
-        expect(vm.$el.querySelector('.js-insights-dropdown .dropdown-item').innerText.trim()).toBe(
+          expect(
+            vm.$el.querySelector('.js-insights-dropdown .dropdown-item').innerText.trim(),
+          ).toBe(title);
+        });
+      });
+
+      it('disables the tab selector', () => {
+        return vm.$nextTick(() => {
+          expect(
+            vm.$el.querySelector('.js-insights-dropdown > button').getAttribute('disabled'),
+          ).toBe('disabled');
+        });
+      });
+    });
+
+    describe('when charts have been initialized', () => {
+      const page = {
         title,
-        );
+        charts: [chart1, chart2],
+      };
+
+      beforeEach(() => {
+        vm.$store.state.insights.configLoading = false;
+        vm.$store.state.insights.activePage = page;
+        vm.$store.state.insights.configData = {
+          bugsPerTeam: page,
+        };
+        vm.$store.state.insights.chartData = {
+          [chart1.title]: {},
+          [chart2.title]: {},
+        };
+      });
+
+      it('enables the tab selector', () => {
+        return vm.$nextTick(() => {
+          expect(
+            vm.$el.querySelector('.js-insights-dropdown > button').getAttribute('disabled'),
+          ).toBe('disabled');
+        });
       });
     });
 
-    describe('when loading page', () => {
+    describe('when some charts have been loaded', () => {
+      const page = {
+        title,
+        charts: [chart1],
+      };
+
       beforeEach(() => {
-        vm.$store.state.insights.pageLoading = true;
+        vm.$store.state.insights.configLoading = false;
+        vm.$store.state.insights.activePage = page;
+        vm.$store.state.insights.configData = {
+          bugsPerTeam: page,
+        };
+        vm.$store.state.insights.chartData = {
+          [chart2.title]: { loaded: true },
+        };
       });
 
       it('disables the tab selector', () => {
@@ -90,6 +143,60 @@ describe('Insights component', () => {
         });
       });
     });
+
+    describe('when all charts have loaded', () => {
+      const page = {
+        title,
+        charts: [chart1, chart2],
+      };
+
+      beforeEach(() => {
+        vm.$store.state.insights.configLoading = false;
+        vm.$store.state.insights.activePage = page;
+        vm.$store.state.insights.configData = {
+          bugsPerTeam: page,
+        };
+        vm.$store.state.insights.chartData = {
+          [chart1.title]: { loaded: true },
+          [chart2.title]: { loaded: true },
+        };
+      });
+
+      it('enables the tab selector', () => {
+        return vm.$nextTick(() => {
+          expect(
+            vm.$el.querySelector('.js-insights-dropdown > button').getAttribute('disabled'),
+          ).toBe(null);
+        });
+      });
+    });
+
+    describe('when one chart has an error', () => {
+      const page = {
+        title,
+        charts: [chart1, chart2],
+      };
+
+      beforeEach(() => {
+        vm.$store.state.insights.configLoading = false;
+        vm.$store.state.insights.activePage = page;
+        vm.$store.state.insights.configData = {
+          bugsPerTeam: page,
+        };
+        vm.$store.state.insights.chartData = {
+          [chart1.title]: { error: 'Baz' },
+          [chart2.title]: { loaded: true },
+        };
+      });
+
+      it('enables the tab selector', () => {
+        return vm.$nextTick(() => {
+          expect(
+            vm.$el.querySelector('.js-insights-dropdown > button').getAttribute('disabled'),
+          ).toBe(null);
+        });
+      });
+    });
   });
 
   describe('empty config', () => {
@@ -105,6 +212,12 @@ describe('Insights component', () => {
         );
       });
     });
+
+    it('does not display dropdown', () => {
+      return vm.$nextTick(() => {
+        expect(vm.$el.querySelector('.js-insights-dropdown > button')).toBe(null);
+      });
+    });
   });
 
   describe('filtered out items', () => {
@@ -120,6 +233,12 @@ describe('Insights component', () => {
         );
       });
     });
+
+    it('does not display dropdown', () => {
+      return vm.$nextTick(() => {
+        expect(vm.$el.querySelector('.js-insights-dropdown > button')).toBe(null);
+      });
+    });
   });
 
   describe('hash fragment present', () => {

ee/spec/frontend/insights/stores/insights/actions_spec.js

@@ -295,18 +295,4 @@ describe('Insights store actions', () => {
       );
     });
   });
-
-  describe('setPageLoading', () => {
-    it('commits SET_PAGE_LOADING', () => {
-      const pageLoading = false;
-
-      return testAction(
-        actions.setPageLoading,
-        pageLoading,
-        null,
-        [{ type: 'SET_PAGE_LOADING', payload: false }],
-        [],
-      );
-    });
-  });
 });

ee/spec/frontend/insights/stores/insights/mutations_spec.js

@@ -114,6 +114,10 @@ describe('Insights mutations', () => {
       seriesNames: ['Dataset 1', 'Dataset 2'],
     };
 
+    beforeEach(() => {
+      mutations[types.INIT_CHART_DATA](state, [chart.title]);
+    });
+
     it('sets charts loaded state to true on success', () => {
       mutations[types.RECEIVE_CHART_SUCCESS](state, { chart, data: incomingData });
 
@@ -192,14 +196,4 @@ describe('Insights mutations', () => {
       expect(state.chartData).toEqual({ a: {}, b: {} });
     });
   });
-
-  describe(types.SET_PAGE_LOADING, () => {
-    const pageLoading = true;
-
-    it('sets pageLoading state', () => {
-      mutations[types.SET_PAGE_LOADING](state, pageLoading);
-
-      expect(state.pageLoading).toBe(pageLoading);
-    });
-  });
 });