Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions static/app/components/charts/chartLegend.snapshots.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import {Container} from '@sentry/scraps/layout';

import {ChartLegend} from './chartLegend';

describe('ChartLegend', () => {
it.snapshot.each([false, true])('transformed-long-label-%s', longLabel => (
<Container padding="md" width="720px" background="primary">
<ChartLegend
items={[
{name: 'original', label: 'avg(span.duration)', color: '#7553ff'},
{
name: 'transformed',
label: longLabel
? 'Average duration for database operations grouped by transaction'
: 'avg(span.duration)',
suffix: 'fill(linear) → smooth(sma,5)',
color: '#069fba',
},
]}
selected={{}}
onSelectionChange={() => {}}
/>
</Container>
));
});
61 changes: 61 additions & 0 deletions static/app/components/charts/chartLegend.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(
<ChartLegend
items={[
{
name: 'transformed-series',
label: 'Duration',
suffix: 'fill(linear) → smooth(sma,5)',
color: '#ff0000',
},
]}
selected={{}}
onSelectionChange={onSelectionChange}
/>
);

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(
<ChartLegend
truncateMenuLabels
items={[
{name: 'original', label: 'Duration', color: '#ff0000'},
{
name: 'smoothed',
label: 'Duration',
suffix: 'smooth(sma,5)',
color: '#0000ff',
},
]}
selected={{}}
onSelectionChange={onSelectionChange}
/>
);

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();
Expand Down
32 changes: 27 additions & 5 deletions static/app/components/charts/chartLegend.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface LegendItem {
color: string;
label: string;
name: string;
suffix?: string;
}

interface ChartLegendProps {
Expand Down Expand Up @@ -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 ? (
<Flex align="center" gap="xs" minWidth="0">
<Text
ellipsis={truncateMenuLabels || undefined}
style={truncateMenuLabels ? {maxWidth: MAX_LABEL_WIDTH} : undefined}
>
{item.label}
</Text>
<Text variant="secondary" wrap="nowrap">
· {item.suffix}
</Text>
</Flex>
) : truncateMenuLabels ? (
<Text ellipsis style={{maxWidth: MAX_LABEL_WIDTH}}>
{item.label}
</Text>
Expand All @@ -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),
Expand Down Expand Up @@ -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))}
>
<LegendCheckbox
color={item.color}
checked={selected[item.name] !== false}
onChange={() => {}}
aria-label={item.label}
aria-label={getLegendItemText(item)}
/>

<Text size="xs" ellipsis style={{maxWidth: MAX_LABEL_WIDTH}}>
{item.label}
</Text>
{item.suffix && (
<Text size="xs" variant="secondary" wrap="nowrap">
· {item.suffix}
</Text>
)}
</LegendItemButton>
))}
</Flex>
Expand Down Expand Up @@ -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.
Expand All @@ -271,7 +293,7 @@ function renderLeadingCheckbox(item: LegendItem) {
color={item.color}
checked={isSelected}
onChange={() => {}}
aria-label={item.label}
aria-label={getLegendItemText(item)}
/>
);
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
);
});
Original file line number Diff line number Diff line change
@@ -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);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export function formatTimeSeriesTransformations(
transformations: readonly string[] = []
): string {
return transformations.join(' → ');
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import type {Ref} from 'react';
import {TimeSeriesFixture} from 'sentry-fixture/timeSeries';

import {render, screen, userEvent, within} from 'sentry-test/reactTestingLibrary';

import {Line} from './plottables/line';
import {TimeSeriesWidgetVisualization} from './timeSeriesWidgetVisualization';

jest.mock('echarts-for-react/lib/core', () => {
const {useImperativeHandle, useRef, useState} =
jest.requireActual<typeof import('react')>('react');

return function Chart({
option,
ref,
}: {
option: {
series: Array<{
data: Array<{value: [number, number]}>;
name: string;
type: string;
}>;
tooltip: {formatter: (params: unknown[], ticket: string) => string};
};
ref: Ref<unknown>;
}) {
const elementRef = useRef<HTMLDivElement>(null);
const [tooltip, setTooltip] = useState('');
useImperativeHandle(ref, () => ({ele: elementRef.current}), []);

return (
<div
ref={elementRef}
className="echarts-for-react"
aria-label="Time series chart"
onMouseOver={() => {
const params = option.series.flatMap((series, seriesIndex) => {
const data = series.data[0];
return data
? [
{
data,
value: data.value,
seriesName: series.name,
seriesType: series.type,
seriesIndex,
dataIndex: 0,
marker: '',
},
]
: [];
});
setTooltip(option.tooltip.formatter(params, 'test'));
}}
>
<div role="tooltip" dangerouslySetInnerHTML={{__html: tooltip}} />
</div>
);
};
});

describe('TimeSeriesWidgetVisualization', () => {
afterEach(() => {
jest.restoreAllMocks();
});

it('shows ordered transformation suffixes beside aliases in the tooltip', async () => {
jest.spyOn(HTMLElement.prototype, 'clientWidth', 'get').mockReturnValue(1000);
const original = new Line(TimeSeriesFixture(), {alias: 'Duration'});
const transformed = new Line(
TimeSeriesFixture({
requestedTransformations: ['fill(linear)', 'smooth(sma,5)'],
}),
{alias: 'Duration'}
);
render(<TimeSeriesWidgetVisualization plottables={[original, transformed]} />);

expect(
screen.getByRole('button', {
name: 'Toggle Duration · fill(linear) → smooth(sma,5)',
})
).toBeInTheDocument();
await userEvent.hover(screen.getByLabelText('Time series chart'));

const tooltip = screen.getByRole('tooltip');
expect(tooltip).toHaveTextContent('Duration · fill(linear) → smooth(sma,5)');
expect(within(tooltip).getAllByText('Duration')).toHaveLength(2);
expect(tooltip).toHaveTextContent('7.46K/s');
});

it('escapes alias and suffix text when rendering tooltip HTML', async () => {
const plottable = new Line(
TimeSeriesFixture({requestedTransformations: ['fill(<img src=x>)']}),
{alias: '<b>Duration</b>'}
);
render(<TimeSeriesWidgetVisualization plottables={[plottable]} />);

await userEvent.hover(screen.getByLabelText('Time series chart'));

const tooltip = screen.getByRole('tooltip');
expect(tooltip).toHaveTextContent('<b>Duration</b> · fill(<img src=x>)');
expect(within(tooltip).queryByRole('img')).not.toBeInTheDocument();
});
});
Loading
Loading