From 12025097e1c5b0114a294ab6a21049d8fb0f7cf8 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 18 Jul 2026 09:29:05 +0000 Subject: [PATCH 1/2] fix(#294): dashboard filter strips never wrap, remove visible Clear-all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reverses the visible Clear-all UX decision from #286/#293: the toolbar and filter field region now stay flex-wrap:nowrap at every viewport width (not just the ≤768px mobile breakpoint), with the field region scrolling horizontally in a dedicated `.filter-strip-scroll` viewport and the fixed "N active" count as a non-scrolling sibling. The visible Clear-all button is deleted; DashboardViewerSession.clearAllFilters() stays a tested application-level operation with no UI trigger. Also, per multi-angle review: factors the shared scroll-viewport CSS contract between the Workbench var-strip and the new Dashboard filter strip (previously copy-pasted), fixes a focus-ring clipping regression on the new scroll viewport, and keeps the "N active" count vertically centered against a filter row taller than one line. New Playwright coverage for both surfaces (dashboard-mobile.spec.js, workbench-var-strip.spec.js) since CSS wrap/overflow is invisible to the happy-dom unit suite. Closes #294 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GLMprUnwnz4oaz3cnLRKcz --- CHANGELOG.md | 29 +++++++++++ src/styles.css | 68 +++++++++++++++++------- src/ui/dashboard.ts | 15 ++++-- src/ui/filter-bar.ts | 22 ++------ tests/e2e/dashboard-mobile.html | 21 +++++++- tests/e2e/dashboard-mobile.spec.js | 74 +++++++++++++++++++++++++-- tests/e2e/workbench-var-strip.html | 38 ++++++++++++++ tests/e2e/workbench-var-strip.spec.js | 47 +++++++++++++++++ tests/unit/dashboard.test.ts | 35 +++++++++---- 9 files changed, 295 insertions(+), 54 deletions(-) create mode 100644 tests/e2e/workbench-var-strip.html create mode 100644 tests/e2e/workbench-var-strip.spec.js diff --git a/CHANGELOG.md b/CHANGELOG.md index bee7702..4e52479 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,35 @@ auto-generated per-PR notes; this file is the curated, human-readable history. ## [Unreleased] +### Fixed +- **Dashboard filter strips no longer wrap, and the visible "Clear all" control + is removed** (#294 — reverses the visible clear-all UX decision made in + #286/#293). `.dash-toolbar` and the filter field region now stay + `flex-wrap: nowrap` at every viewport width, not just the ≤768px mobile + breakpoint from #248 — several `{name:Type}` params at desktop width used to + wrap the toolbar onto extra rows, permanently shrinking the dashboard's data + area. The field region is now a dedicated horizontally-scrolling viewport + (`.filter-strip-scroll`, the same nowrap/overflow-x:auto/hidden-scrollbar + contract the Workbench `.var-strip` already used), with the fixed "N active" + status (`.dash-filter-count-host`) as a non-scrolling sibling so it never + gets pushed onto a second line or scrolled out of view; the layout switcher + stays fixed alongside it on desktop. The visible **Clear all** button + (`filterClearAllButton`, `.dash-filter-clear-all`) is deleted outright — + `DashboardViewerSession.clearAllFilters()` stays a tested application-level + operation with no UI trigger. Combobox popovers, relative-time previews, and + filter execution/activation/debounce/recents/validation are unchanged. The + "hide native scrollbar, allow horizontal auto-scroll" viewport contract is + now one shared rule read by both `.var-strip` and `.filter-strip-scroll` + (previously copy-pasted per surface — review follow-up), and the scroll + viewport carries enough vertical padding that a focused field's box-shadow + ring is never clipped by its `overflow-y: hidden`; the "N active" count is + `align-self: center` so it stays vertically centered against the field row + even when a field (e.g. relative-time's resolved-value preview line) is + taller than one line. New Playwright coverage (`dashboard-mobile.spec.js`, + `workbench-var-strip.spec.js`) pins the never-wrap/scroll contract, the + count's centering against a taller row, and the focus-ring padding for both + surfaces in a real browser (invisible to the happy-dom unit suite). + ### Added - **Dashboard v1 contracts, codecs, canonical encoding, and resource limits** (#283, phase 1 of #280). New canonical JSON Schemas ship through the existing diff --git a/src/styles.css b/src/styles.css index 16fe1f2..6ed94f7 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1310,18 +1310,32 @@ body.detached-tab .graph-overlay-panel { } .ed-toolbar > * { flex-shrink: 0; } .ed-toolbar::-webkit-scrollbar { display: none; } +/* Shared "hide the native scrollbar, allow horizontal auto-scroll, contain + overscroll" viewport contract (#294) — the Workbench var-strip and the + Dashboard filter-strip scroll viewport (`.filter-strip-scroll`, below) read + it off one declaration so the two surfaces can't drift apart. Each keeps its + own layout rule (`.var-strip` is itself the flex/nowrap row; the Dashboard + nests a separate flex row — `.dash-filters` — inside this viewport so the + fixed "N active" status can sit outside it as a sibling). */ +.var-strip, +.filter-strip-scroll { + overflow-x: auto; overflow-y: hidden; scrollbar-width: none; + overscroll-behavior-x: contain; -webkit-overflow-scrolling: touch; +} +.var-strip::-webkit-scrollbar, +.filter-strip-scroll::-webkit-scrollbar { display: none; } /* Query-variable strip (#134): one field per detected {name:Type}. A single row that swipe-scrolls horizontally when it overflows (many variables, or a - narrow/mobile workbench) — never wraps to a second line and never clips. */ + narrow/mobile workbench) — never wraps to a second line and never clips. + The 6px vertical padding also keeps a focused field's box-shadow ring from + being clipped by `overflow-y: hidden` above. */ .var-strip { display: flex; align-items: flex-start; gap: 14px; padding: 6px 10px; background: var(--bg-toolbar); border-bottom: 1px solid var(--border); flex-shrink: 0; flex-wrap: nowrap; - overflow-x: auto; scrollbar-width: none; } -.var-strip::-webkit-scrollbar { display: none; } .var-field { display: inline-grid; grid-template-columns: max-content max-content; @@ -2134,9 +2148,12 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } gap: 12px; padding: 11px 20px; background: var(--bg-header); border-bottom: 1px solid var(--border); flex-wrap: wrap; } -/* Layout toolbar (#149 D2) — becomes the filter bar in D4. */ +/* Layout toolbar (#149 D2) — becomes the filter bar in D4. `flex-wrap: nowrap` + at every width (#294): filters used to wrap the toolbar onto extra rows once + several `{name:Type}` params were present, growing the toolbar's height as + the dashboard's data area shrank. */ .dash-toolbar { - display: flex; align-items: center; gap: 12px; flex-wrap: wrap; + display: flex; align-items: center; gap: 12px; flex-wrap: nowrap; padding: 8px 20px; background: var(--bg-header); border-bottom: 1px solid var(--border); } .dash-seg { display: inline-flex; background: var(--bg-chip); border: 1px solid var(--border); border-radius: 8px; padding: 2px; } @@ -2146,13 +2163,36 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } } .dash-seg-btn:hover { color: var(--fg); } .dash-seg-btn.is-active { background: var(--bg); color: var(--fg); box-shadow: 0 1px 2px rgba(0,0,0,.12); } -.dash-layout-wrap { display: inline-flex; align-items: center; gap: 8px; } +.dash-layout-wrap { display: inline-flex; align-items: center; gap: 8px; flex: 0 0 auto; } .dash-seg-label { font-size: 11px; color: var(--fg-faint); } /* Global filter bar (#149 D3) — one field per detected {name:Type}, reusing the - workbench's var-field/var-name/var-input classes. Sits in the toolbar's - existing flex-wrap row (no new sticky/scroll container needed); absent - entirely (display:none, no gap) when no favorite has a param. */ + workbench's var-field/var-name/var-input classes. + #294: the field region lives in its own horizontally-scrolling viewport + (`.filter-strip-scroll`, the shared scroll contract above, following the + workbench `.var-strip` / editor-tabs precedent) so excess fields scroll + instead of wrapping, while the fixed "N active" status + (`.dash-filter-count-host`) stays outside it on the same row at every + viewport width — not just the ≤768px breakpoint. `align-self: center` on + the count keeps it vertically centered against the field row even when a + field grows taller (e.g. a relative-time field's resolved-value preview + line), rather than pinned to the row's top edge. The 4px vertical padding + on the scroll viewport keeps a focused field's box-shadow ring from being + clipped by its `overflow-y: hidden`. */ +.dash-filter-host { + display: flex; align-items: flex-start; gap: 12px; + flex: 1 1 auto; min-width: 0; +} +.filter-strip-scroll { flex: 1 1 auto; min-width: 0; padding: 4px 0; } +.dash-filter-count-host { flex: 0 0 auto; align-self: center; } .dash-filters { display: flex; align-items: flex-start; gap: 14px; flex-wrap: wrap; } +/* `.dash-filters` is shared with the detached Data view's filter row + (`.detached-filter-row .dash-filters`, above), which keeps the default wrap + — only the Dashboard toolbar's instance, nested in `.filter-strip-scroll`, + needs the never-wrap contract, so the override is scoped to that ancestor + rather than changed on the shared class itself. `.var-field` already sets + `flex-shrink: 0`, so `flex-wrap: nowrap` alone is enough for the overflow to + reach the scroll viewport above — no extra width rule needed. */ +.filter-strip-scroll .dash-filters { flex-wrap: nowrap; } .dash-icobtn { width: 30px; height: 30px; display: inline-flex; align-items: center; justify-content: center; background: var(--bg-chip); color: var(--fg-mute); border: 1px solid var(--border); @@ -2346,16 +2386,8 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } .dash-refresh { width: 30px; height: 30px; flex-shrink: 0; } .dash-refresh { padding: 0; justify-content: center; } - .dash-toolbar { flex-wrap: nowrap; padding: 6px 10px; } + .dash-toolbar { padding: 6px 10px; } .dash-toolbar:not(.has-filters) { display: none; } - .dash-filter-host { width: 100%; min-width: 0; overflow: hidden; } - .dash-filters { - width: 100%; flex-wrap: nowrap; overflow-x: auto; overflow-y: hidden; - scrollbar-width: none; overscroll-behavior-x: contain; - -webkit-overflow-scrolling: touch; - } - .dash-filters::-webkit-scrollbar { display: none; } - .dash-filters > .var-field { flex-shrink: 0; } .dash-grid, .dash-grid.is-wide, diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index 23775f0..fcaa50e 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -42,7 +42,7 @@ import { analyzeParameterizedSources, fieldControls } from '../core/param-pipeli import type { ValidationMode } from '../core/param-pipeline.js'; import { queryDashboardRole } from '../dashboard/model/workspace-semantics.js'; import { renderKpiCards, KPI_STREAM_ARIA } from './kpi-panel.js'; -import { buildFilterBar, filterClearAllButton, filterActiveCount } from './filter-bar.js'; +import { buildFilterBar, filterActiveCount } from './filter-bar.js'; import type { FilterBarApp } from './filter-bar.js'; import { createDashboardViewerSession } from '../dashboard/application/dashboard-viewer-session.js'; import type { @@ -238,9 +238,15 @@ export async function renderDashboard(app: DashboardApp): Promise { const layoutWrap = h('div', { class: 'dash-layout-wrap' }, h('span', { class: 'dash-seg-label' }, 'Layout'), presetSeg.el); // ── Filter bar (shared buildFilterBar, viewer-backed) ───────────────────── - const filterHost = h('div', { class: 'dash-filter-host' }); + // #294: the field region scrolls horizontally in its own viewport + // (`.filter-strip-scroll`) so it never wraps the toolbar onto a second row; + // the "N active" count sits outside it as a fixed, non-scrolling sibling. + // No visible Clear-all control (reverses the #286/#293 decision) — + // `session.clearAllFilters()` stays a tested application-level operation + // with no UI trigger. + const filterScroll = h('div', { class: 'filter-strip-scroll' }); const filterCountNode = h('span', { class: 'dash-filter-count-host' }); - const clearAllNode = h('span', { class: 'dash-filter-clear-all-host' }); + const filterHost = h('div', { class: 'dash-filter-host' }, filterScroll, filterCountNode); // The draft value/active bag the shared filter bar reads + mutates; re-seeded // from committed filter state on each (re)build. Recents come from the real // app — the viewer never touches AppState. @@ -274,7 +280,7 @@ export async function renderDashboard(app: DashboardApp): Promise { }; const getField = (name: string, mode: ValidationMode) => session.getFilterField(name, mode, draftValues, draftActive); const bar = buildFilterBar(filterBarApp, session.controls, onCommit, getField, { curatedFields, document: doc }); - filterHost.replaceChildren(bar.el, clearAllNode, filterCountNode); + filterScroll.replaceChildren(bar.el); filterBarDispose = bar.dispose; } @@ -458,7 +464,6 @@ export async function renderDashboard(app: DashboardApp): Promise { // progress ticks — so in-progress typing is not disturbed mid-wave. const sig = JSON.stringify(sview.filters.map((f) => [f.id, f.active, valueString(f.value), !!(f.options && f.options.length)])); if (sig !== barSig) { barSig = sig; rebuildFilterBar(sview); } - clearAllNode.replaceChildren(filterClearAllButton({ active: sview.activeFilterCount > 0, onClearAll: () => session.clearAllFilters() })); filterCountNode.replaceChildren(filterActiveCount(sview.activeFilterCount)); tileCountLabel.textContent = sview.tiles.length + (sview.tiles.length === 1 ? ' tile' : ' tiles'); empty.style.display = sview.tiles.length ? 'none' : ''; diff --git a/src/ui/filter-bar.ts b/src/ui/filter-bar.ts index 4b0d3ea..4bb702d 100644 --- a/src/ui/filter-bar.ts +++ b/src/ui/filter-bar.ts @@ -86,24 +86,12 @@ const buildRecentField = _buildRecentField as (opts: { onValueInput: () => void; onCommit: () => void; }) => FilterBarComboField; -// ── #188 filter-bar usability affordances (#286) ───────────────────────────── +// ── #188 filter-bar usability affordances (#286; clear-all button removed by +// #294 — see below) ────────────────────────────────────────────────────────── // Small, reusable DOM builders the Dashboard viewer drives from its filter -// state (clearFilter / clearAllFilters / activeFilterCount / blocking). Kept -// here, on the shared filter-bar module, so any filter surface can compose the -// same affordances; they are pure element factories (no app/state coupling). - -/** The toolbar clear-all affordance (#188): resets every filter in one wave. - * Hidden (not just disabled) when nothing is active, so it never draws focus - * to a no-op. */ -export function filterClearAllButton(opts: { active: boolean; onClearAll: () => void }): HTMLButtonElement { - const btn = h('button', { - type: 'button', class: 'dash-filter-clear-all', - title: 'Clear all filters', 'aria-label': 'Clear all filters', - onclick: () => opts.onClearAll(), - }, 'Clear all'); - if (!opts.active) btn.style.display = 'none'; - return btn; -} +// state (activeFilterCount / blocking). Kept here, on the shared filter-bar +// module, so any filter surface can compose the same affordances; they are +// pure element factories (no app/state coupling). /** The "N active" indicator (#188): counts ACTIVE filter definitions, readable * by assistive tech without a live-region announcement (a plain labeled diff --git a/tests/e2e/dashboard-mobile.html b/tests/e2e/dashboard-mobile.html index bfd384f..4e43604 100644 --- a/tests/e2e/dashboard-mobile.html +++ b/tests/e2e/dashboard-mobile.html @@ -38,7 +38,10 @@ -
+
+
+ 3 active +
@@ -54,6 +57,7 @@
+ + diff --git a/tests/e2e/workbench-var-strip.spec.js b/tests/e2e/workbench-var-strip.spec.js new file mode 100644 index 0000000..2a9409c --- /dev/null +++ b/tests/e2e/workbench-var-strip.spec.js @@ -0,0 +1,47 @@ +import { test, expect } from '@playwright/test'; + +// The Workbench `{name:Type}` variable strip (#134/#248 precedent) already +// implements the never-wrap/horizontally-scrolling interaction contract #294 +// generalizes to Dashboard filters. happy-dom cannot compute CSS layout +// (wrapping/overflow are invisible to the unit suite), so this real-browser +// harness pins the contract directly against `.var-strip`. +async function openAt(page, width, height = 700) { + await page.setViewportSize({ width, height }); + await page.goto('/tests/e2e/workbench-var-strip.html'); + await page.waitForFunction(() => window.__ready === true); +} + +test.describe('Workbench var-strip overflow', () => { + test('stays on one row and scrolls horizontally instead of wrapping', async ({ page }) => { + await openAt(page, 900); + const strip = page.locator('.var-strip'); + const layout = await strip.evaluate((node) => ({ + flexWrap: getComputedStyle(node).flexWrap, + overflowX: getComputedStyle(node).overflowX, + clientWidth: node.clientWidth, + scrollWidth: node.scrollWidth, + fieldTops: [...node.querySelectorAll('.var-field')].map((field) => field.getBoundingClientRect().top), + fieldWidths: [...node.querySelectorAll('.var-field')].map((field) => field.getBoundingClientRect().width), + pageOverflow: document.documentElement.scrollWidth - innerWidth, + })); + expect(layout.flexWrap).toBe('nowrap'); + expect(layout.overflowX).toBe('auto'); + expect(layout.scrollWidth).toBeGreaterThan(layout.clientWidth); + expect(Math.max(...layout.fieldTops) - Math.min(...layout.fieldTops)).toBeLessThan(2); + expect(Math.min(...layout.fieldWidths)).toBeGreaterThan(150); + expect(layout.pageOverflow).toBeLessThanOrEqual(0); + }); + + test('reaches the final field by scroll and by keyboard focus', async ({ page }) => { + await openAt(page, 900); + const strip = page.locator('.var-strip'); + + await strip.evaluate((node) => { node.scrollLeft = node.scrollWidth; }); + expect(await strip.evaluate((node) => node.scrollLeft)).toBeGreaterThan(0); + await strip.evaluate((node) => { node.scrollLeft = 0; }); + + const lastInput = page.locator('.var-strip .var-field').last().locator('input'); + await lastInput.focus(); + await expect(lastInput).toBeInViewport(); + }); +}); diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 3408861..37d8c6e 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -594,7 +594,7 @@ describe('renderDashboard — shared rich filter bar over the viewer (#188)', () expect(added[0].params.param_p).toBe('x'); }); - it('shows the N-active count and clear-all when a filter is active; both hidden otherwise', async () => { + it('shows the N-active count when a filter is active, hidden otherwise; no visible Clear-all control at any count (#294)', async () => { const { app } = dashApp({ workspace: wsWith({ queries: [q('q1', 'SELECT k, v FROM a WHERE n = {n:UInt8}')], @@ -604,13 +604,30 @@ describe('renderDashboard — shared rich filter bar over the viewer (#188)', () }); await render(app); expect(qs(app.root, '.dash-filter-count')?.textContent).toBe('1 active'); - const clearAll = qs(app.root, '.dash-filter-clear-all'); - expect(clearAll.style.display).toBe(''); - // Clear-all resets to defaults (one coalesced wave; a no-op here since the - // filter is already at its default) — exercises the toolbar action. - clearAll.dispatchEvent(new Event('click', { bubbles: true })); - await Promise.resolve(); - expect(qs(app.root, '.dash-filter-count')?.textContent).toBe('1 active'); + // #294 reverses #286/#293's visible Clear-all control — it never renders, + // active count or not (`DashboardViewerSession.clearAllFilters()` stays a + // tested application-level operation with no UI trigger). + expect(qs(app.root, '.dash-filter-clear-all')).toBeNull(); + }); + + it('separates the scrolling filter-field region from the fixed count region (#294)', async () => { + const { app } = dashApp({ + workspace: wsWith({ + queries: [q('q1', 'SELECT k, v FROM a WHERE n = {n:UInt8}')], + tiles: [{ id: 't1', queryId: 'q1' }], + filters: [{ id: 'n', parameter: 'n', defaultValue: 5, defaultActive: true }], + }), + }); + await render(app); + const host = qs(app.root, '.dash-filter-host'); + const scroll = qs(host, '.filter-strip-scroll'); + const count = qs(host, '.dash-filter-count-host'); + // The fields live inside the scroll viewport, the count lives outside it — + // as separate host children, not nested one inside the other. + expect(scroll.contains(qs(host, '.dash-filters'))).toBe(true); + expect(scroll.contains(count)).toBe(false); + expect(count.contains(scroll)).toBe(false); + expect(count.querySelector('.dash-filter-count')?.textContent).toBe('1 active'); }); it('renders no per-filter "required/invalid" badge (owner decision — dropped as noise)', async () => { @@ -623,7 +640,7 @@ describe('renderDashboard — shared rich filter bar over the viewer (#188)', () await render(app); expect(qs(app.root, '.dash-filter-blocking')).toBeNull(); expect((qs(app.root, '.dash-filter-count') as HTMLElement).style.display).toBe('none'); - expect((qs(app.root, '.dash-filter-clear-all') as HTMLElement).style.display).toBe('none'); + expect(qs(app.root, '.dash-filter-clear-all')).toBeNull(); }); }); From 520061f26d708b1d1a669c439671d4e5f708fa6b Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Sat, 18 Jul 2026 09:59:53 +0000 Subject: [PATCH 2/2] fix(#294): remove N-active count, move layout switcher to header, bold required filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same-PR owner follow-up on the just-shipped filter nowrap/Clear-all-removal change: - The "N active" count (`filterActiveCount`, `.dash-filter-count`/ `.dash-filter-count-host`) is deleted outright, same as Clear-all — DashboardViewerSession.activeFilterCount stays tested app-level state with no display. `.dash-filter-host` collapses back to a single-level scroll viewport (no more `.filter-strip-scroll` wrapper) now that there's no count sibling to keep outside it. The toolbar is hidden whenever there are no filters at every width (was mobile-only), since it now holds nothing else. - The flow preset switcher moves off the filter toolbar and into the header's top row (right after the tile-count chip), as a compact `` — matching + the Workbench's panel-type picker convention (`renderPanelTypePicker` in + `panels.ts`) — in the header's top row right after the tile-count chip, + replacing the four-button `.dash-seg` segmented control that used to live in + the filter toolbar; a select needs far less room, so the whole toolbar width + goes to filters. The required/optional `{name:Type}` filter-name affordance + (shared by the Workbench var-strip and the Dashboard filter bar) is now bold + for a required name instead of a leading `*` glyph. Combobox popovers, + relative-time previews, and filter execution/activation/debounce/recents/ + validation are unchanged. The "hide native scrollbar, allow horizontal + auto-scroll" viewport contract is one shared rule read by both `.var-strip` + and `.dash-filter-host` (previously copy-pasted per surface — review + follow-up), and the scroll viewport carries enough vertical padding that a + focused field's box-shadow ring is never clipped by its `overflow-y: hidden`. + New Playwright coverage (`dashboard-mobile.spec.js`, `workbench-var-strip.spec.js`) + pins the never-wrap/scroll contract, the focus-ring padding, the bold/muted + required-vs-optional styling, and the relocated layout select, for both surfaces in a real browser (invisible to the happy-dom unit suite). + **Dead-CSS cleanup** from an audit prompted by the same change: `.dash-skip` + (styled but never constructed by `dashboard.ts` since #286 flipped Dashboard + membership onto `dashboard.tiles[]` — there is no "N not shown" concept left + to display) is deleted from `src/styles.css` and the `dashboard-mobile.html`/ + `.spec.js` fixture. A second dead class, `.dash-grid.is-wide`, turned out to + guard a real, still-intended "Full width" preset behavior that #286's + `flow@1` rewrite never re-wired (`is-report` kept its toggle, `is-wide` lost + its) — filed as **#298** rather than deleted or silently fixed here, since + restoring it is a distinct rendering fix that needs its own >1560px-viewport + regression test. ### Added - **Dashboard v1 contracts, codecs, canonical encoding, and resource limits** diff --git a/src/styles.css b/src/styles.css index 6ed94f7..b5385d7 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1312,18 +1312,17 @@ body.detached-tab .graph-overlay-panel { .ed-toolbar::-webkit-scrollbar { display: none; } /* Shared "hide the native scrollbar, allow horizontal auto-scroll, contain overscroll" viewport contract (#294) — the Workbench var-strip and the - Dashboard filter-strip scroll viewport (`.filter-strip-scroll`, below) read - it off one declaration so the two surfaces can't drift apart. Each keeps its - own layout rule (`.var-strip` is itself the flex/nowrap row; the Dashboard - nests a separate flex row — `.dash-filters` — inside this viewport so the - fixed "N active" status can sit outside it as a sibling). */ + Dashboard filter host (`.dash-filter-host`, below) read it off one + declaration so the two surfaces can't drift apart. Each keeps its own + layout rule (`.var-strip` is itself the flex/nowrap row; the Dashboard + nests a separate flex row — `.dash-filters` — inside this viewport). */ .var-strip, -.filter-strip-scroll { +.dash-filter-host { overflow-x: auto; overflow-y: hidden; scrollbar-width: none; overscroll-behavior-x: contain; -webkit-overflow-scrolling: touch; } .var-strip::-webkit-scrollbar, -.filter-strip-scroll::-webkit-scrollbar { display: none; } +.dash-filter-host::-webkit-scrollbar { display: none; } /* Query-variable strip (#134): one field per detected {name:Type}. A single row that swipe-scrolls horizontally when it overflows (many variables, or a narrow/mobile workbench) — never wraps to a second line and never clips. @@ -1350,14 +1349,14 @@ body.detached-tab .graph-overlay-panel { white-space: nowrap; } .var-name::after { content: ':'; color: var(--fg-mute); } -/* Required/optional-variable affordance (#165): a parameter confined to - optional filter blocks may stay blank — blank keeps its predicates out - instead of gating Run or a dashboard tile. Optional names are muted; a - required field's name is additionally marked with a leading "*" before the - colon (`name*:`) so the strip reads required vs optional at a glance (the - input title spells it out either way). */ +/* Required/optional-variable affordance (#165, restyled 2026-07-18): a + parameter confined to optional filter blocks may stay blank — blank keeps + its predicates out instead of gating Run or a dashboard tile. Optional + names are muted; a required field's name is bold instead of starred, so + the strip reads required vs optional at a glance without an asterisk + glyph (the input title spells it out either way). */ .var-field.is-optional .var-name { color: var(--fg-mute); } -.var-field:not(.is-optional) .var-name::after { content: '*' ':'; } +.var-field:not(.is-optional) .var-name { font-weight: 700; } .var-input { height: 24px; width: 150px; padding: 0 8px; background: var(--bg-input); color: var(--fg); @@ -2148,51 +2147,42 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } gap: 12px; padding: 11px 20px; background: var(--bg-header); border-bottom: 1px solid var(--border); flex-wrap: wrap; } -/* Layout toolbar (#149 D2) — becomes the filter bar in D4. `flex-wrap: nowrap` - at every width (#294): filters used to wrap the toolbar onto extra rows once - several `{name:Type}` params were present, growing the toolbar's height as - the dashboard's data area shrank. */ +/* Filter toolbar (#149 D2, now filters-only since the layout switcher moved to + the header — 2026-07-18). `flex-wrap: nowrap` at every width (#294): filters + used to wrap the toolbar onto extra rows once several `{name:Type}` params + were present, growing the toolbar's height as the dashboard's data area + shrank. Hidden entirely (`:not(.has-filters)`) at every width, not just + mobile, now that it holds nothing else worth showing when there are no + filters. */ .dash-toolbar { display: flex; align-items: center; gap: 12px; flex-wrap: nowrap; padding: 8px 20px; background: var(--bg-header); border-bottom: 1px solid var(--border); } -.dash-seg { display: inline-flex; background: var(--bg-chip); border: 1px solid var(--border); border-radius: 8px; padding: 2px; } -.dash-seg-btn { - appearance: none; border: none; background: none; cursor: pointer; - padding: 4px 12px; border-radius: 6px; font: 500 12px var(--ui); color: var(--fg-mute); -} -.dash-seg-btn:hover { color: var(--fg); } -.dash-seg-btn.is-active { background: var(--bg); color: var(--fg); box-shadow: 0 1px 2px rgba(0,0,0,.12); } +.dash-toolbar:not(.has-filters) { display: none; } +/* The flow preset switcher (2026-07-18: a `.result-panel-select` dropdown in + the header's top row, replacing the old `.dash-seg` segmented button group + that lived in the filter toolbar — see dashboard.ts). */ .dash-layout-wrap { display: inline-flex; align-items: center; gap: 8px; flex: 0 0 auto; } -.dash-seg-label { font-size: 11px; color: var(--fg-faint); } /* Global filter bar (#149 D3) — one field per detected {name:Type}, reusing the workbench's var-field/var-name/var-input classes. - #294: the field region lives in its own horizontally-scrolling viewport - (`.filter-strip-scroll`, the shared scroll contract above, following the + #294: the field region IS its own horizontally-scrolling viewport + (`.dash-filter-host`, the shared scroll contract above, following the workbench `.var-strip` / editor-tabs precedent) so excess fields scroll - instead of wrapping, while the fixed "N active" status - (`.dash-filter-count-host`) stays outside it on the same row at every - viewport width — not just the ≤768px breakpoint. `align-self: center` on - the count keeps it vertically centered against the field row even when a - field grows taller (e.g. a relative-time field's resolved-value preview - line), rather than pinned to the row's top edge. The 4px vertical padding - on the scroll viewport keeps a focused field's box-shadow ring from being - clipped by its `overflow-y: hidden`. */ -.dash-filter-host { - display: flex; align-items: flex-start; gap: 12px; - flex: 1 1 auto; min-width: 0; -} -.filter-strip-scroll { flex: 1 1 auto; min-width: 0; padding: 4px 0; } -.dash-filter-count-host { flex: 0 0 auto; align-self: center; } + instead of wrapping, at every viewport width — not just the ≤768px + breakpoint. No visible Clear-all or "N active" count sit beside it + (2026-07-18 owner override — see the header comment in dashboard.ts). The + 4px vertical padding keeps a focused field's box-shadow ring from being + clipped by the viewport's `overflow-y: hidden`. */ +.dash-filter-host { flex: 1 1 auto; min-width: 0; padding: 4px 0; } .dash-filters { display: flex; align-items: flex-start; gap: 14px; flex-wrap: wrap; } /* `.dash-filters` is shared with the detached Data view's filter row (`.detached-filter-row .dash-filters`, above), which keeps the default wrap - — only the Dashboard toolbar's instance, nested in `.filter-strip-scroll`, + — only the Dashboard toolbar's instance, nested in `.dash-filter-host`, needs the never-wrap contract, so the override is scoped to that ancestor rather than changed on the shared class itself. `.var-field` already sets `flex-shrink: 0`, so `flex-wrap: nowrap` alone is enough for the overflow to reach the scroll viewport above — no extra width rule needed. */ -.filter-strip-scroll .dash-filters { flex-wrap: nowrap; } +.dash-filter-host .dash-filters { flex-wrap: nowrap; } .dash-icobtn { width: 30px; height: 30px; display: inline-flex; align-items: center; justify-content: center; background: var(--bg-chip); color: var(--fg-mute); border: 1px solid var(--border); @@ -2214,7 +2204,6 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } .dash-fav { color: var(--accent); } .dash-src { font-family: var(--mono); } .dash-dot { width: 6px; height: 6px; border-radius: 6px; background: #22C55E; } -.dash-skip { font-size: 11px; color: var(--fg-faint); } .dash-updated { font-size: 11px; color: var(--fg-faint); font-family: var(--mono); } .dash-btn { display: inline-flex; align-items: center; gap: 6px; height: 30px; padding: 0 12px; @@ -2228,7 +2217,8 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } preset's columns, or 1 on mobile), set inline by the flow renderer. The container must NOT impose its own `grid-template-columns`, or it would lay the rows out side by side (regression: full-width showed 3 rows in a line, 3-col - showed 9). The four-way layout switcher lives in the toolbar (#149 D2, #184). */ + showed 9). The four-way layout switcher lives in the header (#149 D2, #184; + moved out of the toolbar 2026-07-18). */ .dash-grid { display: flex; flex-direction: column; gap: 14px; padding: 18px 20px 40px; max-width: 1560px; margin: 0 auto; @@ -2372,7 +2362,6 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } } .dash-back-label, .dash-fav, - .dash-skip, .dash-src, .dash-updated, .dash-spacer, @@ -2387,7 +2376,6 @@ table.res-table tbody tr:hover td.idx { background: var(--bg-hover); } .dash-refresh { padding: 0; justify-content: center; } .dash-toolbar { padding: 6px 10px; } - .dash-toolbar:not(.has-filters) { display: none; } .dash-grid, .dash-grid.is-wide, diff --git a/src/ui/dashboard.ts b/src/ui/dashboard.ts index fcaa50e..a034b7c 100644 --- a/src/ui/dashboard.ts +++ b/src/ui/dashboard.ts @@ -42,7 +42,7 @@ import { analyzeParameterizedSources, fieldControls } from '../core/param-pipeli import type { ValidationMode } from '../core/param-pipeline.js'; import { queryDashboardRole } from '../dashboard/model/workspace-semantics.js'; import { renderKpiCards, KPI_STREAM_ARIA } from './kpi-panel.js'; -import { buildFilterBar, filterActiveCount } from './filter-bar.js'; +import { buildFilterBar } from './filter-bar.js'; import type { FilterBarApp } from './filter-bar.js'; import { createDashboardViewerSession } from '../dashboard/application/dashboard-viewer-session.js'; import type { @@ -97,22 +97,27 @@ const valueString = (value: unknown): string => (typeof value === 'string' ? value : value == null ? '' : String(value)); /** - * Build a segmented control (the flow preset switcher). `options` are - * `[value,label,title?]`; exactly one reads active from `getActive()`; - * `onPick(value)` fires on a click; `sync()` repaints the active button. + * Build the flow preset switcher as a compact ` + + + + + +
clickhouse.example.internal Updated 12:34 @@ -29,24 +36,10 @@
-
- Layout -
- - - - -
-
-
-
- 3 active -
+
-
-
Layout-only toolbar
-
+
Empty toolbar (no filters)
Panel one
Panel two
@@ -61,6 +54,10 @@ import { wireComboInput } from '/src/ui/combobox.js'; const filters = document.querySelector('.dash-filters'); + // 'version' is the one optional field here — real coverage for the bold + // (required) vs muted (optional) `.var-name` contrast (2026-07-18: replaced + // the leading "*" glyph). + const OPTIONAL = new Set(['version']); for (const name of ['region', 'service', 'environment', 'owner', 'status', 'version']) { const field = buildRecentField({ document, name, type: 'String', value: '', baseTitle: `${name}: String`, @@ -69,15 +66,14 @@ }); wireComboInput(field, { onValueInput: () => {}, onCommit: () => {} }); const label = document.createElement('label'); - label.className = 'var-field'; + label.className = 'var-field' + (OPTIONAL.has(name) ? ' is-optional' : ''); label.innerHTML = `${name}`; label.append(field.el); filters.append(label); } // A relative-time field's resolved-value preview line (#294 review finding) // makes this field taller than the plain recent-value fields above — real - // coverage for the fixed count staying centered against a taller row, - // instead of pinned to the row's top edge. + // coverage for the focused-field padding/clipping fix. const timeField = buildRelativeTimeField({ document, name: 'since', type: 'DateTime', value: 'now', baseTitle: 'since: DateTime', wallNow: () => Date.parse('2026-07-18T12:00:00Z'), diff --git a/tests/e2e/dashboard-mobile.spec.js b/tests/e2e/dashboard-mobile.spec.js index 9b64288..85c0d33 100644 --- a/tests/e2e/dashboard-mobile.spec.js +++ b/tests/e2e/dashboard-mobile.spec.js @@ -17,7 +17,7 @@ test.describe('Dashboard mobile layout', () => { await expect(page.getByRole('button', { name: 'Refresh dashboard' })).toBeVisible(); await expect(page.locator('.dash-back-label')).toBeHidden(); await expect(page.locator('.dash-refresh-label')).toBeHidden(); - for (const selector of ['.dash-fav', '.dash-skip', '.dash-src', '.dash-updated']) { + for (const selector of ['.dash-fav', '.dash-src', '.dash-updated', '.dash-layout-wrap']) { await expect(page.locator(selector)).toBeHidden(); } @@ -106,7 +106,7 @@ test.describe('Dashboard mobile layout', () => { test('scrolls filters in one row while fixed combobox content escapes clipping', async ({ page }) => { await openAt(page, 390); - const scroll = page.locator('.filter-strip-scroll'); + const scroll = page.locator('.dash-filter-host'); const filters = page.locator('.dash-filters'); const before = await scroll.evaluate((node) => ({ clientWidth: node.clientWidth, @@ -121,13 +121,8 @@ test.describe('Dashboard mobile layout', () => { expect(before.overflowX).toBe('auto'); expect(await filters.evaluate((node) => getComputedStyle(node).flexWrap)).toBe('nowrap'); - // The fixed count status is a sibling of the scroll region, not inside it — - // scrolling the fields must not move it. - const countBefore = await page.locator('.dash-filter-count-host').evaluate((node) => node.getBoundingClientRect().left); await scroll.evaluate((node) => { node.scrollLeft = node.scrollWidth; }); expect(await scroll.evaluate((node) => node.scrollLeft)).toBeGreaterThan(0); - const countAfter = await page.locator('.dash-filter-count-host').evaluate((node) => node.getBoundingClientRect().left); - expect(countAfter).toBe(countBefore); await scroll.evaluate((node) => { node.scrollLeft = 0; }); const first = page.getByRole('combobox', { name: 'region' }); @@ -152,69 +147,106 @@ test.describe('Dashboard mobile layout', () => { await expect(first).toHaveValue('alpha'); }); - test('removes an empty toolbar only on mobile', async ({ page }) => { + test('removes an empty toolbar at every viewport width (2026-07-18: the layout switcher no longer lives there, so an empty toolbar is never worth showing)', async ({ page }) => { await openAt(page, 390); await expect(page.locator('#no-filter-toolbar')).toBeHidden(); - await page.setViewportSize({ width: 769, height: 844 }); - await expect(page.locator('#no-filter-toolbar')).toBeVisible(); + await page.setViewportSize({ width: 1100, height: 844 }); + await expect(page.locator('#no-filter-toolbar')).toBeHidden(); + }); + + test('marks a required filter name bold instead of a leading asterisk; an optional name stays muted (2026-07-18)', async ({ page }) => { + await openAt(page, 1100, 800); + const names = await page.locator('.dash-filters .var-name').evaluateAll((nodes) => nodes.map((node) => ({ + // The old convention prepended a literal "*" via `::after { content }` — + // never part of `textContent` even before this change — so the real + // regression check is the CSS-generated content string itself, not the + // DOM text (which never had an asterisk to begin with). + afterContent: getComputedStyle(node, '::after').content, + fontWeight: getComputedStyle(node).fontWeight, + optional: node.closest('.var-field').classList.contains('is-optional'), + }))); + const required = names.filter((n) => !n.optional); + const optional = names.filter((n) => n.optional); + expect(required.length).toBeGreaterThan(0); + expect(optional.length).toBeGreaterThan(0); + for (const n of [...required, ...optional]) expect(n.afterContent).not.toContain('*'); + for (const n of required) expect(Number(n.fontWeight)).toBeGreaterThanOrEqual(700); + for (const n of optional) expect(Number(n.fontWeight)).toBeLessThan(700); }); - test('desktop: filters stay on one row and scroll horizontally, the layout switcher and count stay fixed, no Clear-all control exists (#294)', async ({ page }) => { + test('desktop: filters stay on one row and scroll horizontally, no Clear-all or count control exists (#294)', async ({ page }) => { await openAt(page, 1100, 800); await expect(page.locator('.dash-filter-clear-all')).toHaveCount(0); + await expect(page.locator('.dash-filter-count')).toHaveCount(0); + await expect(page.locator('.dash-filter-count-host')).toHaveCount(0); const toolbar = page.locator('.dash-toolbar.has-filters'); const before = await toolbar.evaluate((node) => node.getBoundingClientRect().height); const layout = await page.evaluate(() => { - const layoutWrap = document.querySelector('.dash-layout-wrap'); - const scroll = document.querySelector('.filter-strip-scroll'); + const host = document.querySelector('.dash-filter-host'); const filters = document.querySelector('.dash-filters'); - const count = document.querySelector('.dash-filter-count-host'); const fields = [...filters.querySelectorAll('.var-field')]; - const center = (rect) => rect.top + rect.height / 2; return { toolbarWrap: getComputedStyle(document.querySelector('.dash-toolbar')).flexWrap, - layoutWrapVisible: getComputedStyle(layoutWrap).display !== 'none', - layoutCenter: center(layoutWrap.getBoundingClientRect()), filtersWrap: getComputedStyle(filters).flexWrap, - scrollWidth: scroll.scrollWidth, - clientWidth: scroll.clientWidth, + scrollWidth: host.scrollWidth, + clientWidth: host.clientWidth, fieldTops: fields.map((field) => field.getBoundingClientRect().top), fieldWidths: fields.map((field) => field.getBoundingClientRect().width), - countCenter: center(count.getBoundingClientRect()), - countVisible: getComputedStyle(count.querySelector('.dash-filter-count')).display !== 'none', pageOverflow: document.documentElement.scrollWidth - innerWidth, - // #294 review finding: the fixture's last field is a relative-time - // field with a resolved-value preview line beneath the input, making - // its `.var-field` taller than the plain recent-value fields — this - // exercises the count staying vertically centered against a row taller - // than one line, not just pinned to its top edge. - scrollPaddingTop: parseFloat(getComputedStyle(scroll).paddingTop), - scrollPaddingBottom: parseFloat(getComputedStyle(scroll).paddingBottom), + // The scroll viewport's `overflow-y: hidden` must not clip a focused + // field's box-shadow ring (`.var-input:focus`'s 3px spread) — the + // vertical padding is the buffer that keeps it visible (#294 review + // finding, exercised against the fixture's taller relative-time field). + hostPaddingTop: parseFloat(getComputedStyle(host).paddingTop), + hostPaddingBottom: parseFloat(getComputedStyle(host).paddingBottom), }; }); expect(layout.toolbarWrap).toBe('nowrap'); - expect(layout.layoutWrapVisible).toBe(true); expect(layout.filtersWrap).toBe('nowrap'); expect(layout.scrollWidth).toBeGreaterThan(layout.clientWidth); expect(Math.max(...layout.fieldTops) - Math.min(...layout.fieldTops)).toBeLessThan(2); expect(Math.min(...layout.fieldWidths)).toBeGreaterThan(150); - expect(Math.abs(layout.countCenter - layout.layoutCenter)).toBeLessThan(3); - expect(layout.countVisible).toBe(true); expect(layout.pageOverflow).toBeLessThanOrEqual(0); - // The scroll viewport's `overflow-y: hidden` must not clip a focused - // field's box-shadow ring (`.var-input:focus`'s 3px spread) — the vertical - // padding is the buffer that keeps it visible. - expect(layout.scrollPaddingTop).toBeGreaterThanOrEqual(3); - expect(layout.scrollPaddingBottom).toBeGreaterThanOrEqual(3); - - // Scrolling the fields reaches the final one and doesn't grow the toolbar - // or move the layout switcher / count off their row. - await page.locator('.filter-strip-scroll').evaluate((node) => { node.scrollLeft = node.scrollWidth; }); + expect(layout.hostPaddingTop).toBeGreaterThanOrEqual(3); + expect(layout.hostPaddingBottom).toBeGreaterThanOrEqual(3); + + // Scrolling the fields reaches the final one and doesn't grow the toolbar. + await page.locator('.dash-filter-host').evaluate((node) => { node.scrollLeft = node.scrollWidth; }); const after = await toolbar.evaluate((node) => node.getBoundingClientRect().height); expect(after).toBe(before); const lastField = page.locator('.dash-filters .var-field').last(); await expect(lastField).toBeInViewport(); }); + + test('the layout switcher is a compact select in the header, right after the tile-count chip, matching the workbench panel-picker style (2026-07-18)', async ({ page }) => { + await openAt(page, 1100, 800); + const select = page.locator('.dash-layout-select'); + await expect(select).toBeVisible(); + await expect(select).toHaveClass(/result-panel-select/); + // No more four-button segmented control. + await expect(page.locator('.dash-seg-layout')).toHaveCount(0); + + const geometry = await page.evaluate(() => { + const header = document.querySelector('.dash-header'); + const children = [...header.children]; + const tileCountIndex = children.findIndex((c) => c.classList.contains('dash-fav')); + const layoutWrapIndex = children.findIndex((c) => c.classList.contains('dash-layout-wrap')); + const layoutWrap = children[layoutWrapIndex]; + const tileCount = children[tileCountIndex]; + return { + layoutRightAfterTileCount: layoutWrapIndex === tileCountIndex + 1, + inHeaderNotToolbar: !document.querySelector('.dash-toolbar .dash-layout-wrap'), + sameRow: Math.abs( + (layoutWrap.getBoundingClientRect().top + layoutWrap.getBoundingClientRect().height / 2) + - (tileCount.getBoundingClientRect().top + tileCount.getBoundingClientRect().height / 2), + ) < 2, + }; + }); + expect(geometry).toEqual({ layoutRightAfterTileCount: true, inHeaderNotToolbar: true, sameRow: true }); + + await select.selectOption('columns-2'); + expect(await select.inputValue()).toBe('columns-2'); + }); }); diff --git a/tests/unit/dashboard.test.ts b/tests/unit/dashboard.test.ts index 37d8c6e..7a1c221 100644 --- a/tests/unit/dashboard.test.ts +++ b/tests/unit/dashboard.test.ts @@ -33,7 +33,7 @@ const qs = (root: ParentNode | null, selector: (root as ParentNode).querySelector(selector) as T; const qsa = (root: ParentNode | null, selector: string): T[] => [...(root as ParentNode).querySelectorAll(selector)] as T[]; -/** `el.onclick`'s DOM-lib type takes a `MouseEvent`; every `.dash-btn`/`.dash-seg-btn` +/** `el.onclick`'s DOM-lib type takes a `MouseEvent`; every `.dash-btn` * handler this suite exercises is a real zero-arg (often async) closure `ui/dashboard.ts` * assigns directly — narrower than the lib's declared signature (an assignable * direction, not a fixture gap), so calling through it needs no argument. */ @@ -299,8 +299,14 @@ function dashApp(opts: { } const render = (app: TestApp): Promise => renderDashboard(app as unknown as Parameters[0]); -const seg = (root: ParentNode | null, label: string): HTMLElement | undefined => - qsa(root, '.dash-seg-layout .dash-seg-btn').find((b) => b.textContent === label); +/** The flow preset switcher (2026-07-18: a `