feat(render): Dashboard UX improvements — 12-column grid, smart defaults, card styling#2720
feat(render): Dashboard UX improvements — 12-column grid, smart defaults, card styling#2720jswir wants to merge 33 commits into
Conversation
|
These three automated review comments I understand and agree with
This one is outside my area of competence, I pass it on. It feels when reading this like an LLM made a change for you and it made an expedient but not thoughtful change ...
There are two more comments from the view session that I am going to think about before I pass them on, but I thought I would hand these back to you now. |
|
My AI is pretty unhappy with the storybook deps loading fix. It feels they are expedient, poorly explained and possibly maintenance problem. Again this is not my area of expertise. But until both our AIs are happy that one is a problem. |
|
The AI thinks there is missing "story" coverage: Please fill in/confirm only the unresolved rows in this verification matrix: • gap = 0 behavior (edge case): no explicit story/test currently identified. |
|
When generating error messages, to make validation work better for an LLM responding to vlaidation, it would be great if the errors followed something like this pattern (i.e. this is not the rule, just a guideline) |
|
This pr seems to accidentally include #2746 -- you need to rebase it so it can be judged separately. I think grid dashboards stress the validation framework in a way which makes them impossible to correctly validate and I am working on PR to extend validation which will help this. Also the AI suggests: "Invalid layout values should not still drive layout. Right now # span, # dashboard { columns=... }, and # dashboard { gap=... } are validated, but the raw values are still consumed by the dashboard layout code. Those should log and then fall back to default behavior, not feed broken CSS/layout. |
|
Ok #2750 is merged to main now. I think this PR should be updated to use that new validation ownership machinery for the dashboard child tags. In particular, # span, # subtitle, and # borderless look like dashboard-owned child tags in the same sense that # break is, and I think they should be validated and consumed through the new renderer validation ownership mechanism rather than being resolved globally up front. That would let this PR keep the nice short dashboard markup while fitting cleanly into the new validation architecture. |
- Replace flexbox with CSS Grid (auto-fit + 12-column span system) - Add per-item # span=N tag for explicit grid column control - Add # columns=N tag for fixed equal-column layouts - Add # subtitle="..." tag for dashboard item subtitles - Add # break tag for explicit row breaks - Add # borderless tag for table items without card chrome - Add KPI measure styling (no card, centered, auto-scaling font via cqi) - Add card styling with hover effects and CSS variable-driven theming - Flatten big_value inner card when inside dashboard (no double border) - Fix sparkline cropping by scaling chart SVGs to fill container - Auto-span nest items (tables/charts) to 2 columns by default - Add getFieldLabel utility for snake_case to Title Case conversion - Add dashboard CSS variable defaults and theme pass-through - Add storybook stories for all new layout features Signed-off-by: James Swirhun <james@credibledata.com> Made-with: Cursor
- Add container queries for responsive dashboard layout:
- Below 600px: single column, all spans collapse to full width
- 601-900px: 12-col grid collapses to 6-col, wide spans go full width
- Add configurable gap via # dashboard { gap=N } tag
- Add --malloy-render--dashboard-gap CSS variable with 16px default
- Add dashboard_custom_gap storybook story
Signed-off-by: James Swirhun <james@credibledata.com>
Made-with: Cursor
Made-with: Cursor
- KPI measure titles/subtitles now properly center-aligned (was overridden by the base .dashboard-item-title left-align rule) - Remove redundant CSS width:100% on dashboard tables; the shouldFillWidth prop already adds the .full-width class Made-with: Cursor
The borderless card style applies to any item type, not just tables. Removed the table-only gate so # borderless works on charts too. Made-with: Cursor
Measures now render inside cards by default (same as charts/tables). Use # borderless to opt out of card styling on any dashboard item. Added dashboard_borderless_kpis story to demonstrate the opt-out. Made-with: Cursor
- Remove auto span 2 for nests and max-width 500px for big_value; let the grid auto-fit or explicit # span control sizing - When a group mixes # span items with non-span items, compute a sensible default span for unspanned items based on remaining cols - Add stories: tall chart (size.height=400), size=lg in narrow span, mixed span defaults Made-with: Cursor
Two stories with zero new annotations, mimicking real-world dashboards like malloy-samples/auto_recalls: KPI measures + charts + tables using only pre-existing features (# dashboard, # line_chart, # bar_chart, # currency). Tests that the default auto-fit grid handles legacy dashboards gracefully. Made-with: Cursor
- Label: add uppercase + letter-spacing 0.025em (matches big_value) - Value: 32px (was 28px), line-height 1.2 (matches big_value) - Update CSS variable default to 32px Made-with: Cursor
The table's own overflow:auto handles scrolling; the dashboard-item-value wrapper was adding a redundant horizontal scrollbar. Made-with: Cursor
All dashboard groups now use a 12-column grid instead of auto-fit. Default spans by item type: measures get span 2, nests (charts/tables) get span 4. Items wrap naturally when they exceed 12 columns. Explicit # span still overrides the default. Made-with: Cursor
Measures at span 2 were too narrow for formatted values. New defaults: - Measures: span 3 (4 per row) - Nests (charts/tables): span 6 (2 per row) Made-with: Cursor
- Measure cards use align-self:start so they don't stretch to match taller siblings (charts/tables) - Nest default span now depends on visible column count: <=3 cols -> span 4, <=5 cols -> span 6, >5 cols -> span 8 Made-with: Cursor
Tables or views containing nested children (sparklines, subtables, embedded charts) need full width. Detect via child.isNest() and auto-assign span 12. Made-with: Cursor
Tests that a table containing an embedded bar chart column auto-detects the sub-nest and gets span 12 (full width). Made-with: Cursor
Instead of blanket span 12 for any nest with sub-nests, weight each child: scalar columns = 1, nested children (charts/subtables) = 3. Thresholds: <=3 -> span 4, <=5 -> span 6, <=8 -> span 8, >8 -> span 12. E.g. brand + product_count + total_retail + bar_chart = 1+1+1+3 = 6 -> span 6 instead of 12. Made-with: Cursor
- Validate span (1-12), columns (>0), gap (>=0) in validateFieldTags - Touch span/subtitle/borderless in resolveBuiltInTags to prevent false "unknown tag" warnings in headless validation - Read columns/gap in resolveDashboardTags and use resolved config in Dashboard component instead of re-reading tags at render time - Fix table horizontal scrollbar: only clip overflow-x on fill-width tables inside dashboard cards, preserving scroll on wide tables Made-with: Cursor
- Replace fragile style*= CSS selectors with data-span attribute - Preserve null maxTableHeight (no-limit mode) instead of collapsing to 361 - Skip span overrides in columns mode (span and columns are mutually exclusive) - Warn when # span is used inside a columns-mode dashboard - Fix invalid // CSS comment to /* */ - Use strict undefined check in getFieldLabel so # label="" is respected - Add !important explanation comment on sparkline chart overrides - Remove redundant span tag read in DashboardItem (spanOverride handles it) Signed-off-by: James Swirhun <james@credibledata.com>
Move container-type: inline-size and cqi font scaling to grid-only context so KPI measure cards size based on content in flex layout. The inline-size containment was collapsing cards when combined with width: fit-content. Also pre-bundle Vite deps to fix Storybook first-load failures. Signed-off-by: James Swirhun <james@credibledata.com> Made-with: Cursor
…dation messages - Fix falsy numeric handling: gap=0 and columns=0 no longer ignored - Replace render-time tag reads with setup-time resolved properties (label, subtitle, span, break, borderless) on FieldBase - Delete field-label-utils.ts; use field.getLabel() throughout - Move dashboard-specific CSS from big-value.css to dashboard.css - Add explanatory comments for storybook optimizeDeps config - Rewrite validation messages to follow structured format: Invalid <tag-path> on '<field>': expected <constraint>, got <value>. Fix: <example>. - Add dashboard_gap_zero story for edge case coverage Signed-off-by: James Swirhun <james@credibledata.com>
Move span, subtitle, break, and borderless tag resolution from global cross-cutting into dashboard-specific context so these tags are only consumed when the field is a dashboard child. Register them as childOwnedPaths in renderer-validation-specs so the ownership model drives unread-tag warnings correctly. Signed-off-by: James Swirhun <james@credibledata.com>
I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 494df73 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: f5d8c7a I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 9e281ab I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 77d7c17 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: e7203da I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 5a17044 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: b21d1d5 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 98fa36a I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 68debb4 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 10302d1 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 45e4327 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 64d75be I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 44d2518 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 95256d3 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: a3234f3 Signed-off-by: James Swirhun <james@credibledata.com>
…tent-aware collapse Replace brittle 12-column grid with per-span CSS rules with a per-row approach. Each row group gets its own grid-template-columns computed from item span proportions (e.g. 8fr 4fr), and a content-aware collapse class (sm/md/lg) based on minimum viable width. Rows collapse independently: all items proportional or all stacked. - Remove grid-column: span N from individual items - Compute frTemplate per row group from span proportions - Bucket collapse thresholds: 400px (KPI rows), 600px (mixed), 900px (wide) - Columns mode uses repeat(N, 1fr) with same collapse buckets - Rename stories with _flex/_grid suffixes, reorder flex-first - Remove debug console.log statements Signed-off-by: James Swirhun <james@credibledata.com>
# Conflicts: # packages/malloy-render/src/render-field-metadata.ts
The merge with main silently took this branch's rewritten error messages over malloydata#2774's wording. Those message rewrites were scope drift from this PR's goal (dashboard UX) and malloydata#2774's tests check for the upstream wording. Revert the pre-existing validator messages to main's format; keep only the net-new dashboard block.
Add cases for out-of-range # span (too high, zero), non-positive # dashboard.columns, negative # dashboard.gap, and the span-with-columns conflict warn. Closes the gap noted in validation.md's pre-PR checklist — each dashboard validation rule now has a test.
Centralize item min-widths (MIN_WIDTH_MEASURE, MIN_WIDTH_ITEM) and collapse buckets (COLLAPSE_BUCKETS) at the top of dashboard.tsx. Replace duplicated <=400/<=600/else ternaries with bucketFor(). Sync flex-mode min-widths in dashboard.css to match the TSX constants (300/120 in both places) and fix per-row columnsMinWidth to use actual content type instead of a hardcoded 120 — this was why rows with charts/tables in columns=N mode were stacking too late.
I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: a819411 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: c3edb24 I, James Swirhun <james@credibledata.com>, hereby add my Signed-off-by to this commit: 0480a30 Signed-off-by: James Swirhun <james@credibledata.com>
Drops out-of-range columns, gap, and span values in resolveDashboardTags so the layout falls back to its default behavior instead of feeding the bad value into CSS (e.g. repeat(-3, 1fr), gap: -5px, span 15 of 12). The renderer validation contract from malloydata#2774 already logs a clear error when the user supplies these values; this change makes sure the dashboard keeps rendering correctly while the user fixes the tag. Addresses mtoy's Apr 4 review item on PR malloydata#2720. Bounds mirror the validator at render-field-metadata.ts:485-525: columns: positive integer gap: >= 0 span: integer 1..12 The renderer exposes no public hook for inspecting a field's resolved tag config from a test, so the new behavior is covered by three Storybook fixtures under DEFENSIVE FALLBACKS in dashboard.stories.malloy (columns=-3, gap=-5, span=15). The existing validator tests in render-validator.spec.ts already cover the error-logging side. Signed-off-by: Monty Lennie <montylennie@gmail.com>
CI's prettier check fails on four pre-existing format issues in dashboard.tsx that landed on feat/dashboard-ux. Running prettier --write on just those lines clears them so the rest of the PR can be reviewed against a green CI. No logic change. Signed-off-by: Monty Lennie <montylennie@gmail.com>
Drop the inaccurate parenthetical from the dashboard_fallback_bad_span story comment; the previous wording claimed "4 for nests with one inner field" but the example has two visible children, and the actual default comes from computeSpan's weight logic. Comment-only. Signed-off-by: Monty Lennie <montylennie@gmail.com>
Replace the em-dash with a period and capitalize the next sentence; otherwise no change to meaning. Comment-only cleanup made while touching the surrounding lines in the prior prettier commit. Signed-off-by: Monty Lennie <montylennie@gmail.com>
fix(render): clamp invalid dashboard layout values
|
In collaboration with Claude -- I have read or written all of this ... a little more verbose than I would reply myself but I am letting Claude decide how much to write: First off — this is looking great. The setup-time resolution, the defensive fallbacks, and the error-message wording are all exactly what I was hoping for, and the responsiveness to the earlier rounds shows. Thank you. A couple of the things I flagged earlier I communicated badly, and you (reasonably) did extra work to satisfy contradictory asks. Let me clear those up. And there's one new thing that recent renderer work has me thinking about that I'd like to get ahead of in this PR.
Back in March I said "resolve dashboard item metadata at setup time, stop reading tags at render time." Then in April I said "consume span/subtitle/borderless through the #2750 ownership mechanism rather than resolving globally up front." Those two notes point in opposite directions, and you ended up doing both: resolving the child tags at setup time in resolveBuiltInTags and declaring them in childOwnedPaths. That's on me for not reconciling them. The setup-time resolution is the one I actually want — it's what docs/validation.md calls the default. Given that, the childOwnedPaths entries are now redundant: the setup-time reads already mark those tags as read, so the unread-tag detector never fires on them. I confirmed this — emptying childOwnedPaths entirely leaves the "no warnings for # span / # subtitle / # borderless / # break" tests green. So: keep the setup-time resolution, and drop the span/subtitle/borderless/break entries from renderer-validation-specs.ts (the dashboard spec can go back to empty). One mechanism, not two. Smaller related thought, not a blocker: the resolved values currently live as four new fields on FieldBase (_resolvedSpan, _resolvedBorderless, …). Those are dashboard-only concepts sitting on the universal field base. If it's cheap, I'd lean toward stashing them in a per-child config object rather than growing the base class — but I don't want to over-rotate on this if the base-class route is genuinely simpler, so your call.
When I said "move the dashboard overrides into dashboard-owned styling," that got actioned as relocating the .dashboard-item… selectors out of big-value.css and into dashboard.css — which is good, but dashboard.css now reaches the other way into big-value's internals (.malloy-big-value-card, -value-row, -comparison, …). The leak moved sides rather than closing. What I was actually after is reducing how much either component knows about the other's internal class names, not just which file the selectors live in. The clean shape is big-value exposing an "embedded/flattened" mode that dashboard opts into, instead of dashboard overriding big-value's guts. That's more than a CSS shuffle, so I don't want to block the PR on it — but let's name it as known debt so it doesn't quietly become a regression source when the two evolve apart.
This is the one from recent experience. I was copying and re-theming dashboards by extending views, and hit a wall that this PR should get ahead of. Right now the grid-vs-flex decision is inferred: grid if columns set, OR gap set, OR any item has span, OR any item has break. That makes the mode hard to override when you copy a dashboard: flex → grid is fine — add # dashboard { columns = 3 } to the copy and you're in grid. I think the real fix isn't a new mode tag — it's that span and break shouldn't be mode triggers at all: break is meaningful in flex. The row-grouping already runs in both modes (nonDimensionsGrouped splits on break regardless of grid/flex), so in flex a break just forces a new wrapping row. It doesn't need grid and shouldn't switch you into it. So my proposal: narrow the grid signal to columns/gap only. Make span a grid-scoped sizing hint (warn if used without a grid). Let break work in both modes. The payoff is that the mode is then controlled entirely at the view level, which is cleanly negatable — so grid↔flex round-trips on a copy with a single annotation, and stray spans become harmless no-ops in flex instead of forcing a layout you can't escape. (Happy to share the little probe I ran if it'd help — it walks the resolved tags for each override case.) Minor / logistics The new validator tests are good and cover the right branches. One gap: they assert that an error/warning fires, not what it says. Since I asked for the Invalid on '': expected … got … Fix: … shape, could you add one assertion that pins Fix: (or the "expected … got") so a regression back to a terse message gets caught? And a gap = 0 "no error" assertion would close the last row of that verification matrix. |
Summary
# span,# dashboard { columns }, or# dashboard { gap }tags use a flex layout where items size naturally based on content. When any of those tags are present, the dashboard switches to a 12-column CSS grid with smart default spans (measures → span 3, nests → span 4-12 based on weighted child content).big_valuestyling (uppercase, letter-spacing, 32px value). KPIs self-size height instead of stretching to match adjacent items.# span = Ntag: Explicit 12-column grid span (1-12) on any dashboard item, overrides defaults. Triggers grid mode.# breaktag: Starts a new row before the annotated field.# subtitletag: Adds subtitle text below the item title.# borderlesstag: Opt out of card styling on any item type.# dashboard { columns = N }tag: Force N equal columns instead of 12-col grid.# dashboard { gap = N }tag: Custom gap spacing in pixels.# labeltag: Custom display label; default is snake_case → Title Case conversion.@containerqueries collapse grid to single column at ≤600px, halve to 6 columns at ≤900px. Flex mode wraps naturally.shouldFillWidth: true, no double scrollbars (overflow-x clipped only on fill-width tables, preserving scroll on wide tables with many columns).span(1-12),columns(>0),gap(>=0). Dashboard tags resolved at setup time for headless validation support.container-type: inline-sizeandcqifont scaling scoped to grid mode only. In flex mode, KPI cards size dynamically based on title/value content instead of collapsing due to the circular dependency betweenfit-contentwidth and inline-size containment.@duckdb/duckdb-wasm,apache-arrow, etc.) viaoptimizeDeps.includeto fix first-load failures from Vite runtime dep discovery.https://www.loom.com/share/581a55d7cc1c4cd487fec943555d2825