From c03768728e2e4506e98380771a13abfa6463aae5 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Jun 2025 15:24:11 +0200 Subject: [PATCH 1/7] :fire: Remove unused prop --- .../expression_metric/public/components/metric_vis.tsx | 6 ++---- .../public/components/secondary_metric.tsx | 4 ---- 2 files changed, 2 insertions(+), 8 deletions(-) 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 index f1870105a26d2..0a58231b2b517 100644 --- 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 }) => ( ( + extra: ({ color }) => ( From ac14ba1fafaa8b88a19b373579a487462ec9fb6b Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Jun 2025 15:24:25 +0200 Subject: [PATCH 2/7] :bug: Fix warning issue --- .../public/components/metric_vis.test.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) 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 index bee7ea66d16f7..3ad2782776881 100644 --- 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()); } From fd4de74fa0309250f383044060bec443617dafa0 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Jun 2025 15:24:47 +0200 Subject: [PATCH 3/7] :sparkles: Introduce new sign flag for numeric formats --- .../shared/field_formats/common/converters/numeral.ts | 8 +++++++- .../shared/field_formats/common/converters/percent.ts | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/platform/plugins/shared/field_formats/common/converters/numeral.ts b/src/platform/plugins/shared/field_formats/common/converters/numeral.ts index 100898fd558b6..438f53bd916ee 100644 --- a/src/platform/plugins/shared/field_formats/common/converters/numeral.ts +++ b/src/platform/plugins/shared/field_formats/common/converters/numeral.ts @@ -31,6 +31,7 @@ export abstract class NumeralFormat extends FieldFormat { getParamDefaults = () => ({ pattern: this.getConfig!(`format:${this.id}:defaultPattern`), + alwaysShowSign: false, }); protected getConvertedValue(val: number | string | object): string { @@ -50,7 +51,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 = this.param('pattern'); + if (this.param('alwaysShowSign')) { + pattern = /^\+/.test(pattern) || 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 index c091ba1866d5d..43cbd5e1216e3 100644 --- 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) => { From 32ebfcce56287386f9c2f2da9fa744345f489e4b Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Jun 2025 15:25:06 +0200 Subject: [PATCH 4/7] :sparkles: Adopt the new sign flag for primary delta values --- .../public/components/helpers.ts | 26 ++++++++-- .../public/components/secondary_metric.tsx | 47 +++++++++++++++++-- 2 files changed, 65 insertions(+), 8 deletions(-) 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 index 21d13655421e0..4b0df91dead5f 100644 --- 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/secondary_metric.tsx b/src/platform/plugins/shared/chart_expressions/expression_metric/public/components/secondary_metric.tsx index 9eacf88067ccf..55bae8bbf5bde 100644 --- 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; @@ -150,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 ( + + ); + } return ( ; 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,7 +246,12 @@ export function SecondaryMetric({ color, }: 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; From a670cd49970a79f52a979ca609f89ad0c0a2eef6 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 5 Jun 2025 15:29:44 +0200 Subject: [PATCH 5/7] :white_check_mark: Fix tests for new feature --- .../public/components/secondary_metric.test.tsx | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) 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 index ab0cd0ef52615..c8643b77ea3d3 100644 --- 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 = {}) { 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( From 6297b617dffd3648cc4187ec426ccdce1dbe89d4 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 6 Jun 2025 11:28:34 +0200 Subject: [PATCH 6/7] :bug: Fix testing issue --- .../shared/field_formats/common/converters/numeral.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/platform/plugins/shared/field_formats/common/converters/numeral.ts b/src/platform/plugins/shared/field_formats/common/converters/numeral.ts index 438f53bd916ee..3da2be6d9da17 100644 --- 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,8 @@ 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, }); @@ -51,9 +52,9 @@ export abstract class NumeralFormat extends FieldFormat { (this.getConfig && this.getConfig(FORMATS_UI_SETTINGS.FORMAT_NUMBER_DEFAULT_LOCALE)) || 'en'; numeral.language(defaultLocale); - let pattern = this.param('pattern'); + let pattern: string = this.param('pattern'); if (this.param('alwaysShowSign')) { - pattern = /^\+/.test(pattern) || val === 0 ? pattern : `+ ${pattern}`; + pattern = pattern.startsWith('+') || val === 0 ? pattern : `+ ${pattern}`; } const formatted = numeralInst.set(val).format(pattern); From 9430cfc5131f81d29a687b54e4a6ec6ca35f4c99 Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Fri, 6 Jun 2025 11:41:08 +0200 Subject: [PATCH 7/7] Update src/platform/plugins/shared/field_formats/common/converters/numeral.ts --- .../plugins/shared/field_formats/common/converters/numeral.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/plugins/shared/field_formats/common/converters/numeral.ts b/src/platform/plugins/shared/field_formats/common/converters/numeral.ts index 3da2be6d9da17..6c74153fca36b 100644 --- a/src/platform/plugins/shared/field_formats/common/converters/numeral.ts +++ b/src/platform/plugins/shared/field_formats/common/converters/numeral.ts @@ -53,7 +53,7 @@ export abstract class NumeralFormat extends FieldFormat { numeral.language(defaultLocale); let pattern: string = this.param('pattern'); - if (this.param('alwaysShowSign')) { + if (pattern && this.param('alwaysShowSign')) { pattern = pattern.startsWith('+') || val === 0 ? pattern : `+ ${pattern}`; }