From 56760bd6ba65760013193f7831c2b961810e79ec Mon Sep 17 00:00:00 2001 From: Julien Gautier Date: Sun, 17 May 2026 01:16:37 +0200 Subject: [PATCH] fix(audit): chart colours + Charts-tab layout regressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Bar chart now uses a single fixed fill (DEFAULT_BAR_FILL = #1d4ed8) instead of cycling Set2 across the X axis. Time-bucket bars carry no per-day semantics; one colour reads correctly. - Donut chart accepts an optional colorMap so the audit page can map success → green, denied → orange, failure → red. New semanticStatusColors export keeps consumers inside the lib's curated palette per ADR-0023. - Chart envelope and audit grid items get min-width: 0 + max-width: 100% on the inner SVG/figure, fixing the body scrollbar + footer gap caused by Plot's fixed-width fallback render on a hidden tab panel. --- .../src/app/pages/audit/audit.html | 3 +- .../src/app/pages/audit/audit.scss | 8 ++++ .../portal-admin/src/app/pages/audit/audit.ts | 16 ++++++- libs/shared/charts/src/index.ts | 5 +++ .../src/lib/_internal/chart-envelope.scss | 37 ++++++++++++++++ .../charts/src/lib/_internal/palette.ts | 42 +++++++++++++++++++ .../src/lib/bar-chart/bar-chart.spec.ts | 23 ++++++++++ .../charts/src/lib/bar-chart/bar-chart.ts | 38 ++++++++++------- .../src/lib/donut-chart/donut-chart.spec.ts | 26 ++++++++++++ .../charts/src/lib/donut-chart/donut-chart.ts | 20 ++++++++- 10 files changed, 201 insertions(+), 17 deletions(-) diff --git a/apps/portal-admin/src/app/pages/audit/audit.html b/apps/portal-admin/src/app/pages/audit/audit.html index a9eb355..0e11425 100644 --- a/apps/portal-admin/src/app/pages/audit/audit.html +++ b/apps/portal-admin/src/app/pages/audit/audit.html @@ -181,8 +181,9 @@ categoryKey="outcome" valueKey="count" [centerLabel]="s.total.toString()" + [colorMap]="outcomeColorMap" caption="Outcome breakdown" - description="Donut chart of audit event outcomes (success, failure, denied) across the full filtered set." + description="Donut chart of audit event outcomes (success in green, denied in orange, failure in red) across the full filtered set." ariaLabel="Outcome breakdown — donut chart" /> diff --git a/apps/portal-admin/src/app/pages/audit/audit.scss b/apps/portal-admin/src/app/pages/audit/audit.scss index 612670f..d085397 100644 --- a/apps/portal-admin/src/app/pages/audit/audit.scss +++ b/apps/portal-admin/src/app/pages/audit/audit.scss @@ -307,6 +307,14 @@ } } +// Grid items default to `min-width: auto`, which prevents the column +// from shrinking below the intrinsic width of its content. Without +// the override, Plot's fixed-width SVG could push the column wider +// than 1fr, busting the layout and forcing a horizontal scrollbar. +.chart-tile { + min-width: 0; +} + // The stacked-bar chart carries a legend and benefits from the // full width; flag it via a modifier and span both columns. .chart-tile--wide { diff --git a/apps/portal-admin/src/app/pages/audit/audit.ts b/apps/portal-admin/src/app/pages/audit/audit.ts index ead5520..7d96f4a 100644 --- a/apps/portal-admin/src/app/pages/audit/audit.ts +++ b/apps/portal-admin/src/app/pages/audit/audit.ts @@ -1,7 +1,7 @@ import { ChangeDetectionStrategy, Component, computed, inject, signal } from '@angular/core'; import { FormsModule } from '@angular/forms'; import { firstValueFrom } from 'rxjs'; -import { BarChart, DonutChart, StackedBarChart } from 'shared-charts'; +import { BarChart, DonutChart, StackedBarChart, semanticStatusColors } from 'shared-charts'; import { environment } from '../../../environments/environment'; import { AuditEventsService, @@ -47,6 +47,20 @@ export class AuditPage { protected readonly pageSizes = PAGE_SIZES; + /** + * Outcome → fill mapping for the donut chart. Semantic colours + * resolved from the shared-charts curated set: success = green, + * denied = orange (`warning` intent), failure = red. Keeps the + * legend "obvious without reading" — admins eyeballing the donut + * see the green slice and know it's success without parsing the + * tooltip. + */ + protected readonly outcomeColorMap: Readonly> = { + success: semanticStatusColors.success, + failure: semanticStatusColors.error, + denied: semanticStatusColors.warning, + }; + // Filter form state. Each field is a separate signal so a change // to one doesn't churn the others through ngModel's reference // equality. All start empty — the BFF returns the most recent diff --git a/libs/shared/charts/src/index.ts b/libs/shared/charts/src/index.ts index cce5b5c..4612d48 100644 --- a/libs/shared/charts/src/index.ts +++ b/libs/shared/charts/src/index.ts @@ -5,3 +5,8 @@ export { type StackedBarChartInputs, } from './lib/stacked-bar-chart/stacked-bar-chart'; export type { ChartBaseInputs, ColorScheme } from './lib/_internal/chart-types'; +export { + DEFAULT_BAR_FILL, + semanticStatusColors, + type SemanticStatus, +} from './lib/_internal/palette'; diff --git a/libs/shared/charts/src/lib/_internal/chart-envelope.scss b/libs/shared/charts/src/lib/_internal/chart-envelope.scss index 2971c2e..32d840f 100644 --- a/libs/shared/charts/src/lib/_internal/chart-envelope.scss +++ b/libs/shared/charts/src/lib/_internal/chart-envelope.scss @@ -9,11 +9,25 @@ // only carries the chart-specific tweaks (canvas dimensions, // chart-specific SVG element rules). +// Host elements default to `display: inline` for Angular custom +// elements, which would let the inner `
` escape flex/grid +// sizing constraints from the parent container and trigger +// horizontal overflow when a chart re-renders before the panel is +// laid out. Forcing `display: block` makes the host fill its grid +// cell and lets `max-width: 100%` actually take effect. +lib-bar-chart, +lib-donut-chart, +lib-stacked-bar-chart { + display: block; + min-width: 0; +} + .chart-figure { margin: 0; display: flex; flex-direction: column; gap: 0.5rem; + min-width: 0; } .chart-caption { @@ -41,8 +55,31 @@ .chart-canvas { width: 100%; + // Grid items default to `min-width: auto` which prevents shrinking + // below the intrinsic content size — when Plot ships an `` + // with a fixed `width="720"` attribute (because the canvas was + // hidden via `[hidden]` on first render and `clientWidth` was 0), + // the canvas couldn't fit in a narrower column and overflowed the + // panel, dragging the body's vertical scrollbar with it. The pair + // `min-width: 0` + `max-width: 100%` on the inner SVG/figure + // forces the chart to scale down to the actual column width. + min-width: 0; min-height: 16rem; + // Constrain Plot's rendered output. Plot returns either a bare + // `` or a `
` wrapping a legend + ``, both with + // fixed pixel dimensions from the `width` config. The browser + // honours the viewBox aspect ratio when we cap width to 100% and + // let height adjust accordingly. + figure { + max-width: 100%; + margin: 0; + } + svg { + max-width: 100%; + height: auto; + } + // Plot's default text colour is hardcoded black; override to the // theme-aware grayscale so axis ticks and labels stay readable // in dark mode without re-rendering. diff --git a/libs/shared/charts/src/lib/_internal/palette.ts b/libs/shared/charts/src/lib/_internal/palette.ts index a14acf6..9a30b3e 100644 --- a/libs/shared/charts/src/lib/_internal/palette.ts +++ b/libs/shared/charts/src/lib/_internal/palette.ts @@ -19,10 +19,52 @@ import { interpolateCividis, interpolateViridis, schemeSet2 } from 'd3-scale-chr * rule (added in this PR) blocks direct imports of * `d3-scale-chromatic` outside this file so the lib stays the only * source of palettes. + * + * In addition to the two scale families, the lib exposes two + * curated colour constants: + * + * - {@link DEFAULT_BAR_FILL} — single fixed fill used by + * `` when no per-bar encoding is requested. + * Picked to be visually neutral against both light and dark + * surfaces and colour-blind-safe (Wong 2011 "blue" deuteranopia- + * tested). + * - {@link semanticStatusColors} — small map of intent-bearing + * colours (`success / warning / error / info / neutral`) + * consumers can opt into via a `colorMap` on the donut chart + * when the categorical axis carries *meaning* rather than just + * distinct labels. Values are tuned for AA contrast on white + * surfaces and remain distinguishable under deuteranopia / + * protanopia simulation (red and orange differ in lightness, not + * just hue). */ export type ColorScheme = 'sequential' | 'categorical'; +/** + * Default fill colour for bars when no per-bar encoding applies. + * `#1d4ed8` ≈ tailwind blue-700 — high contrast against white, + * still readable on a dark surface. + */ +export const DEFAULT_BAR_FILL = '#1d4ed8'; + +/** + * Intent-bearing colours for the small set of statuses APF charts + * deal with. Consumers cannot author their own colours from outside + * the lib (per ADR-0023), but they can pick from this curated map + * by name — keeps the colour-blind-safety guarantee while letting + * the donut chart encode meaning (success = green, denied = + * orange). + */ +export const semanticStatusColors = { + success: '#16a34a', // green-600 + warning: '#ea580c', // orange-600 + error: '#dc2626', // red-600 + info: '#2563eb', // blue-600 + neutral: '#6b7280', // gray-500 +} as const; + +export type SemanticStatus = keyof typeof semanticStatusColors; + /** * Build a function that maps a numeric input in [0, 1] to a hex * colour from the sequential ramp. Use this for value-driven fills diff --git a/libs/shared/charts/src/lib/bar-chart/bar-chart.spec.ts b/libs/shared/charts/src/lib/bar-chart/bar-chart.spec.ts index 1470a70..d3e7d33 100644 --- a/libs/shared/charts/src/lib/bar-chart/bar-chart.spec.ts +++ b/libs/shared/charts/src/lib/bar-chart/bar-chart.spec.ts @@ -72,6 +72,29 @@ describe('BarChart', () => { expect(firstTwo[1]?.textContent).toContain('Bar chart showing the count'); }); + it('fills every bar with the same single colour (no per-bar palette cycle)', () => { + const fixture = render(); + const root = fixture.nativeElement as HTMLElement; + const svg = root.querySelector('.chart-canvas svg'); + expect(svg).not.toBeNull(); + // Plot applies a literal-string `fill` either on the bar-mark + // `` group or on each ``. Collect every `[fill]` attr + // present below the mark group and confirm there's only one + // distinct value (i.e. we are no longer cycling a categorical + // palette across the X axis). Axes / rules carry their own + // fills (text, ticks), so we narrow to the bar-mark subtree + // via `aria-label`, which Plot sets to "bar" on the bar group. + const barGroup = svg?.querySelector('[aria-label="bar"]'); + expect(barGroup).not.toBeNull(); + const fills = new Set(); + const groupFill = barGroup?.getAttribute('fill'); + if (groupFill) fills.add(groupFill); + barGroup + ?.querySelectorAll('[fill]') + .forEach((el) => fills.add(el.getAttribute('fill') ?? '')); + expect(fills.size).toBe(1); + }); + it('regenerates the chart when `data` changes', async () => { const fixture = render(); const root = fixture.nativeElement as HTMLElement; diff --git a/libs/shared/charts/src/lib/bar-chart/bar-chart.ts b/libs/shared/charts/src/lib/bar-chart/bar-chart.ts index e406f4f..f6d4dd4 100644 --- a/libs/shared/charts/src/lib/bar-chart/bar-chart.ts +++ b/libs/shared/charts/src/lib/bar-chart/bar-chart.ts @@ -9,15 +9,9 @@ import { input, } from '@angular/core'; import * as Plot from '@observablehq/plot'; -import { - chartId, - findChartSvg, - injectSvgTitleDesc, - prefersReducedMotion, - resolveTheme, -} from '../_internal/a11y'; +import { chartId, findChartSvg, injectSvgTitleDesc, prefersReducedMotion } from '../_internal/a11y'; import type { ChartBaseInputs } from '../_internal/chart-types'; -import { paletteFor } from '../_internal/palette'; +import { DEFAULT_BAR_FILL } from '../_internal/palette'; /** * `` — vertical bar chart over a categorical X axis. @@ -26,6 +20,15 @@ import { paletteFor } from '../_internal/palette'; * row in `data` becomes one bar with its height encoding `yKey` and * its position encoding `xKey`. * + * Every bar is filled with the same colour by default — a single- + * series bar chart's X axis is typically a *label* (day, bucket, + * name) rather than a category that meaningfully encodes colour, so + * rotating through a categorical palette adds visual noise without + * carrying signal. Consumers that genuinely want per-bar colouring + * (a categorical axis where each bar IS a category with semantic + * meaning) should use `` instead, or pass an + * explicit `fillColor` if they really do want a different shade. + * * Per the lib's a11y contract: * - outer `
` carries `role="img"` + `aria-labelledby` + * `aria-describedby`, @@ -33,8 +36,8 @@ import { paletteFor } from '../_internal/palette'; * them; the helper in `_internal/a11y.ts` does), * - `
` disclosure with a `
` fallback ships every * data point in a screen-reader-friendly form, - * - palette is resolved from `_internal/palette.ts` (Set2 - * categorical by default, Viridis sequential when requested), + * - fill defaults to `DEFAULT_BAR_FILL` from + * `_internal/palette.ts` (a colour-blind-safe blue), * - `prefers-reduced-motion` skips transitions. */ @@ -46,6 +49,12 @@ export interface BarChartInputs extends ChartBaseInputs { /** Human-readable axis labels (no implicit i18n inside the lib). */ readonly xLabel?: string; readonly yLabel?: string; + /** + * Override the default single fill colour. Caller-owned — keep it + * colour-blind-safe and contrasted against the surrounding surface. + * Most consumers should leave it unset and accept the lib default. + */ + readonly fillColor?: string; } @Component({ @@ -70,7 +79,7 @@ export class BarChart> { readonly ariaLabel = input.required(); readonly xLabel = input(undefined); readonly yLabel = input(undefined); - readonly colorScheme = input<'sequential' | 'categorical'>('categorical'); + readonly fillColor = input(DEFAULT_BAR_FILL); protected readonly chartId = chartId(); protected readonly titleId = `${this.chartId}-title`; @@ -105,7 +114,7 @@ export class BarChart> { const yKey = this.yKey(); const xLabel = this.xLabel() ?? null; const yLabel = this.yLabel() ?? null; - const palette = paletteFor(this.colorScheme(), resolveTheme(), Math.max(1, data.length)); + const fill = this.fillColor(); const plot = Plot.plot({ // Mirrors the host element's available width so the chart is @@ -115,12 +124,13 @@ export class BarChart> { marginBottom: 40, x: { label: xLabel, type: 'band' }, y: { label: yLabel, grid: true, nice: true }, - color: { type: 'ordinal', range: palette }, marks: [ Plot.barY(data as Plot.Data, { x: xKey, y: yKey, - fill: xKey, + // Literal-string fill — Plot interprets non-key strings as + // a fixed colour. Same shade on every bar, by design. + fill, // Plot's default tooltip would require interaction wiring // we haven't shipped yet — the tabular fallback under // `
` is the v1 substitute for hover values. diff --git a/libs/shared/charts/src/lib/donut-chart/donut-chart.spec.ts b/libs/shared/charts/src/lib/donut-chart/donut-chart.spec.ts index 0d4b2dd..1947a58 100644 --- a/libs/shared/charts/src/lib/donut-chart/donut-chart.spec.ts +++ b/libs/shared/charts/src/lib/donut-chart/donut-chart.spec.ts @@ -75,4 +75,30 @@ describe('DonutChart', () => { expect(rows[0]?.querySelector('th')?.textContent?.trim()).toBe('success'); expect(rows[0]?.querySelector('td')?.textContent?.trim()).toBe('120'); }); + + it('honours an optional `colorMap` to colour slices by category', () => { + TestBed.configureTestingModule({ imports: [DonutChart] }); + const fixture = TestBed.createComponent>(DonutChart); + fixture.componentRef.setInput('data', SAMPLE); + fixture.componentRef.setInput('categoryKey', 'outcome'); + fixture.componentRef.setInput('valueKey', 'count'); + fixture.componentRef.setInput('caption', 'Outcome breakdown'); + fixture.componentRef.setInput('description', 'Donut chart of audit-event outcomes.'); + fixture.componentRef.setInput('ariaLabel', 'Outcome breakdown — donut chart'); + fixture.componentRef.setInput('colorMap', { + success: '#16a34a', + failure: '#dc2626', + denied: '#ea580c', + }); + fixture.detectChanges(); + + const root = fixture.nativeElement as HTMLElement; + const slices = root.querySelectorAll('.chart-canvas svg path'); + // SAMPLE order: success, failure, denied — fills must match the + // map exactly so admins read "green = success, orange = denied" + // without consulting the legend. + expect(slices[0]?.getAttribute('fill')).toBe('#16a34a'); + expect(slices[1]?.getAttribute('fill')).toBe('#dc2626'); + expect(slices[2]?.getAttribute('fill')).toBe('#ea580c'); + }); }); diff --git a/libs/shared/charts/src/lib/donut-chart/donut-chart.ts b/libs/shared/charts/src/lib/donut-chart/donut-chart.ts index 1bc1938..5e29652 100644 --- a/libs/shared/charts/src/lib/donut-chart/donut-chart.ts +++ b/libs/shared/charts/src/lib/donut-chart/donut-chart.ts @@ -36,6 +36,16 @@ export interface DonutChartInputs extends ChartBaseInputs { readonly valueKey: keyof T & string; /** Optional centre-of-donut text (e.g. the total). */ readonly centerLabel?: string; + /** + * Optional per-category fill override. When the keys on the X + * axis carry intent (e.g. audit outcomes — `success` MUST read + * as green, `denied` as orange), pass a map from category value + * to colour. Categories not in the map fall back to the + * colour-blind-safe categorical palette. Use the + * `semanticStatusColors` export from this lib to stay within the + * curated colour set (per ADR-0023). + */ + readonly colorMap?: Readonly>; } @Component({ @@ -56,6 +66,7 @@ export class DonutChart> { readonly ariaLabel = input.required(); readonly centerLabel = input(undefined); readonly colorScheme = input<'sequential' | 'categorical'>('categorical'); + readonly colorMap = input> | undefined>(undefined); protected readonly chartId = chartId(); protected readonly titleId = `${this.chartId}-title`; @@ -85,6 +96,7 @@ export class DonutChart> { const categoryKey = this.categoryKey(); const valueKey = this.valueKey(); const palette = paletteFor(this.colorScheme(), resolveTheme(), Math.max(1, data.length)); + const colorMap = this.colorMap(); const width = canvas.clientWidth || 320; const height = 240; @@ -108,7 +120,13 @@ export class DonutChart> { arcs.forEach((slice, i) => { const path = canvas.ownerDocument.createElementNS(svgNs, 'path'); path.setAttribute('d', arcGen(slice) ?? ''); - path.setAttribute('fill', palette[i % palette.length] ?? '#888'); + // Semantic override first (when the consumer maps a category + // to an intent-bearing colour), then palette cycle as a + // fallback for any unmapped category. Final `#888` is a + // defensive default that should never actually trigger. + const category = String(slice.data[categoryKey]); + const fill = colorMap?.[category] ?? palette[i % palette.length] ?? '#888'; + path.setAttribute('fill', fill); path.setAttribute('stroke', resolveTheme() === 'dark' ? '#111827' : '#ffffff'); path.setAttribute('stroke-width', '1.5'); // Each slice gets its own child so hovering announces