Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions component/src/charts/__tests__/base-chart.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ vi.mock("echarts/components", () => ({
DataZoomComponent: vi.fn(),
AriaComponent: vi.fn(),
RadarComponent: vi.fn(),
MarkLineComponent: vi.fn(),
}));

describe("BaseChart", () => {
Expand Down
47 changes: 47 additions & 0 deletions component/src/charts/__tests__/reference-line.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { describe, it, expect } from "vitest";
import { parseReferenceLines } from "../chart-utils";
import type { ReferenceLine } from "../chart-utils";

describe("parseReferenceLines", () => {
it("returns empty array for undefined input", () => {
expect(parseReferenceLines(undefined)).toEqual([]);
});

it("returns empty array for empty string", () => {
expect(parseReferenceLines("")).toEqual([]);
});

it("parses a single horizontal reference line", () => {
const input = JSON.stringify([{ value: 50, label: "Target", color: "#ff0000" }]);
const result = parseReferenceLines(input);
expect(result).toHaveLength(1);
expect(result[0]).toEqual({ value: 50, label: "Target", color: "#ff0000" });
});

it("parses multiple reference lines", () => {
const input = JSON.stringify([
{ value: 25, label: "Low" },
{ value: 75, label: "High", color: "#00ff00" },
]);
const result = parseReferenceLines(input);
expect(result).toHaveLength(2);
});

it("returns empty array for invalid JSON", () => {
expect(parseReferenceLines("not-json")).toEqual([]);
});

it("filters out entries without a value", () => {
const input = JSON.stringify([{ label: "No value" }, { value: 50, label: "OK" }]);
const result = parseReferenceLines(input);
expect(result).toHaveLength(1);
expect(result[0].value).toBe(50);
});
});

describe("ReferenceLine type", () => {
it("accepts minimal reference line", () => {
const line: ReferenceLine = { value: 100 };
expect(line.value).toBe(100);
});
});
13 changes: 11 additions & 2 deletions component/src/charts/bar-chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import {
resolveShowLegend,
buildCompactGrid,
resolveItemColor,
parseReferenceLines,
buildMarkLineFromRefs,
} from "./chart-utils";
import { parseColorThresholds } from "./color-threshold";
import type { StylingRule } from "./styling-rule";
Expand All @@ -34,6 +36,8 @@ export interface BarChartProps extends Omit<BaseChartProps, "options"> {
xAxisLabel?: string;
/** Y-axis name label */
yAxisLabel?: string;
/** JSON string of reference lines: [{ value, label?, color? }] */
referenceLines?: string;
Comment on lines +39 to +40

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Issue #136 requires vertical lines support.

The acceptance criteria states "Support both horizontal and vertical lines." Current implementation only supports horizontal lines (yAxis positioning in buildMarkLineFromRefs).

Consider adding an axis or direction field to ReferenceLine to support xAxis positioning for vertical lines.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@component/src/charts/bar-chart.tsx` around lines 39 - 40, The current
referenceLines prop only produces horizontal marks via yAxis in
buildMarkLineFromRefs; add an axis (or direction) field to the ReferenceLine
shape (accepting 'x'|'y' or 'vertical'|'horizontal') and update
buildMarkLineFromRefs to branch on that field: when axis === 'y' keep the
existing yAxis positioning for horizontal lines, and when axis === 'x' generate
a mark line positioned on xAxis (vertical line) using the provided value and
label/color handling; also validate/backward-compatibly default missing axis to
'y' so existing JSON strings still render as horizontal lines. Ensure you
reference the referenceLines prop parsing and the buildMarkLineFromRefs function
to locate and modify the logic.

/** @deprecated Use stylingRules instead. JSON string of thresholds for per-bar coloring */
colorThresholds?: string;
/** Rule-based styling rules */
Expand Down Expand Up @@ -62,6 +66,7 @@ function BarChart({
showGridLines = true,
xAxisLabel,
yAxisLabel,
referenceLines: referenceLinesJson,
colorThresholds,
stylingRules,
paramValues,
Expand All @@ -79,6 +84,8 @@ function BarChart({
const effectiveShowValues = compact ? false : showValues;
const effectiveBarWidth = barWidth > 0 ? barWidth : undefined;
const thresholds = stylingRules ? [] : parseColorThresholds(colorThresholds ?? "");
const refLines = parseReferenceLines(referenceLinesJson);
const markLine = buildMarkLineFromRefs(refLines);

const categoryAxis = {
type: "category" as const,
Expand All @@ -103,7 +110,7 @@ function BarChart({
grid: buildCompactGrid(compact, effectiveShowLegend),
xAxis: isHorizontal ? valueAxis : categoryAxis,
yAxis: isHorizontal ? categoryAxis : valueAxis,
series: seriesKeys.map((key) => ({
series: seriesKeys.map((key, idx) => ({
name: key,
type: "bar" as const,
data: data.map((d) => {
Expand All @@ -121,9 +128,11 @@ function BarChart({
? { show: true, position: isHorizontal ? ("right" as const) : ("top" as const) }
: undefined,
emphasis: seriesKeys.length > 1 ? { focus: "series" as const } : {},
// Attach reference lines to the first series only
...(idx === 0 && markLine ? { markLine } : {}),
})),
};
}, [data, orientation, stacked, showValues, showLegend, barWidth, barGap, showGridLines, xAxisLabel, yAxisLabel, colorThresholds, stylingRules, paramValues, compact, hideLegend]);
}, [data, orientation, stacked, showValues, showLegend, barWidth, barGap, showGridLines, xAxisLabel, yAxisLabel, referenceLinesJson, colorThresholds, stylingRules, paramValues, compact, hideLegend]);

return (
<div ref={containerRef} className="h-full w-full">
Expand Down
2 changes: 2 additions & 0 deletions component/src/charts/base-chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
DataZoomComponent,
AriaComponent,
RadarComponent,
MarkLineComponent,
} from "echarts/components";
import { CanvasRenderer } from "echarts/renderers";
import type { EChartsOption } from "echarts";
Expand All @@ -35,6 +36,7 @@ echarts.use([
DataZoomComponent,
AriaComponent,
RadarComponent,
MarkLineComponent,
CanvasRenderer,
]);

Expand Down
53 changes: 53 additions & 0 deletions component/src/charts/chart-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,59 @@ import { resolveThresholdColor } from "./color-threshold";
import type { StylingRule } from "./styling-rule";
import { resolveStylingRuleColor } from "./styling-rule";

// ---------------------------------------------------------------------------
// Reference lines (markLine)
// ---------------------------------------------------------------------------

export interface ReferenceLine {
value: number;
label?: string;
color?: string;
}

/**
* Parse a JSON string of reference lines. Returns empty array on
* invalid input or missing values.
*/
export function parseReferenceLines(input: string | undefined): ReferenceLine[] {
if (!input) return [];
try {
const parsed = JSON.parse(input);
if (!Array.isArray(parsed)) return [];
return parsed.filter(
(item: unknown): item is ReferenceLine =>
typeof item === "object" &&
item !== null &&
"value" in item &&
typeof (item as ReferenceLine).value === "number",
);
} catch {
return [];
}
}

/**
* Build ECharts markLine data from reference lines.
*/
export function buildMarkLineFromRefs(lines: ReferenceLine[]) {
if (!lines.length) return undefined;
return {
silent: true,
symbol: "none",
data: lines.map((line) => ({
yAxis: line.value,
label: {
formatter: line.label ?? String(line.value),
position: "insideEndTop" as const,
},
lineStyle: {
color: line.color ?? "#888",
type: "dashed" as const,
},
})),
};
}

/** Detect whether the document is currently in dark mode. */
export function isDark(): boolean {
if (typeof document === "undefined") return false;
Expand Down
2 changes: 2 additions & 0 deletions component/src/components/composed/chart-options-schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const barOptions: ChartOptionDef[] = [
{ key: "xAxisLabel", label: "X-Axis Label", type: "text", default: "", category: "Labels", description: "Custom label displayed below the horizontal axis." },
{ key: "yAxisLabel", label: "Y-Axis Label", type: "text", default: "", category: "Labels", description: "Custom label displayed beside the vertical axis." },
{ key: "showGridLines", label: "Show Grid Lines", type: "boolean", default: true, category: "Style", description: "Show faint horizontal reference lines behind the bars." },
{ key: "referenceLines", label: "Reference Lines (JSON)", type: "text", default: "", category: "Annotations", description: 'Horizontal reference lines as JSON: [{"value":50,"label":"Target","color":"#ff0000"}]' },
];

const lineOptions: ChartOptionDef[] = [
Expand All @@ -45,6 +46,7 @@ const lineOptions: ChartOptionDef[] = [
{ key: "xAxisLabel", label: "X-Axis Label", type: "text", default: "", category: "Labels", description: "Custom label displayed below the horizontal axis." },
{ key: "yAxisLabel", label: "Y-Axis Label", type: "text", default: "", category: "Labels", description: "Custom label displayed beside the vertical axis." },
{ key: "showLegend", label: "Show Legend", type: "boolean", default: true, category: "Labels", description: "Show the chart legend identifying each data series." },
{ key: "referenceLines", label: "Reference Lines (JSON)", type: "text", default: "", category: "Annotations", description: 'Horizontal reference lines as JSON: [{"value":50,"label":"Target","color":"#ff0000"}]' },
Comment thread
coderabbitai[bot] marked this conversation as resolved.
];

const pieOptions: ChartOptionDef[] = [
Expand Down
1 change: 1 addition & 0 deletions component/vitest.setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ vi.mock("echarts/components", () => ({
DataZoomComponent: vi.fn(),
AriaComponent: vi.fn(),
RadarComponent: vi.fn(),
MarkLineComponent: vi.fn(),
}));

vi.mock("echarts/renderers", () => ({
Expand Down
Loading