fix(xy): remove wrongly represented null/missing values in tooltip (#…

…1415) A regression was introduced and caused `null` values to show up on the tooltip when rendering stacked bars and areas. fix #1414
elastic · Oct 7, 2021 · e5963a3 · e5963a3
1 parent 9fa9783
commit e5963a3
Show file tree

Hide file tree

Showing 10 changed files with 94 additions and 33 deletions.
diff --git a/integration/server/generate/vrt_page_template.js b/integration/server/generate/vrt_page_template.js
@@ -34,6 +34,8 @@ appendIconComponentCache({
   visualizeApp: require('@elastic/eui/es/components/icon/assets/app_visualize').icon,
 });
 
+const path = new URL(window.location.toString()).searchParams.get('path');
+document.getElementsByTagName('body')[0].style.overflow = path ? 'hidden' : 'scroll';
 ReactDOM.render(<VRTPage />, document.getElementById('story-root') as HTMLElement);
 
 `.trim();

diff --git a/integration/server/index.ejs b/integration/server/index.ejs
@@ -4,11 +4,6 @@
     <meta charset="UTF-8" />
     <meta name="viewport" content="width=device-width, initial-scale=1.0" />
     <meta http-equiv="X-UA-Compatible" content="ie=edge" />
-    <style>
-      html {
-        overflow:hidden !important;
-      }
-    </style>
   </head>
 
   <body>

diff --git a/...r-all-stories-bar-chart-stacked-as-percentage-visually-looks-correct-1-snap.png b/...r-all-stories-bar-chart-stacked-as-percentage-visually-looks-correct-1-snap.png
diff --git a/...ns-test-ts-interactions-hide-null-bars-in-tooltip-in-percentage-mode-1-snap.png b/...ns-test-ts-interactions-hide-null-bars-in-tooltip-in-percentage-mode-1-snap.png
diff --git a/integration/tests/interactions.test.ts b/integration/tests/interactions.test.ts
@@ -341,6 +341,18 @@ describe('Interactions', () => {
       );
     });
   });
+
+  it('Hide null bars in tooltip in percentage mode', async () => {
+    await common.expectChartWithMouseAtUrlToMatchScreenshot(
+      'http://localhost:9001/?path=/story/bar-chart--test-stacked-bar-chart-with-null-bars&knob-stack%20as%20percentage=true',
+      { left: 350, top: 150 },
+      {
+        screenshotSelector: '#story-root',
+        delay: 1000,
+      },
+    );
+  });
+
   describe('should show null values in tooltip if configured', () => {
     it('show N/A tooltip for areas', async () => {
       await common.expectChartWithMouseAtUrlToMatchScreenshot(

diff --git a/packages/charts/src/chart_types/xy_chart/rendering/bars.ts b/packages/charts/src/chart_types/xy_chart/rendering/bars.ts
@@ -10,13 +10,14 @@ import { Color } from '../../../common/colors';
 import { Scale } from '../../../scales';
 import { ScaleType } from '../../../scales/constants';
 import { TextMeasure, withTextMeasure } from '../../../utils/bbox/canvas_text_bbox_calculator';
-import { clamp, isNil, mergePartial } from '../../../utils/common';
+import { clamp, mergePartial } from '../../../utils/common';
 import { Dimensions } from '../../../utils/dimensions';
 import { BandedAccessorType, BarGeometry } from '../../../utils/geometry';
 import { BarSeriesStyle, DisplayValueStyle } from '../../../utils/themes/theme';
 import { IndexedGeometryMap } from '../utils/indexed_geometry_map';
 import { DataSeries, DataSeriesDatum, XYChartSeriesIdentifier } from '../utils/series';
 import { BarStyleAccessor, DisplayValueSpec, LabelOverflowConstraint, StackMode } from '../utils/specs';
+import { getDatumYValue } from './points';
 
 const PADDING = 1; // default padding for now
 const FONT_SIZE_FACTOR = 0.7; // Take 70% of space for the label text
@@ -87,8 +88,8 @@ export function renderBars(
       const width = clamp(seriesStyle.rect.widthPixel ?? xScale.bandwidth, minPixelWidth, maxPixelWidth);
       const x = xScaled + xScale.bandwidth * orderIndex + xScale.bandwidth / 2 - width / 2;
 
-      const originalY1Value = stackMode === StackMode.Percentage ? (isNil(y1) ? null : y1 - (y0 ?? 0)) : initialY1;
-      const formattedDisplayValue = displayValueSettings?.valueFormatter?.(originalY1Value);
+      const y1Value = getDatumYValue(datum, false, false, stackMode);
+      const formattedDisplayValue = displayValueSettings?.valueFormatter?.(y1Value);
 
       // only show displayValue for even bars if showOverlappingValue
       const displayValueText =
@@ -140,7 +141,7 @@ export function renderBars(
         width,
         height,
         color,
-        value: { x: datum.x, y: originalY1Value, mark: null, accessor: BandedAccessorType.Y1, datum: datum.datum },
+        value: { x: datum.x, y: y1Value, mark: null, accessor: BandedAccessorType.Y1, datum: datum.datum },
         seriesIdentifier,
         seriesStyle,
         panel,

diff --git a/packages/charts/src/chart_types/xy_chart/rendering/points.ts b/packages/charts/src/chart_types/xy_chart/rendering/points.ts
@@ -150,30 +150,25 @@ export function getPointStyleOverrides(
  * @param lookingForY0 if we are interested in the y0 value, false for y1
  * @param isBandChart if the chart is a band chart
  * @param stackMode an optional stack mode
+ * @internal
  */
-function getDatumYValue(
+export function getDatumYValue(
   { y1, y0, initialY1, initialY0 }: DataSeriesDatum,
   lookingForY0: boolean,
   isBandChart: boolean,
   stackMode?: StackMode,
 ) {
   if (isBandChart) {
-    return stackMode === StackMode.Percentage
-      ? // on band stacked charts in percentage mode, the values I'm looking for are the percentage value
-        // that are already computed and available on y0 and y1
-        lookingForY0
-        ? y0
-        : y1
-      : // in all other cases for band charts, I want to get back the original/initial value of y0 and y1
-      // not the computed value
-      lookingForY0
-      ? initialY0
-      : initialY1;
+    // on band stacked charts in percentage mode, the values I'm looking for are the percentage value
+    // that are already computed and available on y0 and y1
+    // in all other cases for band charts, I want to get back the original/initial value of y0 and y1
+    // not the computed value
+    return stackMode === StackMode.Percentage ? (lookingForY0 ? y0 : y1) : lookingForY0 ? initialY0 : initialY1;
   }
   // if not a band chart get use the original/initial value in every case except for stack as percentage
   // in this case, we should take the difference between the bottom position of the bar and the top position
   // of the bar
-  return stackMode === StackMode.Percentage ? (y1 ?? 0) - (y0 ?? 0) : initialY1;
+  return stackMode === StackMode.Percentage ? (isNil(y1) || isNil(initialY1) ? null : y1 - (y0 ?? 0)) : initialY1;
 }
 
 /**

diff --git a/packages/charts/src/chart_types/xy_chart/state/chart_state.interactions.test.tsx b/packages/charts/src/chart_types/xy_chart/state/chart_state.interactions.test.tsx
@@ -17,7 +17,7 @@ import { Rect } from '../../../geoms/types';
 import { MockAnnotationSpec, MockGlobalSpec, MockSeriesSpec } from '../../../mocks/specs/specs';
 import { MockStore } from '../../../mocks/store';
 import { ScaleType } from '../../../scales/constants';
-import { BrushEvent, SettingsSpec } from '../../../specs';
+import { BrushEvent, SettingsSpec, StackMode } from '../../../specs';
 import { SpecType, TooltipType, BrushAxis } from '../../../specs/constants';
 import { onExternalPointerEvent } from '../../../state/actions/events';
 import { onPointerMove, onMouseDown, onMouseUp } from '../../../state/actions/mouse';
@@ -1353,6 +1353,59 @@ describe('Clickable annotations', () => {
     store.dispatch(onMouseUp({ x: 10, y: 10 }, 200));
     expect(onAnnotationClick).toBeCalled();
   });
+
+  describe('Tooltip on null/missing values', () => {
+    const partialSpec = {
+      data: [
+        { x: 0, y: 0, g: 'a' },
+        { x: 1, y: null, g: 'a' },
+        { x: 0, y: 4, g: 'b' },
+        { x: 1, y: 5, g: 'b' },
+        { x: 2, y: 2, g: 'b' },
+      ],
+      xAccessor: 'x',
+      yAccessors: ['y'],
+      splitSeriesAccessors: ['g'],
+      stackAccessors: ['x'],
+      xScaleType: ScaleType.Ordinal,
+      yScaleType: ScaleType.Linear,
+    };
+    let store: Store;
+    beforeEach(() => {
+      store = MockStore.default({ width: 90, height: 100, top: 0, left: 0 });
+    });
+
+    const tooltipValues = (s: Store) =>
+      getTooltipInfoAndGeometriesSelector(s.getState()).tooltip.values.map((d) => [d.label, d.value]);
+
+    it.each`
+      type               | stackMode               | first                   | second        | third
+      ${SeriesType.Bar}  | ${StackMode.Percentage} | ${[['a', 0], ['b', 1]]} | ${[['b', 1]]} | ${[['b', 1]]}
+      ${SeriesType.Bar}  | ${undefined}            | ${[['a', 0], ['b', 4]]} | ${[['b', 5]]} | ${[['b', 2]]}
+      ${SeriesType.Area} | ${StackMode.Percentage} | ${[['a', 0], ['b', 1]]} | ${[['b', 1]]} | ${[['b', 1]]}
+      ${SeriesType.Area} | ${undefined}            | ${[['a', 0], ['b', 4]]} | ${[['b', 5]]} | ${[['b', 2]]}
+    `(
+      `tooltip should hide null/missing values on stacked $type in $stackMode mode`,
+      ({ type, stackMode, first, second, third }) => {
+        MockStore.addSpecs(
+          [
+            MockSeriesSpec.byTypePartial(type)({ ...partialSpec, stackMode }),
+            MockGlobalSpec.settingsNoMargins({ theme: { colors: { vizColors: ['red', 'blue'] } } }),
+          ],
+          store,
+        );
+        // move over the 1st bar
+        store.dispatch(onPointerMove({ x: 15, y: 50 }, 0));
+        expect(tooltipValues(store)).toIncludeSameMembers(first);
+        // move over the 2nd bar (hide the null)
+        store.dispatch(onPointerMove({ x: 45, y: 50 }, 1));
+        expect(tooltipValues(store)).toIncludeSameMembers(second);
+        // move over the 3rd bar (hide missing series)
+        store.dispatch(onPointerMove({ x: 75, y: 50 }, 1));
+        expect(tooltipValues(store)).toIncludeSameMembers(third);
+      },
+    );
+  });
 });
 
 /* eslint-enable jest/no-conditional-expect */
diff --git a/storybook/stories/bar/12_stacked_as_percentage.story.tsx b/storybook/stories/bar/12_stacked_as_percentage.story.tsx
@@ -14,17 +14,17 @@ import { Axis, BarSeries, Chart, Position, ScaleType, Settings, StackMode } from
 import { useBaseTheme } from '../../use_base_theme';
 
 export const Example = () => {
-  const stackedAsPercentage = boolean('stacked as percentage', true);
-  const clusterBars = boolean('cluster', true);
+  const stackMode = boolean('stacked as percentage', true) ? StackMode.Percentage : undefined;
   return (
     <Chart>
       <Settings showLegend showLegendExtra legendPosition={Position.Right} baseTheme={useBaseTheme()} />
       <Axis id="bottom" position={Position.Bottom} title="Bottom axis" showOverlappingTicks />
+
       <Axis
         id="left2"
         title="Left axis"
         position={Position.Left}
-        tickFormat={(d: any) => (stackedAsPercentage && !clusterBars ? `${Number(d * 100).toFixed(0)} %` : d)}
+        tickFormat={(d: any) => (stackMode === StackMode.Percentage ? `${Number(d * 100).toFixed(0)} %` : d)}
       />
 
       <BarSeries
@@ -33,18 +33,18 @@ export const Example = () => {
         yScaleType={ScaleType.Linear}
         xAccessor="x"
         yAccessors={['y']}
-        stackAccessors={clusterBars ? [] : ['x']}
-        stackMode={clusterBars ? undefined : StackMode.Percentage}
+        stackMode={stackMode}
+        stackAccessors={['x']}
         splitSeriesAccessors={['g']}
         data={[
-          { x: 0, y: 2, g: 'a' },
-          { x: 1, y: 7, g: 'a' },
-          { x: 2, y: 3, g: 'a' },
-          { x: 3, y: 6, g: 'a' },
           { x: 0, y: 4, g: 'b' },
           { x: 1, y: 5, g: 'b' },
           { x: 2, y: 8, g: 'b' },
           { x: 3, y: 2, g: 'b' },
+          { x: 0, y: 2, g: 'a' },
+          { x: 1, y: 2, g: 'a' },
+          { x: 2, y: 0, g: 'a' },
+          { x: 3, y: null, g: 'a' },
         ]}
       />
     </Chart>

diff --git a/storybook/stories/bar/39_test_stacked_null.story.tsx b/storybook/stories/bar/39_test_stacked_null.story.tsx
@@ -6,9 +6,10 @@
  * Side Public License, v 1.
  */
 
+import { boolean } from '@storybook/addon-knobs';
 import React from 'react';
 
-import { Axis, BarSeries, Chart, Position, ScaleType, Settings } from '@elastic/charts';
+import { Axis, BarSeries, Chart, Position, ScaleType, Settings, StackMode } from '@elastic/charts';
 import { KIBANA_METRICS } from '@elastic/charts/src/utils/data_samples/test_dataset_kibana';
 
 import { useBaseTheme } from '../../use_base_theme';
@@ -25,6 +26,7 @@ export const Example = () => {
     [3, null, 5, 'b'],
     [4, 4, 6, 'b'],
   ];
+  const stackMode = boolean('stack as percentage', false) ? StackMode.Percentage : undefined;
   return (
     <Chart>
       <Settings baseTheme={useBaseTheme()} />
@@ -43,6 +45,7 @@ export const Example = () => {
         yAccessors={[1]}
         splitSeriesAccessors={[3]}
         stackAccessors={[0]}
+        stackMode={stackMode}
         data={data}
       />
     </Chart>