diff --git a/.claude/skills/design-review/skill.md b/.claude/skills/design-review/skill.md index 07d40811..32333670 100644 --- a/.claude/skills/design-review/skill.md +++ b/.claude/skills/design-review/skill.md @@ -132,6 +132,13 @@ grid: { left: 16, right: 16, top: 16, bottom: 24, containLabel: true } // Compact mode (container < 300px) grid: { left: 8, right: 8, top: 8, bottom: 8 } +// Compact drops the axis NAME and the value-axis numbers. Category labels +// stay — truncated to 10 chars under 400px — because a chart with no +// category names identifies nothing (#1247). + +// Gridlines: one weight and colour for every cartesian chart, from the +// registered theme (GRID_LINE_COLOR in charts/theme.ts). Charts set only +// `splitLine: { show }` — never `splitLine.lineStyle`. // Legend position legend: { bottom: 0 } // ALWAYS bottom-aligned @@ -146,6 +153,8 @@ tooltip: { trigger: "axis", axisPointer: { type: "shadow" } } - NEVER set chart colors inline — always use `resolveChartColors()`. - NEVER add title inside the chart — widget card header IS the title. - NEVER register additional ECharts themes — use `neoboard-light` / `neoboard-dark` only. +- NEVER set `splitLine.lineStyle` in a chart module — gridline weight belongs to the theme (#1247). +- NEVER let a responsive breakpoint hide category labels — degrade to truncation, not to nothing (#1247). - Dark mode chart colors are DIFFERENT from light mode — this is by design (higher lightness for contrast). ### Graph Chart (NVL) diff --git a/component/src/charts/__tests__/axis-label-utils.test.ts b/component/src/charts/__tests__/axis-label-utils.test.ts index 8058a0a5..fe30dc32 100644 --- a/component/src/charts/__tests__/axis-label-utils.test.ts +++ b/component/src/charts/__tests__/axis-label-utils.test.ts @@ -48,9 +48,22 @@ describe("buildCategoryAxisLabel", () => { expect(result.tooltip).toEqual({ show: true }); }); - it("returns show: false when compact is true", () => { - const result = buildCategoryAxisLabel(10, { compact: true }); - expect(result.show).toBe(false); + it("keeps labels in compact mode, truncated not hidden (#1247)", () => { + const result = buildCategoryAxisLabel(4, { + containerWidth: 280, + }); + expect(result.show).toBe(true); + const fmt = result.formatter as (value: string) => string; + expect(fmt("Electronics")).toBe("Electroni…"); + expect(fmt("Home")).toBe("Home"); + }); + + it("keeps common-prefix labels distinguishable in a compact container", () => { + const result = buildCategoryAxisLabel(7, { + containerWidth: 280, + }); + const fmt = result.formatter as (value: string) => string; + expect(new Set(["Widget A", "Widget G"].map(fmt)).size).toBe(2); }); it("normalizes -1 sentinel to automatic rotation", () => { diff --git a/component/src/charts/__tests__/cartesian-axis-defaults.test.tsx b/component/src/charts/__tests__/cartesian-axis-defaults.test.tsx new file mode 100644 index 00000000..33ee61e5 --- /dev/null +++ b/component/src/charts/__tests__/cartesian-axis-defaults.test.tsx @@ -0,0 +1,168 @@ +import { render } from "@testing-library/react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { BarChart } from "../bar-chart"; +import { LineChart } from "../line-chart"; +import { GanttChart } from "../gantt-chart"; +import { registerNeoboardThemes, GRID_LINE_COLOR } from "../theme"; + +/** + * Cross-chart axis and gridline defaults (#1247). + * + * Two failures this pins down: + * 1. Gridline weight must come from the registered theme only — a chart that + * declares its own splitLine.lineStyle makes two widgets on one dashboard + * look like they came from different tools. + * 2. A compact container (< 300px) may drop value numbers, but never the + * category identification — four unlabelled bars are not readable data. + */ + +const mockSetOption = vi.fn(); + +const size = vi.hoisted(() => ({ width: 600, height: 400 })); + +vi.mock("@/hooks/useContainerSize", () => ({ + useContainerSize: () => ({ + width: size.width, + height: size.height, + containerRef: vi.fn(), + }), +})); + +vi.mock("echarts/core", () => { + const use = vi.fn(); + const init = vi.fn(() => ({ + setOption: mockSetOption, + resize: vi.fn(), + dispose: vi.fn(), + on: vi.fn(), + off: vi.fn(), + showLoading: vi.fn(), + hideLoading: vi.fn(), + })); + const registerTheme = vi.fn(); + return { use, init, registerTheme, default: { use, init, registerTheme } }; +}); + +type AxisOption = { + type?: string; + axisLabel?: { show?: boolean; formatter?: (v: string) => string }; + splitLine?: { show?: boolean; lineStyle?: unknown }; +}; + +/** Render a chart and return the flattened x/y axis options it emitted. */ +function axesOf(ui: React.ReactElement): AxisOption[] { + render(ui); + const opts = mockSetOption.mock.calls[0][0] as { + xAxis?: AxisOption | AxisOption[]; + yAxis?: AxisOption | AxisOption[]; + }; + return [opts.xAxis, opts.yAxis].flat().filter(Boolean) as AxisOption[]; +} + +const categoryAxis = (axes: AxisOption[]) => + axes.find((a) => a.type === "category") as AxisOption; +const valueAxis = (axes: AxisOption[]) => + axes.find((a) => a.type === "value") as AxisOption; + +const barData = [ + { label: "Electronics & Media", value: 100 }, + { label: "Home", value: 200 }, + { label: "Garden", value: 150 }, + { label: "Toys", value: 90 }, +]; + +const lineData = [ + { x: "Jan", value: 10 }, + { x: "Feb", value: 20 }, + { x: "Mar", value: 15 }, +]; + +const ganttData = [ + { task: "Design", start: 1700000000000, end: 1700500000000 }, + { task: "Build", start: 1700500000000, end: 1701000000000 }, +]; + +describe("cartesian gridlines come from the theme (#1247)", () => { + beforeEach(() => { + vi.clearAllMocks(); + size.width = 600; + size.height = 400; + }); + + it.each([ + ["bar", ], + ["line", ], + ["gantt", ], + ])("%s chart declares no gridline style of its own", (_name, ui) => { + for (const axis of axesOf(ui)) { + expect(axis.splitLine?.lineStyle).toBeUndefined(); + } + }); + + it.each(["neoboard-light", "neoboard-dark"] as const)( + "%s registers one gridline colour for category and value axes", + (name) => { + const themes: Record> = {}; + registerNeoboardThemes((themeName, theme) => { + themes[themeName] = theme as Record; + }); + const theme = themes[name] as unknown as Record< + string, + { splitLine: { lineStyle: { color: string } } } + >; + const expected = + GRID_LINE_COLOR[name === "neoboard-dark" ? "dark" : "light"]; + // timeAxis included: a time-series line chart and the gantt draw + // ECharts' un-themed light-grey grid without it — glaring in dark. + for (const axis of ["categoryAxis", "valueAxis", "timeAxis"]) { + expect(theme[axis].splitLine.lineStyle.color).toBe(expected); + } + }, + ); +}); + +describe("compact containers keep category identification (#1247)", () => { + beforeEach(() => { + vi.clearAllMocks(); + size.width = 280; + size.height = 400; + }); + + it("bar chart still labels its categories", () => { + const axis = categoryAxis(axesOf()); + expect(axis.axisLabel?.show).toBe(true); + }); + + it("bar chart truncates rather than dropping labels", () => { + const axis = categoryAxis(axesOf()); + const formatter = axis.axisLabel?.formatter as (v: string) => string; + expect(formatter("Electronics & Media")).toBe("Electroni…"); + expect(formatter("Home")).toBe("Home"); + }); + + it("bar chart still drops the value numbers", () => { + const axis = valueAxis(axesOf()); + expect(axis.axisLabel?.show).toBe(false); + }); + + it("line chart truncates long category labels like the bar chart does", () => { + const axis = categoryAxis( + axesOf( + , + ), + ); + const formatter = axis.axisLabel?.formatter as (v: string) => string; + expect(formatter("Electronics & Media")).toBe("Electroni\u2026"); + }); + + it("line chart still labels its x axis", () => { + const axes = axesOf(); + expect(categoryAxis(axes).axisLabel?.show).toBe(true); + expect(valueAxis(axes).axisLabel?.show).toBe(false); + }); +}); diff --git a/component/src/charts/bar-chart.tsx b/component/src/charts/bar-chart.tsx index a01cd018..383a6103 100644 --- a/component/src/charts/bar-chart.tsx +++ b/component/src/charts/bar-chart.tsx @@ -136,7 +136,6 @@ function BarChart({ const categoryLabels = data.map((d) => d.label); const axisLabelConfig = buildCategoryAxisLabel(categoryLabels.length, { - compact, rotateOverride: axisLabelRotation, containerWidth: width, }); diff --git a/component/src/charts/chart-utils.ts b/component/src/charts/chart-utils.ts index 1ef47a1b..b17dc3e3 100644 --- a/component/src/charts/chart-utils.ts +++ b/component/src/charts/chart-utils.ts @@ -280,8 +280,6 @@ export interface CategoryAxisLabelOptions { rotateOverride?: number; /** Maximum label length before truncation (default: 15). */ maxLabelLength?: number; - /** Whether the chart is in compact mode (hides labels). */ - compact?: boolean; /** Container width in pixels — used for width-based auto-rotation. */ containerWidth?: number; } @@ -299,6 +297,8 @@ export interface CategoryAxisLabelConfig { * - 15+ categories: rotate 45° * - Labels longer than maxLabelLength are truncated with ellipsis (U+2026) * - ECharts axisPointer tooltip shows the full text on hover + * - Category labels are never hidden: a compact container drops the value + * axis, since a chart with no category names identifies nothing (#1247) * * A `rotateOverride` of -1 is the "automatic" sentinel from the UI and is * normalized to undefined so the category-count heuristic applies. @@ -307,7 +307,7 @@ export function buildCategoryAxisLabel( categoryCount: number, options: CategoryAxisLabelOptions = {}, ): CategoryAxisLabelConfig { - const { maxLabelLength = 15, compact = false, containerWidth } = options; + const { maxLabelLength = 15, containerWidth } = options; // Normalize -1 sentinel (automatic mode) to undefined so ECharts uses its // default auto-rotation instead of receiving an invalid rotate: -1. const rotateOverride = @@ -337,7 +337,10 @@ export function buildCategoryAxisLabel( rotate = 0; } - // Width-aware truncation: tighter limit in narrow containers + // Width-aware truncation: tighter limit in narrow containers. A compact + // container is by definition under 400px, so it already gets the tight + // budget — going tighter still collapses common-prefix labels + // ("Widget A".."Widget G") into seven identical stubs (#1247). const effectiveMaxLength = containerWidth && containerWidth < 400 ? Math.min(maxLabelLength, 10) @@ -353,7 +356,7 @@ export function buildCategoryAxisLabel( : undefined; return { - show: !compact, + show: true, rotate, formatter, tooltip: { show: true }, diff --git a/component/src/charts/gantt-chart.tsx b/component/src/charts/gantt-chart.tsx index 47a04760..0d0e0593 100644 --- a/component/src/charts/gantt-chart.tsx +++ b/component/src/charts/gantt-chart.tsx @@ -247,10 +247,9 @@ function GanttChart({ }, xAxis: { type: "time", - splitLine: { - show: showGridLines, - lineStyle: { type: "dashed", opacity: 0.3 }, - }, + // No local lineStyle: the registered theme owns gridline weight and + // colour so every cartesian chart draws the same grid (#1247). + splitLine: { show: showGridLines }, }, yAxis: { type: "category", diff --git a/component/src/charts/line-chart.tsx b/component/src/charts/line-chart.tsx index 43b7dd5e..3dfd09f1 100644 --- a/component/src/charts/line-chart.tsx +++ b/component/src/charts/line-chart.tsx @@ -16,6 +16,7 @@ import { parseReferenceLines, buildMarkLineFromRefs, isTimeSeriesData, + buildCategoryAxisLabel, fadeToTransparent, isDark, } from "./chart-utils"; @@ -245,14 +246,32 @@ function LineChart({ (useDualAxis && !compact ? 56 : 0) + (legendPos === "right" ? 40 : 0) || undefined, }, - xAxis: { - type: useTimeAxis ? "time" : "category", - ...(useTimeAxis ? {} : { data: xValues.map(String) }), - name: compact ? undefined : xAxisLabel, - nameLocation: "middle", - nameGap: 30, - axisLabel: { show: !compact }, - }, + // Built as two literals rather than one object with ternaries: ECharts + // discriminates the axis union on `type`, so a `"time" | "category"` + // union defeats narrowing and the whole option fails to type-check. + // Compact drops the axis *name* and the value numbers, never the x + // labels (#1247). The category branch takes the shared config so long + // labels truncate exactly as they do on the bar chart; the time branch + // keeps plain labels, since ECharts formats and thins dates itself and + // the rotation heuristic is meaningless for them. + xAxis: useTimeAxis + ? { + type: "time" as const, + name: compact ? undefined : xAxisLabel, + nameLocation: "middle" as const, + nameGap: 30, + axisLabel: { show: true }, + } + : { + type: "category" as const, + data: xValues.map(String), + name: compact ? undefined : xAxisLabel, + nameLocation: "middle" as const, + nameGap: 30, + axisLabel: buildCategoryAxisLabel(xValues.length, { + containerWidth: width, + }), + }, yAxis: useDualAxis ? [leftYAxis, rightYAxis] : leftYAxis, series: seriesKeys.map((key, idx) => buildSeries(key, idx)), }; @@ -276,6 +295,7 @@ function LineChart({ rightYAxisLabel, compact, hideLegend, + width, samplingThreshold, samplingMethod, ]); diff --git a/component/src/charts/theme.ts b/component/src/charts/theme.ts index d450596f..5085b493 100644 --- a/component/src/charts/theme.ts +++ b/component/src/charts/theme.ts @@ -75,6 +75,18 @@ export function formatAxisCompact(value: number | string): string { return String(value); } +/** + * One gridline colour per theme, for every cartesian chart (#1247). + * + * Charts must not set `splitLine.lineStyle` themselves — differing grid + * weight makes two widgets on one dashboard look like they came from + * different tools. Enforced by cartesian-axis-defaults.test.tsx. + */ +export const GRID_LINE_COLOR = { + light: "#f0f2f4", + dark: "#1d2025", +} as const; + function axisStyle(line: string, label: string, split: string) { return { axisLine: { lineStyle: { color: line } }, @@ -134,7 +146,7 @@ export function registerNeoboardThemes( ) { // Light: border hsl(220 13% 91%) ≈ #e5e7eb, muted-fg hsl(220 9% 44%) ≈ #666d7a const lightAxis = { - ...axisStyle("#e5e7eb", "#666d7a", "#f0f2f4"), + ...axisStyle("#e5e7eb", "#666d7a", GRID_LINE_COLOR.light), }; registerTheme(THEME_LIGHT, { color: CITRINE_LIGHT, @@ -142,6 +154,9 @@ export function registerNeoboardThemes( textStyle: { color: "#14161a" }, // foreground hsl(220 13% 9%) title: { textStyle: { color: "#14161a" } }, categoryAxis: lightAxis, + // A time axis is a cartesian axis too — without this entry the gantt and + // any date-based line chart draw ECharts' un-themed grid (#1247). + timeAxis: lightAxis, valueAxis: { ...lightAxis, axisLabel: { color: "#666d7a", formatter: formatAxisCompact }, @@ -161,13 +176,15 @@ export function registerNeoboardThemes( }); // Dark: border hsl(220 13% 17%) ≈ #262931, muted-fg hsl(220 9% 62%) ≈ #959ba7 - const darkAxis = axisStyle("#262931", "#959ba7", "#1d2025"); + const darkAxis = axisStyle("#262931", "#959ba7", GRID_LINE_COLOR.dark); registerTheme(THEME_DARK, { color: CITRINE_DARK, backgroundColor: "transparent", textStyle: { color: "#f3f4f6" }, // foreground hsl(220 14% 96%) title: { textStyle: { color: "#f3f4f6" } }, categoryAxis: darkAxis, + // See the light theme note — time axes need the entry too (#1247). + timeAxis: darkAxis, valueAxis: { ...darkAxis, axisLabel: { color: "#959ba7", formatter: formatAxisCompact },