fix(#294): dashboard filter strips never wrap, remove visible Clear-all#297
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GLMprUnwnz4oaz3cnLRKcz
…d required filters
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
`<select class="result-panel-select">` matching the Workbench's
panel-type-picker convention, replacing the four-button `.dash-seg`
segmented control.
- Required vs optional `{name:Type}` filter names (shared by the Workbench
var-strip and the Dashboard filter bar) are now bold instead of a leading
"*" glyph.
Also, from a CSS-orphan audit prompted by this change: removes `.dash-skip`
(dead since #286 flipped Dashboard membership onto `dashboard.tiles[]` — no
"N not shown" concept remains) from styles.css and the e2e fixture. A second
dead class, `.dash-grid.is-wide`, turned out to guard a real, still-intended
"Full width" preset behavior #286's flow@1 rewrite never re-wired — filed as
#298 rather than fixed here (distinct rendering bug, needs its own
>1560px-viewport regression test).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GLMprUnwnz4oaz3cnLRKcz
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Closes #294. PR #293 (Dashboard v1 phases 1-4) shipped a visible Clear all
filter control and an "N active" status that wrapped onto extra toolbar rows
once several
{name:Type}filters were present at desktop widths, growing thetoolbar and shrinking the dashboard's data area. This reverses that specific
UX decision (owner call, per #294): filters now behave like editor tabs and
the existing Workbench variable strip — one stable row, horizontal scroll for
overflow, at every viewport width (not just the ≤768px breakpoint from
#248) — with no visible Clear-all control.
Changes
src/ui/filter-bar.ts: deletedfilterClearAllButton(no remaining UIconsumer).
filterActiveCount(the "N active" status) is unchanged.src/ui/dashboard.ts: filter host is now.dash-filter-host>(
.filter-strip-scrollwrapping the sharedbuildFilterBaroutput, +.dash-filter-count-hostas a fixed sibling). No moreclearAllNode.DashboardViewerSession.clearAllFilters()is untouched — it stays a testedapplication-level operation with no UI trigger.
src/styles.css:.dash-toolbarisflex-wrap: nowrapat every width (was wrap-capableon desktop, nowrap only under the mobile media query).
.filter-strip-scrollviewport (nowrap/overflow-x:auto/hiddenscrollbar) holds the fields;
.filter-strip-scroll .dash-filtersisscoped to nowrap (not the base
.dash-filtersclass, which the detachedData view's filter row also uses and still wraps there — commented in
place).
shared rule read by both the Workbench
.var-stripand the new.filter-strip-scroll, instead of two copy-pasted blocks (reviewfollow-up — the issue explicitly asked for a shared utility here).
this PR: the scroll viewport now carries enough vertical padding that a
focused field's box-shadow focus ring isn't clipped by its
overflow-y: hidden, and the "N active" count isalign-self: centersoit 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 — both are covered by new real-browser regression assertions
(verified to fail without the fix, pass with it).
tests/unit/dashboard.test.ts(no more Clear-allassertions; new coverage for the separate scroll/count DOM regions and for
"no Clear-all at any active count"). Extended
tests/e2e/dashboard-mobile.spec.jswith a desktop-width test (one row,toolbar height stable, count/layout-switcher fixed and aligned, no
Clear-all, no page overflow, focus-ring padding) and a taller relative-time
fixture field. New
tests/e2e/workbench-var-strip.{html,spec.js}pins theWorkbench
.var-strip's pre-existing never-wrap/scroll contract in a realbrowser (it was already correct — CSS wrapping/overflow is invisible to the
happy-dom unit suite, so this generalizes the acceptance requirement to a
real-browser check for both surfaces).
[Unreleased]and roadmap Roadmap to 1.0.0 #68's Dashboard v1 phase 4 (#280): DashboardViewerSession + normative flow@1 layout #286 bullet updated torecord the reversal.
Out-of-scope findings filed separately (not fixed here)
Both predate this issue, surfaced by an independent review pass:
(
buildFilterBaris called withoutariaLabel, unlike the detached view).documented v1 limitation); this PR broadens where it's reachable (every
width, not just mobile) without introducing the underlying gap.
Checklist
npm testpasses (135 files / 3361 tests; per-file coverage gate holds)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updatednpm run test:e2e(chromium + webkit, 86 specs) passes/code-review(high effort, 8-angle fan-out + verification) run; findings appliedCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01GLMprUnwnz4oaz3cnLRKcz