feat(dashboard): table tile header separator and optional alternate row background#2519
feat(dashboard): table tile header separator and optional alternate row background#2519alex-fedotyev wants to merge 6 commits into
Conversation
…ow background Add an always-on 1px separator between a table tile's sticky header and its rows, and a new "Alternate Row Background" display setting (off by default) that zebra-stripes builder table tiles. Both work in light and dark color modes. - Separator: inset box-shadow on the sticky thead using `--color-border`, so it stays pinned under `border-collapse: collapse` where a collapsed border would scroll away with the cell content. - Striping: new optional `alternateRowBackground` boolean on the builder chart config, gated in Display Settings to builder table tiles (mirrors `groupByColumnsOnLeft`). Renders via a new `--color-bg-table-stripe` token defined in both the hyperdx and clickstack themes. Striped rows use the stronger `--color-bg-highlighted` on hover so the row under the cursor stays visible over a stripe. HDX-4601 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 450d6d6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
Greptile SummaryThis PR adds two readability improvements to dashboard table tiles: an always-on 1 px header separator (via
Confidence Score: 5/5Safe to merge — changes are purely additive and presentational; the new field defaults to absent/false so all existing tiles are unaffected. All code paths (schema, serialization, form state, UI gate, CSS) are correctly wired and thoroughly tested. The box-shadow trick for the header separator is a well-known workaround for the border-collapse/sticky-header rendering bug. The new CSS token is distinct from the muted hover color by design, and the compound selector specificity is correctly calculated and documented. No data mutation, migration, or breaking change is involved. packages/app/src/components/ChartEditor/tests/utils.test.ts — builder-path coverage for the new alternateRowBackground pick is absent; all other files look solid. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SharedChartSettingsSchema\nalternateRowBackground?: boolean] --> B[SavedChartConfigSchema\nSQL & builder configs]
B --> C[ChartEditor/utils.ts\nconvertFormStateToSavedChartConfig\nconvertFormStateToChartConfig]
C --> D[EditTimeChartForm.tsx\ndestructure + setValue]
D --> E[ChartDisplaySettingsDrawer\nshowTableOptions = Table\nshowBuilderTableOptions = Table && !sql]
E --> F{configType}
F -->|sql| G[alternateRowBackground toggle only]
F -->|builder| H[groupByColumnsOnLeft toggle\n+ alternateRowBackground toggle]
E --> I[onChange → config persisted]
I --> J[DBTableChart.tsx\nalternateRowBackground = !!queriedConfig.alternateRowBackground]
J --> K[Table component\nstyles.stripedRow on odd virtualRow.index]
K --> L[HDXMultiSeriesTableChart.module.scss\n--color-bg-table-stripe\n.tableRow.stripedRow:hover → --color-bg-highlighted]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[SharedChartSettingsSchema\nalternateRowBackground?: boolean] --> B[SavedChartConfigSchema\nSQL & builder configs]
B --> C[ChartEditor/utils.ts\nconvertFormStateToSavedChartConfig\nconvertFormStateToChartConfig]
C --> D[EditTimeChartForm.tsx\ndestructure + setValue]
D --> E[ChartDisplaySettingsDrawer\nshowTableOptions = Table\nshowBuilderTableOptions = Table && !sql]
E --> F{configType}
F -->|sql| G[alternateRowBackground toggle only]
F -->|builder| H[groupByColumnsOnLeft toggle\n+ alternateRowBackground toggle]
E --> I[onChange → config persisted]
I --> J[DBTableChart.tsx\nalternateRowBackground = !!queriedConfig.alternateRowBackground]
J --> K[Table component\nstyles.stripedRow on odd virtualRow.index]
K --> L[HDXMultiSeriesTableChart.module.scss\n--color-bg-table-stripe\n.tableRow.stripedRow:hover → --color-bg-highlighted]
Reviews (5): Last reviewed commit: "fix(dashboard): persist alternateRowBack..." | Re-trigger Greptile |
E2E Test Results✅ All tests passed • 228 passed • 3 skipped • 1426s
Tests ran across 4 shards in parallel. |
The builder-table-only Display Settings block now holds two options (Group By column ordering and Alternate Row Background) under one gate, so `showGroupByColumnsOnLeft` no longer describes what it controls. Rename it to `showBuilderTableOptions`. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the naming point from the review in 39a99ac: renamed |
Deep ReviewScope: 14 files vs base ✅ No critical issues found. 🟡 P2 — recommended
🔵 P3 nitpicks (1)
Reviewers (8): correctness, testing, maintainability, project-standards, agent-native, learnings-researcher, kieran-typescript, api-contract. Testing gaps:
|
| <CheckBoxControlled | ||
| control={control} | ||
| name="alternateRowBackground" | ||
| size="xs" | ||
| label="Alternate Row Background" | ||
| /> |
There was a problem hiding this comment.
Is there any reason we couldn't enable this option for sql tiles as well?
There was a problem hiding this comment.
Good call, no reason to keep it builder-only. The striping is purely presentational (it just tints alternating rendered rows), so it works the same on any table tile regardless of how the data was produced. I moved alternateRowBackground from the builder config onto the shared chart settings so raw SQL table configs persist it too, and split the gate here: Group By column ordering stays builder-only (it needs the builder select structure to know which columns are group-by keys), while Alternate Row Background now shows for every table tile. Done in e81b3ef.
There was a problem hiding this comment.
Actually, found a problem, need to fix
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
The zebra striping is purely presentational (it tints alternating rendered rows), so it applies to any table tile regardless of how the data was produced. Move alternateRowBackground from the builder config to SharedChartSettingsSchema so raw SQL table configs persist it too, and split the display-settings gate: Group By column ordering stays builder-only (it needs the builder select structure) while Alternate Row Background shows for every table tile.
…e tiles The form-state converters assemble raw SQL and PromQL configs from an explicit field whitelist, and alternateRowBackground was not in it, so toggling it on a SQL table tile did nothing and the setting was dropped on save. Builder configs were unaffected because they spread the whole form state. Add the field to the save and render pick lists so the toggle persists and renders for every table tile.
2 participants
Two small readability improvements to the dashboard table tile, both verified in light and dark color modes.
HDX-4601
Summary
Add an always-on separator between a table tile's sticky header and its rows, and a new Alternate Row Background display setting (off by default) that zebra-stripes builder table tiles. The header separator makes the boundary clear as rows scroll under the sticky header (it was easy to lose in dark mode, where the header and first row sit close in luminance). The optional striping makes wide tables easier to scan across.
Changes
box-shadowon the sticky<thead>using the--color-bordertoken. I used a shadow rather than aborder-bottombecause the table setsborder-collapse: collapse, under which a collapsed border on aposition: stickyheader renders unreliably (it can scroll away with the cell content). The shadow paints on the sticky element, so it stays pinned.alternateRowBackgroundboolean on the builder chart config (_ChartConfigSchema), surfaced in Display Settings for builder table tiles only, gated exactly like the siblinggroupByColumnsOnLeftoption. Off by default, so existing tiles are unchanged apart from the new separator. The field isz.boolean().optional(), and tiles are stored asMixed, so an absent field reads as off with no migration.--color-bg-table-stripein both thehyperdxandclickstackthemes (dark and light). I added a dedicated token instead of reusing--color-bg-muted: that token is also the row hover color, so reusing it would make the stripe and the hover identical and the hover invisible on striped rows.--color-bg-mutedthat the shared muted hover is nearly imperceptible on it (in both themes). Striped rows therefore use the stronger--color-bg-highlightedon hover, via a compound.tableRow.stripedRow:hoverselector that outranks the existing hover rules, so the row under the cursor stays obvious.Screenshots
Header separator only (default, no striping):
Alternate Row Background enabled:
Row hover stays visible over a stripe:
Verified the same flows in the
clickstacktheme (light and dark) via Storybook.Test plan
make ci-lint(eslint + tsc + stylelint on the changed files)HDXMultiSeriesTableChart.test.tsx(stripe class on odd rows when on, none when off),ChartDisplaySettingsDrawer.test.tsx(toggle shows for builder table only; Apply emitsalternateRowBackground),DBTableChart.test.tsx(prop threads from a builder config;falsefor raw SQL)HDXMultiSeriesTableChartstories, light and dark, in both thehyperdxandclickstackthemes; checked the separator, the striping, and hover over a striped row[ui-states: allow] Styling-only change; the empty / loading / error states are rendered by existing
DBTableChartbranches and are untouched.[viewport: allow] The separator and stripe are width-independent.
What's NOT in this PR (follow-up)
The new persisted field is exposed through the app save path only. External surface parity is tracked separately, matching how
groupByColumnsOnLeftis exposed: