[8.19] [Lens] Always show number sign when comparing to Primary metric (#222824) (#223201)

kibanamachine · dej611 · web-flow · commit 15c5b6026c78 · 2025-06-10T14:13:18.000+02:00
# Backport This will backport the following commits from `main` to `8.19`: - [[Lens] Always show number sign when comparing to Primary metric (#222824)](#222824)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
diff --git a/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/helpers.ts b/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/helpers.ts
@@ -16,7 +16,16 @@ import { getColumnByAccessor, getFormatByAccessor } from '@kbn/visualizations-pl
 import { getFormatService, getPaletteService } from '../services';
 import { getDataBoundsForPalette } from '../utils';
 
-function enhanceFieldFormat(serializedFieldFormat: SerializedFieldFormat | undefined) {
+export interface FormatOverrides {
+  number?: { alwaysShowSign?: boolean };
+  percent?: { alwaysShowSign?: boolean };
+  bytes?: { alwaysShowSign?: boolean };
+}
+
+function enhanceFieldFormat(
+  serializedFieldFormat: SerializedFieldFormat | undefined,
+  formatOverrides: FormatOverrides | undefined
+): SerializedFieldFormat {
   const formatId = serializedFieldFormat?.id || 'number';
   if (formatId === 'duration' && !serializedFieldFormat?.params?.formatOverride) {
     return {
@@ -31,17 +40,28 @@ function enhanceFieldFormat(serializedFieldFormat: SerializedFieldFormat | undef
       },
     };
   }
+  if (formatOverrides && formatId in formatOverrides) {
+    return {
+      ...serializedFieldFormat,
+      params: {
+        ...serializedFieldFormat?.params,
+        ...formatOverrides[formatId as keyof FormatOverrides],
+      },
+    };
+  }
+
   return serializedFieldFormat ?? { id: formatId };
 }
 
 export const getMetricFormatter = (
   accessor: ExpressionValueVisDimension | string,
-  columns: Datatable['columns']
+  columns: Datatable['columns'],
+  formatOverrides?: FormatOverrides | undefined
 ) => {
   const type = getColumnByAccessor(accessor, columns)?.meta.type;
   const defaultFormat = type ? { id: type } : undefined;
   const serializedFieldFormat = getFormatByAccessor(accessor, columns, defaultFormat);
-  const enhancedFieldFormat = enhanceFieldFormat(serializedFieldFormat);
+  const enhancedFieldFormat = enhanceFieldFormat(serializedFieldFormat, formatOverrides);
   return getFormatService().deserialize(enhancedFieldFormat).getConverterFor('text');
 };
 
diff --git a/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/metric_vis.test.tsx b/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/metric_vis.test.tsx
@@ -9,7 +9,7 @@
 
 import React from 'react';
 import userEvent from '@testing-library/user-event';
-import { render, screen, waitFor } from '@testing-library/react';
+import { act, render, screen, waitFor } from '@testing-library/react';
 import { Datatable, DatatableColumn } from '@kbn/expressions-plugin/common';
 import { MetricVis, MetricVisComponentProps } from './metric_vis';
 import { MetricWTrend } from '@elastic/charts';
@@ -238,8 +238,12 @@ describe('MetricVisComponent', function () {
   });
 
   async function waitForChartToRender(renderComplete: MetricVisComponentProps['renderComplete']) {
-    // wait for 1 rAF tick (~16ms)
-    jest.advanceTimersByTime(30);
+    // Interestingly we have to wrap this into an act() call to avoid
+    // issues with the React scheduling when testing
+    await act(async () => {
+      // wait for 1 rAF tick (~16ms)
+      jest.advanceTimersByTime(30);
+    });
     // wait for render complete callback
     await waitFor(() => expect(renderComplete).toHaveBeenCalled());
   }
diff --git a/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/metric_vis.tsx b/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/metric_vis.tsx
@@ -184,14 +184,13 @@ export const MetricVis = ({
         title: String(title),
         subtitle,
         icon: config.metric?.icon ? getIcon(config.metric?.icon) : undefined,
-        extra: ({ fontSize, color }) => (
+        extra: ({ color }) => (
           <SecondaryMetric
             row={row}
             config={config}
             columns={data.columns}
             getMetricFormatter={getMetricFormatter}
             color={config.metric.secondaryColor}
-            fontSize={fontSize}
             trendConfig={
               hasDynamicColoring && trendConfig
                 ? { ...trendConfig, borderColor: color }
@@ -212,14 +211,13 @@ export const MetricVis = ({
       title: String(title),
       subtitle,
       icon: config.metric?.icon ? getIcon(config.metric?.icon) : undefined,
-      extra: ({ fontSize, color }) => (
+      extra: ({ color }) => (
         <SecondaryMetric
           row={row}
           config={config}
           columns={data.columns}
           getMetricFormatter={getMetricFormatter}
           color={config.metric.secondaryColor}
-          fontSize={fontSize}
           trendConfig={
             hasDynamicColoring && trendConfig ? { ...trendConfig, borderColor: color } : trendConfig
           }
diff --git a/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/secondary_metric.test.tsx b/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/secondary_metric.test.tsx
@@ -43,7 +43,6 @@ function renderSecondaryMetric(props: Partial<SecondaryMetricProps> = {}) {
         metric: { secondaryPrefix: '' },
       }}
       getMetricFormatter={jest.fn(() => () => formattedValue)}
-      fontSize={16}
       {...props}
     />
   );
@@ -257,9 +256,10 @@ describe('Secondary metric', () => {
         it.each(trendCombinationCompareToPrimary)(
           '[Compare to primary] should render a badge with the trend icon "$icon" and the formatted value (rawValue: $valueFinite, baseline: $baselineFinite)',
           ({ value, baseline, color, icon }) => {
+            const getMetricFormatterMock = jest.fn(() => (v: unknown) => String(v));
             renderSecondaryMetric({
               row: { [id]: value },
-              getMetricFormatter: jest.fn(() => (v: unknown) => String(v)),
+              getMetricFormatter: getMetricFormatterMock,
               trendConfig: {
                 icon: showIcon,
                 value: showValue,
@@ -280,6 +280,13 @@ describe('Secondary metric', () => {
               const el = screen.getByTitle(badgeText);
               expect(el).toBeInTheDocument();
               expect(el).toHaveStyle(`--euiBadgeBackgroundColor: ${color}`);
+              expect(getMetricFormatterMock).toHaveBeenCalledWith(
+                expect.any(String),
+                expect.any(Object),
+                expect.objectContaining({
+                  number: expect.objectContaining({ alwaysShowSign: true }),
+                })
+              );
             }
             if (showValue) {
               expect(
diff --git a/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/secondary_metric.tsx b/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/secondary_metric.tsx
@@ -15,6 +15,7 @@ import type { DatatableColumn, DatatableRow } from '@kbn/expressions-plugin/comm
 import { type FieldFormatConvertFunction } from '@kbn/field-formats-plugin/common';
 import { type VisParams } from '@kbn/visualizations-plugin/common';
 import { getColumnByAccessor } from '@kbn/visualizations-plugin/common/utils';
+import { FormatOverrides } from './helpers';
 
 export interface TrendConfig {
   icon: boolean;
@@ -119,14 +120,12 @@ function SecondaryMetricValue({
   formattedValue,
   trendConfig,
   color,
-  fontSize,
   formatter,
 }: {
   rawValue?: number | string;
   formattedValue?: string;
   trendConfig?: TrendConfig;
   color?: string;
-  fontSize: number;
   formatter?: FieldFormatConvertFunction;
 }) {
   const safeFormattedValue = formattedValue ?? notAvailable;
@@ -152,6 +151,15 @@ function SecondaryMetricValue({
       formatter,
       trendConfig.compareToPrimary
     );
+    // If no value is shown and no icon should be shown (i.e. N/A) then do not render the badge at all
+    if (trendConfig.icon && !trendConfig.value && !icon) {
+      return (
+        <span
+          data-test-subj={`expressionMetricVis-secondaryMetric-badge-${rawValue}`}
+          aria-label={getTrendDescription(trendConfig.value, icon != null, valueToShow, iconLabel)}
+        />
+      );
+    }
     return (
       <EuiBadge
         aria-label={
@@ -189,21 +197,43 @@ export interface SecondaryMetricProps {
   config: Pick<VisParams, 'metric' | 'dimensions'>;
   trendConfig?: TrendConfig;
   color?: string;
-  fontSize: number;
-  getMetricFormatter: (accessor: string, columns: DatatableColumn[]) => FieldFormatConvertFunction;
+  getMetricFormatter: (
+    accessor: string,
+    columns: DatatableColumn[],
+    formatOverrides?: FormatOverrides | undefined
+  ) => FieldFormatConvertFunction;
 }
 
 function getMetricColumnAndFormatter(
   columns: SecondaryMetricProps['columns'],
   config: SecondaryMetricProps['config'],
-  getMetricFormatter: SecondaryMetricProps['getMetricFormatter']
+  getMetricFormatter: SecondaryMetricProps['getMetricFormatter'],
+  formatOverrides: FormatOverrides | undefined
 ) {
   if (!config.dimensions.secondaryMetric) {
     return;
   }
   return {
     metricColumn: getColumnByAccessor(config.dimensions.secondaryMetric, columns),
-    metricFormatter: getMetricFormatter(config.dimensions.secondaryMetric, columns),
+    metricFormatter: getMetricFormatter(
+      config.dimensions.secondaryMetric,
+      columns,
+      formatOverrides
+    ),
+  };
+}
+
+function getEnhancedNumberSignFormatter(
+  trendConfig: TrendConfig | undefined
+): FormatOverrides | undefined {
+  if (!trendConfig?.compareToPrimary) {
+    return;
+  }
+  const paramsOverride = { alwaysShowSign: true };
+  return {
+    number: paramsOverride,
+    percent: paramsOverride,
+    bytes: paramsOverride,
   };
 }
 
@@ -214,10 +244,14 @@ export function SecondaryMetric({
   getMetricFormatter,
   trendConfig,
   color,
-  fontSize,
 }: SecondaryMetricProps) {
   const { metricFormatter, metricColumn } =
-    getMetricColumnAndFormatter(columns, config, getMetricFormatter) || {};
+    getMetricColumnAndFormatter(
+      columns,
+      config,
+      getMetricFormatter,
+      getEnhancedNumberSignFormatter(trendConfig)
+    ) || {};
   const prefix = config.metric.secondaryPrefix ?? metricColumn?.name;
   const value = metricColumn ? row[metricColumn.id] : undefined;
 
@@ -230,7 +264,6 @@ export function SecondaryMetric({
         formattedValue={metricFormatter?.(value)}
         trendConfig={color ? undefined : trendConfig}
         color={color}
-        fontSize={fontSize}
         formatter={metricFormatter}
       />
     </span>
diff --git a/src/platform/plugins/shared/field_formats/common/converters/numeral.ts b/src/platform/plugins/shared/field_formats/common/converters/numeral.ts
@@ -30,7 +30,9 @@ export abstract class NumeralFormat extends FieldFormat {
   abstract title: string;
 
   getParamDefaults = () => ({
-    pattern: this.getConfig!(`format:${this.id}:defaultPattern`),
+    // While this should be always defined, it is not guaranteed in testing that the function is available
+    pattern: this.getConfig?.(`format:${this.id}:defaultPattern`),
+    alwaysShowSign: false,
   });
 
   protected getConvertedValue(val: number | string | object): string {
@@ -50,7 +52,12 @@ export abstract class NumeralFormat extends FieldFormat {
       (this.getConfig && this.getConfig(FORMATS_UI_SETTINGS.FORMAT_NUMBER_DEFAULT_LOCALE)) || 'en';
     numeral.language(defaultLocale);
 
-    const formatted = numeralInst.set(val).format(this.param('pattern'));
+    let pattern: string = this.param('pattern');
+    if (pattern && this.param('alwaysShowSign')) {
+      pattern = pattern.startsWith('+') || val === 0 ? pattern : `+ ${pattern}`;
+    }
+
+    const formatted = numeralInst.set(val).format(pattern);
 
     numeral.language(previousLocale);
 
diff --git a/src/platform/plugins/shared/field_formats/common/converters/percent.ts b/src/platform/plugins/shared/field_formats/common/converters/percent.ts
@@ -26,6 +26,7 @@ export class PercentFormat extends NumeralFormat {
   getParamDefaults = () => ({
     pattern: this.getConfig!(FORMATS_UI_SETTINGS.FORMAT_PERCENT_DEFAULT_PATTERN),
     fractional: true,
+    alwaysShowSign: false,
   });
 
   textConvert: TextContextTypeConvert = (val: string | number) => {
