feat(#316): polish grafana-grid KPI tiles in edit and view modes

CHANGELOG.md

@@ -9,6 +9,29 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
 
 ## [Unreleased]
 
+### Changed
+- **Grafana-grid KPI tiles are polished in both Dashboard modes** (#316,
+  follow-on to #291). Edit mode keeps the full editing shell but drops the
+  never-populated tile footer (no more phantom separator line). View mode
+  strips all outer tile chrome — header, border, background, radius, body
+  padding — leaving the KPI cards (or the loading/unfilled/error state card,
+  now full-width with `role="status"`/`role="alert"` and the tile title in
+  its accessible name) as the only visible surfaces; the tile element stays
+  the CSS-grid item and carries `role="group"` + the query title as its
+  accessible name. KPI cards inside grid tiles lay out as an equal-width
+  responsive grid (`auto-fill`, min 150px — a lone last-row card no longer
+  stretches) with container-query value typography
+  (`clamp(16px, 14cqi, 38px)`). Shared card polish everywhere (workbench,
+  flow bands, grid tiles — owner decision pinned on #316): the value's
+  number+unit render as glued spans that never orphan the unit to its own
+  line, descriptions clamp to two visual lines (full text stays in the DOM),
+  and delta rows anchor to the card bottom for consistent rhythm. Flow-band
+  stream layout, ordinary tiles, schemas, persistence, and KPI value
+  semantics are unchanged. New real-browser e2e suite
+  (`tests/e2e/dashboard-grid-kpi.spec.js`) covers the frameless view, equal
+  widths, unit orphaning, delta alignment, 12/6/4/2 responsive columns,
+  themes, and 360px no-overflow.
+
 ### Added
 - **Line/area/bar/hbar charts over a time-role X column now draw a genuine
   Chart.js `time` scale** (#309, follow-up to #310's category-axis

src/core/kpi.ts

@@ -106,11 +106,19 @@ interface KpiValuePresentation {
   noValue?: string;
 }
 
-export function formatKpiValue(
+/** `formatKpiValue`'s rendering split into its two visual pieces (#316) — the
+ *  digits/no-value text and the unit suffix — so kpi-panel.ts's `.kpi-value`
+ *  can wrap each in its own `<span>` (keeping them visually glued together
+ *  via CSS) without duplicating any of the numeric-formatting logic below.
+ *  `formatKpiValue` itself is exactly `rendered + unit` concatenated, so every
+ *  existing caller that only wants the flat string keeps the identical
+ *  output. */
+export function formatKpiValueParts(
   { value, clickhouseType, presentation = {} }:
   { value: unknown; clickhouseType?: string | null; presentation?: KpiValuePresentation },
-): string {
-  if (value == null) return presentation.noValue ?? '—';
+): { rendered: string; unit: string } {
+  const unit = typeof presentation.unit === 'string' ? presentation.unit : '';
+  if (value == null) return { rendered: presentation.noValue ?? '—', unit: '' };
   const parsedType = parseClickHouseType(clickhouseType);
   // `!`: see isKpiNumericType above — a truthy `parsedType` always unwraps to
   // a real TypeNode.
@@ -129,11 +137,18 @@ export function formatKpiValue(
   else if (exactDecimal) rendered = decimalString(value, explicit ?? 2, explicit == null)!;
   else {
     const number = numericValue(value);
-    if (number == null) return presentation.noValue ?? '—';
+    if (number == null) return { rendered: presentation.noValue ?? '—', unit: '' };
     const fixed = explicit != null ? number.toFixed(explicit) : trimFixed(number, 2);
     rendered = /^-0(?:\.0+)?$/.test(fixed) ? fixed.slice(1) : fixed;
   }
-  return rendered + (typeof presentation.unit === 'string' ? presentation.unit : '');
+  return { rendered, unit };
+}
+
+export function formatKpiValue(
+  args: { value: unknown; clickhouseType?: string | null; presentation?: KpiValuePresentation },
+): string {
+  const { rendered, unit } = formatKpiValueParts(args);
+  return rendered + unit;
 }
 
 /** One diagnostic as `readKpiFields`/kpi-panel.js's `renderKpiCards` produce it. */

src/styles.css

@@ -19,6 +19,8 @@ html, body { margin: 0; padding: 0; height: 100%; overflow: hidden; }
 }
 .kpi-card {
   --kpi-accent: var(--accent);
+  display: flex;
+  flex-direction: column;
   min-width: 0;
   padding: 14px 16px;
   border: 1px solid var(--border);
@@ -27,9 +29,31 @@ html, body { margin: 0; padding: 0; height: 100%; overflow: hidden; }
   background: var(--bg-modal);
 }
 .kpi-label { color: var(--fg-mute); font-size: 12px; font-weight: 600; letter-spacing: .02em; }
+/* #316: value + unit render as two spans (kpi-panel.ts) but must read as one
+   visual value. No whitespace exists BETWEEN the spans (any separator space is
+   part of the author's unit string, inside the unit span), and the unit span
+   is `white-space: nowrap`, so normal wrapping can never split value from
+   unit — while `overflow-wrap: anywhere` on the value still allows a
+   last-resort break inside an exceptionally long number rather than ever
+   clipping/ellipsizing/scrolling. (`nowrap` on `.kpi-value` itself would
+   defeat that: `overflow-wrap` only applies where `white-space` allows
+   wrapping at all.) No margin between the spans — the unit string's own
+   leading space (or absence: `%`) is the author-controlled gap. */
 .kpi-value { margin-top: 6px; font-size: clamp(24px, 4vw, 38px); font-weight: 700; line-height: 1.08; overflow-wrap: anywhere; }
-.kpi-description { margin-top: 7px; color: var(--fg-mute); font-size: 12px; line-height: 1.4; }
-.kpi-delta { margin-top: 9px; font-size: 13px; font-weight: 600; }
+.kpi-value-unit { white-space: nowrap; }
+.kpi-description {
+  margin-top: 7px; color: var(--fg-mute); font-size: 12px; line-height: 1.4;
+  /* Clamp to 2 visual lines; the full text stays in the DOM for assistive
+     tech (#316) — only the rendered box is truncated. */
+  display: -webkit-box; -webkit-line-clamp: 2; -webkit-box-orient: vertical; overflow: hidden;
+}
+/* `margin-top: auto` bottom-anchors a present delta row inside the now-flex
+   `.kpi-card` column (#316); `padding-top` (not another margin) keeps the
+   original 9px gap above it without a collapsing-margin surprise. A card
+   with no delta renders no third row/placeholder at all — nothing forces
+   one; the label/value/description block simply sits at the card's own
+   height. */
+.kpi-delta { margin-top: auto; padding-top: 9px; font-size: 13px; font-weight: 600; }
 .kpi-delta.is-good { color: var(--success, #238636); }
 .kpi-delta.is-bad { color: var(--danger, #cf222e); }
 .kpi-delta.is-neutral { color: var(--fg-mute); }
@@ -2354,6 +2378,15 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); }
   border-top: 1px solid var(--border-faint); font-family: var(--mono);
   font-size: 10.5px; color: var(--fg-faint);
 }
+/* #316: a KPI grid tile's footer is built (ensureTileEl, ui/dashboard.ts) but
+   never populated — `[hidden]` alone loses the cascade to the class rule
+   above (equal specificity, this rule comes first in source order), so an
+   explicit override is needed to actually collapse the border/reserved
+   height it would otherwise leave behind. `ui/dashboard.ts` toggles the
+   `hidden` DOM property both ways on every reconcile (never just sets it
+   once), so switching a tile's panel type can't leave a stale hidden/visible
+   footer either way. */
+.dash-tile-foot[hidden] { display: none; }
 /* KPI bands (#240): a full-width composition primitive, not a tile — spans
    every Dashboard grid column regardless of the selected Full width/Report/
    2 columns/3 columns layout, so several consecutive explicit KPI favorites
@@ -2492,7 +2525,37 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); }
 /* A grid tile's KPI content (#291: KPI tiles render inline, no band) fills the
    tile body like a chart/table does. */
 .dash-gg-tile .dash-tile-body { flex-wrap: wrap; gap: 8px; }
-.dash-gg-tile .dash-tile-body > .kpi-card { flex: 1; min-width: 0; max-width: none; inline-size: auto; }
+/* #316: KPI cards in a Grafana-grid tile use an equal-width responsive grid
+   instead of flex-growing independently — scoped to `.is-kpi` so ordinary
+   chart/table/logs grid tiles keep the flex-row body above untouched.
+   `auto-fill` (not `auto-fit`) is deliberate: it keeps the unfilled tracks
+   of a partial last row instead of collapsing them, so a lone leftover card
+   stays at one column's width rather than stretching across the tile — the
+   single-KPI case still "fills" the row because there is only one track to
+   begin with. `1fr` columns give every card in the same row equal width. */
+.dash-gg-tile.is-kpi .dash-tile-body {
+  display: grid;
+  grid-template-columns: repeat(auto-fill, minmax(150px, 1fr));
+  align-content: start;
+  gap: 8px;
+}
+/* A state card (loading/unfilled/zero-data/error) is always the body's sole
+   content when present (renderKpiInto) — span every track rather than being
+   squeezed into the first ~150px column of a multi-track tile (#316 review
+   finding: the unfilled/error message needs the tile's full width). */
+.dash-gg-tile.is-kpi .dash-tile-body > .dash-kpi-state-card { grid-column: 1 / -1; max-width: none; }
+/* Container-aware value typography is GRID-TILE-ONLY (#316): `container-type:
+   inline-size` cannot go on the shared `.kpi-card` base because inline-size
+   containment sizes the box as if empty — a flow-band card is
+   `inline-size: fit-content` (see .dash-kpi-stream above) and would collapse
+   to its 160px minimum, changing the stream layout this issue pins as
+   untouched. Here the card's width comes from the 1fr track, not its content,
+   so containment is safe — and `14cqi` then scales the value with the card's
+   own width (narrow tile columns shrink the number toward 16px instead of
+   overflowing), while workbench/flow keep the viewport-based clamp on the
+   base `.kpi-value` rule. */
+.dash-gg-tile.is-kpi .dash-tile-body > .kpi-card { container-type: inline-size; }
+.dash-gg-tile.is-kpi .kpi-value { font-size: clamp(16px, 14cqi, 38px); }
 /* No `@media (max-width: 768px)` override here (#291 review finding F1): unlike
    flow's binary mobile flip, grafana-grid@1's ENTIRE responsive behavior is the
    JS container-width clamp (`effectiveGridColumns`, 12/6/4/2 at ≥1160/≥720/≥470)
@@ -2501,3 +2564,28 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); }
    media override here does not know about a tile's span — a span>1 tile whose
    host was forced to `1fr` still tries to occupy N implicit tracks, rendering
    side-by-side/overflowing instead of the model's actual N-column layout. */
+
+/* ── #316: view-mode KPI tiles are frameless ───────────────────────────────
+   `.is-view` is a static per-load class (ui/dashboard.ts sets it once per
+   tile card, from the same `readOnly` that already governs drag/remove/
+   resize — it never toggles mid-session; `.is-kpi` is likewise fixed per
+   session today but is still re-toggled defensively on every reconcile,
+   ui/dashboard.ts `reconcileGridTile`). Scoped to
+   `.dash-gg-grid .dash-gg-tile.is-kpi` so:
+     - a non-KPI grid tile (`.is-view` with no `.is-kpi`) keeps its full frame
+       — only KPI content gets this treatment;
+     - a flow-rendered card is never matched — flow never applies
+       `.dash-gg-tile`/`.is-kpi` (its own KPI tiles render inside the KPI
+       band, not as `.dash-tile` cards at all).
+   The card itself stays the CSS grid item (grid-column/height/order are set
+   directly on `.dash-gg-tile` elsewhere and are untouched here) — this is the
+   "invisible structural wrapper" the issue asks for, not a new DOM node, and
+   never `display: contents` (that would drop it out of grid placement). The
+   accessible name comes from `role="group"`/`aria-label` set directly on this
+   same element (ui/dashboard.ts) — hiding the header visually here does not
+   remove it from the accessibility tree. */
+.dash-gg-grid .dash-gg-tile.is-kpi.is-view {
+  background: transparent; border-color: transparent; border-radius: 0; box-shadow: none;
+}
+.dash-gg-grid .dash-gg-tile.is-kpi.is-view > .dash-tile-head { display: none; }
+.dash-gg-grid .dash-gg-tile.is-kpi.is-view > .dash-tile-body { padding: 0; }

src/ui/dashboard.ts

@@ -651,7 +651,13 @@ export async function renderDashboard(app: DashboardApp): Promise<void> {
     const body = h('div', { class: 'dash-tile-body' });
     const foot = h('div', { class: 'dash-tile-foot' });
     const resizeHandle = !readOnly ? h('div', { class: 'dash-gg-resize', title: 'Resize' }) : null;
-    const card = h('div', { class: 'dash-tile', draggable: String(!readOnly) }, head, body, foot, resizeHandle);
+    // #316: a static, per-load mode class (view mode never toggles mid-session
+    // — `readOnly` is resolved once above, before any tile is built) — CSS
+    // scopes the frameless-KPI-in-view-mode treatment to
+    // `.dash-gg-grid .dash-gg-tile.is-kpi.is-view` (styles.css), so it never
+    // touches a non-KPI tile or a flow-rendered card (flow never adds
+    // `.dash-gg-tile`/`.is-kpi` — its own KPI tiles render inside the band).
+    const card = h('div', { class: 'dash-tile' + (readOnly ? ' is-view' : ''), draggable: String(!readOnly) }, head, body, foot, resizeHandle);
     // Pointer drag is the sole reorder mechanism (#286 owner override, reused
     // verbatim for grafana-grid@1 tiles by #291 — same move-tile command, no
     // engine branching needed): a drop persists the new dashboard.tiles[]
@@ -730,13 +736,29 @@ export async function renderDashboard(app: DashboardApp): Promise<void> {
     paintTileBody(ts, tileEl);
   }
 
+  // A KPI state card's role, per the #316 pinned owner decision: a genuine
+  // query failure (execution error, or a blocking post-execution diagnostic
+  // whose severity is 'error' — e.g. the wrong row count, or no eligible KPI
+  // field) is `alert`; a zero-row result ('kpi-no-data', severity 'info' —
+  // kpi.js) is expected/quiet, like loading or an unfilled parameter, so it
+  // gets `status`.
+  function kpiStateRole(kind: 'loading' | 'unfilled' | 'error' | 'zero-data'): 'status' | 'alert' {
+    return kind === 'error' ? 'alert' : 'status';
+  }
+
   // Render one KPI tile's cards (or its non-ready state) into `host`. On 'ready'
-  // the viewer guarantees columns/rows (no defensive fallback).
+  // the viewer guarantees columns/rows (no defensive fallback). Every state
+  // card carries the tile/query title in its accessible name (#316) — the
+  // frameless view-mode tile has no visible header, so the state card is the
+  // only surface that can announce which tile is loading/blocked/failed.
   function renderKpiInto(host: HTMLElement, ts: ViewerTileState): void {
     if (ts.status !== 'ready') {
-      host.replaceChildren(h('div', { class: 'dash-kpi-state-card' },
-        ts.status === 'error' ? (ts.error || 'Error')
-          : ts.status === 'unfilled' ? 'Enter a value for: ' + ts.unfilled.join(', ') : 'Loading…'));
+      const kind = ts.status === 'error' ? 'error' : ts.status === 'unfilled' ? 'unfilled' : 'loading';
+      const message = ts.status === 'error' ? (ts.error || 'Error')
+        : ts.status === 'unfilled' ? 'Enter a value for: ' + ts.unfilled.join(', ') : 'Loading…';
+      host.replaceChildren(h('div', {
+        class: 'dash-kpi-state-card', role: kpiStateRole(kind), 'aria-label': `${ts.title}: ${message}`,
+      }, message));
       return;
     }
     const panel = (ts.panel || {}) as Record<string, unknown>;
@@ -746,7 +768,10 @@ export async function renderDashboard(app: DashboardApp): Promise<void> {
       serverVersion: state.serverVersion,
     });
     const { cards, errors } = renderKpiCards(resolved.kpi);
-    host.replaceChildren(...(errors.length ? errors.map((e) => h('div', { class: 'dash-kpi-state-card' }, e.message)) : cards));
+    host.replaceChildren(...(errors.length ? errors.map((e) => h('div', {
+      class: 'dash-kpi-state-card', role: kpiStateRole(e.code === 'kpi-no-data' ? 'zero-data' : 'error'),
+      'aria-label': `${ts.title}: ${e.message}`,
+    }, e.message)) : cards));
   }
 
   // ── Grid reconciliation from the flow model ───────────────────────────────
@@ -806,7 +831,39 @@ export async function renderDashboard(app: DashboardApp): Promise<void> {
   // placement differs.
   function reconcileGridTile(ts: ViewerTileState): void {
     const tileEl = ensureTileEl(ts);
-    if (ts.isKpi) { renderKpiInto(tileEl.body, ts); return; }
+    // #316: the generic `.dash-tile-foot` (built once per tile, ensureTileEl)
+    // is never populated for a KPI tile — `paintPanel`/`paintTileBody` (the
+    // only other writers) never run on this branch — so its border/reserved
+    // height must be suppressed at the DOM level (`hidden`, backed by a
+    // styles.css `[hidden]` override strong enough to beat `.dash-tile-foot`'s
+    // own `display: flex`). Toggled BOTH ways on every reconcile (not just set
+    // once) so a tile whose `isKpi`/panel type flips leaves no stale hidden
+    // footer behind on a non-KPI tile, or a stale visible one on a KPI tile.
+    tileEl.foot.hidden = ts.isKpi;
+    // The `.is-kpi` frame class and the group role/name live HERE — not in
+    // `reconcileGrafanaGrid`'s structural loop — because that loop is
+    // short-circuited by the grid signature (columns/span/height only), while
+    // this function runs on every publish. Today `isKpi` is fixed per session
+    // (tile runtimes are built once — dashboard-viewer-session.ts; a real Spec
+    // change recreates session + tile DOM), so the placement is equivalent —
+    // but only THIS placement stays correct if tile runtimes ever become
+    // live-updatable (#287/#288 direction), and it keeps every KPI-gated
+    // mutation (footer, class, role) in one spot. The card is the named group
+    // a frameless view-mode KPI tile relies on for its accessible name (the
+    // visual header is `display: none` in view mode, styles.css). Set in edit
+    // mode too (harmless — the visible header shows the same title) rather
+    // than branching on `readOnly`.
+    tileEl.card.classList.toggle('is-kpi', ts.isKpi);
+    if (ts.isKpi) {
+      tileEl.card.setAttribute('role', 'group');
+      // (`ts.title` is never empty — the session falls back through query
+      // name → queryId → tile id when the tile has no explicit title.)
+      tileEl.card.setAttribute('aria-label', ts.title);
+    } else {
+      tileEl.card.removeAttribute('role');
+      tileEl.card.removeAttribute('aria-label');
+    }
+    if (ts.isKpi) { tileEl.foot.replaceChildren(); renderKpiInto(tileEl.body, ts); return; }
     paintTileBody(ts, tileEl);
   }
 
@@ -833,7 +890,9 @@ export async function renderDashboard(app: DashboardApp): Promise<void> {
       if (!tileEl) continue;
       gridPlacementByTile.set(t.tileId, { span: t.span, heightUnits: t.heightUnits, colStart: t.colStart });
       tileEl.card.classList.add('dash-gg-tile');
-      tileEl.card.classList.toggle('is-kpi', t.isKpi);
+      // (`is-kpi` + the group role/name are maintained by `reconcileGridTile`,
+      // which runs on EVERY pass — this loop is signature-gated and would miss
+      // a panel-type flip with unchanged placement.)
       tileEl.card.style.gridColumn = `span ${t.span}`;
       setGridHeightPx(tileEl.card, t.heightUnits);
       cards.push(tileEl.card);