diff --git a/static/app/components/charts/chartLegend.spec.tsx b/static/app/components/charts/chartLegend.spec.tsx index 9f2fa25c99cd..f1485ad0d941 100644 --- a/static/app/components/charts/chartLegend.spec.tsx +++ b/static/app/components/charts/chartLegend.spec.tsx @@ -65,6 +65,67 @@ describe('ChartLegend', () => { expect(container).toBeEmptyDOMElement(); }); + it('includes the suffix in accessible labels and preserves selection keys', async () => { + jest.spyOn(HTMLElement.prototype, 'clientWidth', 'get').mockReturnValue(1000); + const onSelectionChange = jest.fn(); + render( + + ); + + await userEvent.click( + screen.getByRole('button', { + name: 'Toggle Duration · fill(linear) → smooth(sma,5)', + }) + ); + + expect(onSelectionChange).toHaveBeenCalledWith({'transformed-series': false}); + expect( + screen.getByRole('checkbox', { + name: 'Duration · fill(linear) → smooth(sma,5)', + }) + ).toBeChecked(); + }); + + it('includes suffixes in overflow options', async () => { + jest.spyOn(HTMLElement.prototype, 'clientWidth', 'get').mockReturnValue(100); + jest + .spyOn(HTMLElement.prototype, 'getBoundingClientRect') + .mockReturnValue({width: 80} as DOMRect); + const onSelectionChange = jest.fn(); + render( + + ); + + await userEvent.click(screen.getByRole('button', {name: '+2 more'})); + await userEvent.click(screen.getByRole('option', {name: 'Duration · smooth(sma,5)'})); + + expect(onSelectionChange).toHaveBeenCalledWith({original: true, smoothed: false}); + }); + it('toggles item selection when clicked', async () => { mockDimensions(1000); const onSelectionChange = jest.fn(); diff --git a/static/app/components/charts/chartLegend.tsx b/static/app/components/charts/chartLegend.tsx index d57bbb4bf822..9275cfb5b50d 100644 --- a/static/app/components/charts/chartLegend.tsx +++ b/static/app/components/charts/chartLegend.tsx @@ -19,6 +19,7 @@ export interface LegendItem { color: string; label: string; name: string; + suffix?: string; } interface ChartLegendProps { @@ -156,7 +157,19 @@ export function ChartLegend({ // No `size` here, unlike the row: the menu sets its own option type, and // overriding it would leave this one menu's labels smaller than every // other one in the app. - label: truncateMenuLabels ? ( + label: item.suffix ? ( + + + {item.label} + + + · {item.suffix} + + + ) : truncateMenuLabels ? ( {item.label} @@ -165,7 +178,7 @@ export function ChartLegend({ ), // React-aria needs the plain string once the label stops being one, or // typeahead and screen readers lose the option's name. - textValue: item.label, + textValue: getLegendItemText(item), // Suppress the built-in checkmark; we render a custom LegendCheckbox via leadingItems hideCheck: true, leadingItems: renderLeadingCheckbox(item), @@ -216,18 +229,23 @@ export function ChartLegend({ }} onClick={() => toggleItem(item.name)} role="button" - aria-label={t('Toggle %s', item.label)} + aria-label={t('Toggle %s', getLegendItemText(item))} > {}} - aria-label={item.label} + aria-label={getLegendItemText(item)} /> {item.label} + {item.suffix && ( + + · {item.suffix} + + )} ))} @@ -260,6 +278,10 @@ export function ChartLegend({ ); } +function getLegendItemText(item: LegendItem): string { + return item.suffix ? `${item.label} · ${item.suffix}` : item.label; +} + /** * Renders a leading checkbox for a legend item inside the overflow dropdown. * Stateless — only depends on its arguments, so it lives outside the component. @@ -271,7 +293,7 @@ function renderLeadingCheckbox(item: LegendItem) { color={item.color} checked={isSelected} onChange={() => {}} - aria-label={item.label} + aria-label={getLegendItemText(item)} /> ); }; diff --git a/static/app/views/dashboards/widgetBuilder/components/widgetPreview.spec.tsx b/static/app/views/dashboards/widgetBuilder/components/widgetPreview.spec.tsx index 88c863fb9a21..94fe0e4d6cc9 100644 --- a/static/app/views/dashboards/widgetBuilder/components/widgetPreview.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/widgetPreview.spec.tsx @@ -119,9 +119,12 @@ describe('WidgetPreview', () => { }) ); }); - expect(await screen.findAllByLabelText('Toggle p90(span.duration)')).toHaveLength( - 2 - ); + expect( + await screen.findByLabelText('Toggle p90(span.duration)') + ).toBeInTheDocument(); + expect( + await screen.findByLabelText('Toggle p90(span.duration) · smooth(sma,5)') + ).toBeInTheDocument(); } ); }); diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesTransformations.spec.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesTransformations.spec.tsx new file mode 100644 index 000000000000..fa2e8c4181a8 --- /dev/null +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesTransformations.spec.tsx @@ -0,0 +1,30 @@ +import {formatTimeSeriesTransformations} from './formatTimeSeriesTransformations'; + +describe('formatTimeSeriesTransformations', () => { + it.each([ + {transformations: undefined, expected: ''}, + {transformations: [], expected: ''}, + {transformations: ['fill(zero)'], expected: 'fill(zero)'}, + {transformations: ['fill(locf)'], expected: 'fill(locf)'}, + {transformations: ['fill(linear)'], expected: 'fill(linear)'}, + {transformations: ['smooth(sma,5)'], expected: 'smooth(sma,5)'}, + {transformations: ['smooth(ema,10)'], expected: 'smooth(ema,10)'}, + {transformations: ['smooth(ema,10,0.25)'], expected: 'smooth(ema,10,0.25)'}, + {transformations: ['smooth(asap)'], expected: 'smooth(asap)'}, + {transformations: ['smooth(asap,5)'], expected: 'smooth(asap,5)'}, + { + transformations: ['fill(linear)', 'smooth(sma,5)'], + expected: 'fill(linear) → smooth(sma,5)', + }, + { + transformations: ['smooth(sma,5)', 'fill(linear)'], + expected: 'smooth(sma,5) → fill(linear)', + }, + { + transformations: ['smooth(future,5)', 'unsupported'], + expected: 'smooth(future,5) → unsupported', + }, + ])('formats $transformations as $expected', ({transformations, expected}) => { + expect(formatTimeSeriesTransformations(transformations)).toBe(expected); + }); +}); diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesTransformations.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesTransformations.tsx new file mode 100644 index 000000000000..18c5093fb72a --- /dev/null +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesTransformations.tsx @@ -0,0 +1,5 @@ +export function formatTimeSeriesTransformations( + transformations: readonly string[] = [] +): string { + return transformations.join(' → '); +} diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.spec.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.spec.tsx index 433cbf4b993e..d51f2d3aed40 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.spec.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.spec.tsx @@ -6,6 +6,28 @@ import {ContinuousTimeSeries} from './continuousTimeSeries'; import type {Plottable} from './plottable'; describe('ContinuousTimeSeries', () => { + describe('labelSuffix', () => { + it.each([ + {alias: undefined, groupBy: undefined, expectedLabel: 'avg(span.duration)'}, + {alias: 'Duration', groupBy: undefined, expectedLabel: 'Duration'}, + { + alias: undefined, + groupBy: [{key: 'span.op', value: 'db'}], + expectedLabel: 'db', + }, + ])('keeps $expectedLabel separate from transformations', config => { + const series = TimeSeriesFixture({ + yAxis: 'avg(span.duration)', + groupBy: config.groupBy, + requestedTransformations: ['fill(linear)', 'smooth(sma,5)'], + }); + const plottable = new Dots(series, {alias: config.alias}); + + expect(plottable.label).toBe(config.expectedLabel); + expect(plottable.labelSuffix).toBe('fill(linear) → smooth(sma,5)'); + }); + }); + describe('isEmpty', () => { it('marks normal plottables as not empty', () => { const timeSeries = TimeSeriesFixture(); diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.tsx index 7265a6be7d42..72aff59a5184 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/continuousTimeSeries.tsx @@ -12,6 +12,7 @@ import type { } from 'sentry/views/dashboards/widgets/common/types'; import {formatTimeSeriesLabel} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesLabel'; import {formatTimeSeriesName} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesName'; +import {formatTimeSeriesTransformations} from 'sentry/views/dashboards/widgets/timeSeriesWidget/formatters/formatTimeSeriesTransformations'; import {FALLBACK_TYPE} from 'sentry/views/dashboards/widgets/timeSeriesWidget/settings'; import type {PlottableConfidence, PlottableTimeSeriesValueType} from './plottable'; @@ -87,6 +88,10 @@ export abstract class ContinuousTimeSeries< return this.config?.alias ?? formatTimeSeriesLabel(this.timeSeries); } + get labelSuffix(): string { + return formatTimeSeriesTransformations(this.timeSeries.requestedTransformations); + } + get isEmpty(): boolean { return this.timeSeries.values.every(datum => datum.value === null); } diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/plottable.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/plottable.tsx index cb3fe4ced0cf..e44355046788 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/plottable.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/plottables/plottable.tsx @@ -65,6 +65,7 @@ export interface Plottable { * Optional callback to get access to the chart `ref`. Some Plottables implement this to allow dispatching events to the chart */ handleChartRef?: (ref: ReactEchartsRef) => void; + labelSuffix?: string; /** * `TimeSeriesWidgetVisualization` will call this function if the user clicks a point on a series that originated from this plottable. */ diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx index ec45146de208..075fed709295 100644 --- a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx +++ b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx @@ -306,11 +306,8 @@ export function TimeSeriesWidgetVisualization(props: TimeSeriesWidgetVisualizati [paletteSize, theme.chart] ); - // Create a lookup of series names (given to ECharts) to labels (from - // Plottable). This makes it easier to look up alises when rendering tooltips - // and legends - const aliases = Object.fromEntries( - props.plottables.map(plottable => [plottable.name, plottable.label]) + const plottablesByName = Object.fromEntries( + props.plottables.map(plottable => [plottable.name, plottable]) ); // Create tooltip formatter @@ -383,12 +380,16 @@ export function TimeSeriesWidgetVisualization(props: TimeSeriesWidgetVisualizati // seriesName may be HTML-escaped, so we need to unescape it before looking up the alias // to ensure the lookup works correctly for series with characters that are escaped. - const alias = aliases[unescape(seriesName)]; + const plottable = plottablesByName[unescape(seriesName)]; + const alias = plottable?.label; if (alias) { // The alias value comes from `plottable.label` and is not // HTML-escaped. Escape it for safe insertion into raw HTML // tooltip strings. - return escape(truncationFormatter(alias, true, false)); + const label = escape(truncationFormatter(alias, true, false)); + return plottable.labelSuffix + ? `${label} · ${escape(plottable.labelSuffix)}` + : label; } // `seriesName` is already HTML-escaped by `getFormatter`, so // skip escaping to avoid double-encoding (e.g., `>` → `>` @@ -680,6 +681,7 @@ export function TimeSeriesWidgetVisualization(props: TimeSeriesWidgetVisualizati return { name: plottable.name, label: plottable.label, + suffix: plottable.labelSuffix, color, }; });