Merge branch '235734-mlunoe-address-size-of-chart-skeleton-loader' into 'master'

Fix(chart skeleton loader): adjust size

Closes #235734

See merge request gitlab-org/gitlab!41904

app/assets/javascripts/vue_shared/components/resizable_chart/constants.js

 export const DEFAULT_RX = 0.4;
-export const DEFAULT_BAR_WIDTH = 6;
-export const DEFAULT_LABEL_WIDTH = 4;
-export const DEFAULT_LABEL_HEIGHT = 5;
+export const DEFAULT_BAR_WIDTH = 4;
+export const DEFAULT_LABEL_WIDTH = 3;
+export const DEFAULT_LABEL_HEIGHT = 3;
 export const BAR_HEIGHTS = [5, 7, 9, 14, 21, 35, 50, 80];
 export const GRID_YS = [30, 60, 90];

app/assets/javascripts/vue_shared/components/resizable_chart/skeleton_loader.vue

@@ -61,35 +61,37 @@ export default {
 };
 </script>
 <template>
-  <gl-skeleton-loader :unique-key="uniqueKey">
-    <rect
-      v-for="(y, index) in $options.GRID_YS"
-      :key="`grid-${index}`"
-      data-testid="skeleton-chart-grid"
-      x="0"
-      :y="`${y}%`"
-      width="100%"
-      height="1px"
-    />
-    <rect
-      v-for="(height, index) in $options.BAR_HEIGHTS"
-      :key="`bar-${index}`"
-      data-testid="skeleton-chart-bar"
-      :x="`${getBarXPosition(index)}%`"
-      :y="`${90 - height}%`"
-      :width="`${barWidth}%`"
-      :height="`${height}%`"
-      :rx="`${rx}%`"
-    />
-    <rect
-      v-for="(height, index) in $options.BAR_HEIGHTS"
-      :key="`label-${index}`"
-      data-testid="skeleton-chart-label"
-      :x="`${labelCentering + getBarXPosition(index)}%`"
-      :y="`${100 - labelHeight}%`"
-      :width="`${labelWidth}%`"
-      :height="`${labelHeight}%`"
-      :rx="`${rx}%`"
-    />
-  </gl-skeleton-loader>
+  <div class="gl-px-8">
+    <gl-skeleton-loader :unique-key="uniqueKey" class="gl-p-8">
+      <rect
+        v-for="(y, index) in $options.GRID_YS"
+        :key="`grid-${index}`"
+        data-testid="skeleton-chart-grid"
+        x="0"
+        :y="`${y}%`"
+        width="100%"
+        height="1px"
+      />
+      <rect
+        v-for="(height, index) in $options.BAR_HEIGHTS"
+        :key="`bar-${index}`"
+        data-testid="skeleton-chart-bar"
+        :x="`${getBarXPosition(index)}%`"
+        :y="`${90 - height}%`"
+        :width="`${barWidth}%`"
+        :height="`${height}%`"
+        :rx="`${rx}%`"
+      />
+      <rect
+        v-for="(height, index) in $options.BAR_HEIGHTS"
+        :key="`label-${index}`"
+        data-testid="skeleton-chart-label"
+        :x="`${labelCentering + getBarXPosition(index)}%`"
+        :y="`${100 - labelHeight}%`"
+        :width="`${labelWidth}%`"
+        :height="`${labelHeight}%`"
+        :rx="`${rx}%`"
+      />
+    </gl-skeleton-loader>
+  </div>
 </template>

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

@@ -127,11 +127,11 @@ export default {
       :error="error"
     />
   </div>
-  <resizable-chart-container v-else-if="loaded" class="insights-chart">
+  <resizable-chart-container v-else class="insights-chart">
     <h5 class="text-center">{{ title }}</h5>
     <p v-if="description" class="text-center">{{ description }}</p>
     <gl-column-chart
-      v-if="isColumnChart"
+      v-if="loaded && isColumnChart"
       v-bind="$attrs"
       :height="$options.height"
       :data="data.datasets"
@@ -142,7 +142,7 @@ export default {
       @created="onChartCreated"
     />
     <gl-stacked-column-chart
-      v-else-if="isStackedColumnChart"
+      v-else-if="loaded && isStackedColumnChart"
       v-bind="$attrs"
       :height="$options.height"
       :data="data.datasets"
@@ -155,13 +155,13 @@ export default {
       @created="onChartCreated"
     />
     <gl-line-chart
-      v-else-if="isLineChart"
+      v-else-if="loaded && isLineChart"
       v-bind="$attrs"
       :height="$options.height"
       :data="data.datasets"
       :option="chartOptions"
       @created="onChartCreated"
     />
+    <chart-skeleton-loader v-else />
   </resizable-chart-container>
-  <chart-skeleton-loader v-else />
 </template>

ee/changelogs/unreleased/235734-mlunoe-address-size-of-chart-skeleton-loader.yml

+---
+title: Fix the size of chart placeholder on Analytics pages for Merge Requests, Insights and Value stream, so it matches the actual charts.
+  the size of the real chart shown afterwards
+merge_request: 41904
+author:
+type: fixed

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

@@ -10,8 +10,17 @@ import {
 import InsightsChart from 'ee/insights/components/insights_chart.vue';
 import InsightsChartError from 'ee/insights/components/insights_chart_error.vue';
 import { CHART_TYPES } from 'ee/insights/constants';
+import ResizableChartContainer from '~/vue_shared/components/resizable_chart/resizable_chart_container.vue';
 import ChartSkeletonLoader from '~/vue_shared/components/resizable_chart/skeleton_loader.vue';
 
+const DEFAULT_PROPS = {
+  loaded: false,
+  type: chartInfo.type,
+  title: chartInfo.title,
+  data: null,
+  error: '',
+};
+
 describe('Insights chart component', () => {
   let wrapper;
 
@@ -26,66 +35,32 @@ describe('Insights chart component', () => {
   });
 
   describe('when chart is loading', () => {
-    it('displays the chart loader', () => {
-      wrapper = factory({
-        loaded: false,
-        type: CHART_TYPES.BAR,
-        title: chartInfo.title,
-        data: null,
-        error: '',
-      });
+    it('displays the chart loader in the container', () => {
+      wrapper = factory(DEFAULT_PROPS);
 
       expect(wrapper.find(ChartSkeletonLoader).exists()).toBe(true);
+      expect(wrapper.find(ResizableChartContainer).exists()).toBe(true);
     });
   });
 
-  describe('when chart is loaded', () => {
-    it('displays a bar chart', () => {
-      wrapper = factory({
-        loaded: true,
-        type: CHART_TYPES.BAR,
-        title: chartInfo.title,
-        data: barChartData,
-        error: '',
-      });
-
-      expect(wrapper.find(GlColumnChart).exists()).toBe(true);
-    });
-
-    it('displays a line chart', () => {
-      wrapper = factory({
-        loaded: true,
-        type: CHART_TYPES.LINE,
-        title: chartInfo.title,
-        data: lineChartData,
-        error: '',
-      });
-
-      expect(wrapper.find(GlLineChart).exists()).toBe(true);
-    });
-
-    it('displays a stacked bar chart', () => {
-      wrapper = factory({
-        loaded: true,
-        type: CHART_TYPES.STACKED_BAR,
-        title: chartInfo.title,
-        data: stackedBarChartData,
-        error: '',
-      });
-
-      expect(wrapper.find(GlStackedColumnChart).exists()).toBe(true);
-    });
-
-    it('displays a bar chart when a pie chart is requested', () => {
+  describe.each`
+    type                       | component               | name                      | data
+    ${CHART_TYPES.BAR}         | ${GlColumnChart}        | ${'GlColumnChart'}        | ${barChartData}
+    ${CHART_TYPES.LINE}        | ${GlLineChart}          | ${'GlLineChart'}          | ${lineChartData}
+    ${CHART_TYPES.STACKED_BAR} | ${GlStackedColumnChart} | ${'GlStackedColumnChart'} | ${stackedBarChartData}
+    ${CHART_TYPES.PIE}         | ${GlColumnChart}        | ${'GlColumnChart'}        | ${barChartData}
+  `('when chart is loaded', ({ type, component, name, data }) => {
+    it(`when ${type} is passed: displays the a ${name}-chart in container and not the loader`, () => {
       wrapper = factory({
+        ...DEFAULT_PROPS,
         loaded: true,
-        type: CHART_TYPES.PIE,
-        title: chartInfo.title,
-        data: barChartData,
-        error: '',
+        type,
+        data,
       });
 
-      expect(wrapper.find(GlColumnChart).exists()).toBe(true);
+      expect(wrapper.find(ChartSkeletonLoader).exists()).toBe(false);
+      expect(wrapper.find(ResizableChartContainer).exists()).toBe(true);
+      expect(wrapper.find(component).exists()).toBe(true);
     });
   });
 
@@ -94,15 +69,15 @@ describe('Insights chart component', () => {
 
     beforeEach(() => {
       wrapper = factory({
-        loaded: false,
-        type: chartInfo.type,
-        title: chartInfo.title,
+        ...DEFAULT_PROPS,
         data: {},
         error,
       });
     });
 
     it('displays info about the error', () => {
+      expect(wrapper.find(ChartSkeletonLoader).exists()).toBe(false);
+      expect(wrapper.find(ResizableChartContainer).exists()).toBe(false);
       expect(wrapper.find(InsightsChartError).exists()).toBe(true);
     });
   });