refactor(theme): unify and name chart colors across themes#2276
refactor(theme): unify and name chart colors across themes#2276elizabetdev wants to merge 7 commits into
Conversation
Both themes now consume the same chart palette (categorical 1-10 + semantic success/warning/error + their highlight variants) from a new `packages/app/src/theme/themes/_chart-tokens.scss` partial. Previously the values lived in four byte-similar-but-not-identical blocks across `hyperdx/_tokens.scss` and `clickstack/_tokens.scss` (dark + light each). Behavior change for HyperDX: the categorical palette now leads with ClickStack-style blue (`#437eef`) instead of HyperDX brand green (`#00c28a`), and semantic success becomes Observable green (`#3ca951`) rather than brand green. Single-series HyperDX charts will render in blue going forward; status pills using `getChartColorSuccess()` shift to the more saturated Observable green. This continues the Click UI convergence — chart-color identity stops being a brand differentiator between the two themes. Implementation: - New `_chart-tokens.scss` defines three mixins: `categorical-chart-tokens`, `semantic-chart-tokens`, and the convenience `chart-tokens` that includes both. The same comment about CSS specificity that previously lived inline in `clickstack/_tokens.scss` moves there to explain why scheme blocks must redeclare these vars. - `hyperdx/_tokens.scss` adds `@use '../chart-tokens';` and replaces both inline chart blocks (dark + light) with `@include chart-tokens.chart-tokens;`. Net loss of ~40 lines. - `clickstack/_tokens.scss` does the same. Net loss of another ~40 lines. The "duplication is required" comment is gone now that the duplication is centralized. - `_base-tokens.scss` is unchanged. It already `@include`s `hyperdx-tokens.dark-mode-tokens` for SSR; that mixin transitively includes the chart tokens, so the SSR fallback layer keeps working. JS palette consolidation in `utils.ts` (CHART_PALETTE + CLICKSTACK_CHART_PALETTE -> single CHART_PALETTE, COLORS reorder, detectActiveTheme cleanup) and storybook + doc updates land in follow-up commits in this PR. Verified each partial compiles via `npx sass`. Co-authored-by: Cursor <cursoragent@cursor.com>
Collapses CHART_PALETTE and CLICKSTACK_CHART_PALETTE in `packages/app/src/utils.ts` into a single CHART_PALETTE that mirrors the new `_chart-tokens.scss` partial. Reorders the exported COLORS array so COLORS[0] is blue (matches the new --color-chart-1) and removes the brand-green-first ordering. Knock-on simplifications now that there is only one palette: - `getSemanticChartColor` no longer takes per-theme fallbacks. Signature: `(cssVarName, fallback) -> string`. It still reads the live CSS var first; the fallback only matters for SSR or when `getComputedStyle` throws. - `detectActiveTheme()` is removed entirely. Its only consumer was `getSemanticChartColor`'s per-theme fallback branch. - The SSR/hydration-mismatch caveats in the JSDoc are gone too. Both themes resolve every chart var to the same hex now, so fallback == live value. Verified via `yarn ci:unit src/__tests__/ChartUtils.test.ts` — all 28 tests pass. Tests reference colors positionally as `COLORS[0]`, `COLORS[1]` etc., so reordering is transparent to them. Storybook label reorder and data-viz doc updates land next. Co-authored-by: Cursor <cursoragent@cursor.com>
Updates the storybook label list and the data viz colors guide to match the new shared palette. Both files previously described a per-theme split (HyperDX brand-green-first vs ClickStack blue-first); after this PR's prior commits the palettes are identical, so the docs were factually wrong. Storybook (`ChartColors.stories.tsx`): - Reorders COLOR_LABELS so slot 1 is "Blue (Primary)", matching the new COLORS array and `--color-chart-1`. Old order had "Green (Brand)" at position 0, which would now render as a blue swatch labeled "Green". Data viz doc (`agent_docs/data_viz_colors.md`): - Replaces the per-theme "where the colors live" tables with a single shared palette table. - Adds a "Why one palette across both themes" subsection explaining the two reasons we collapsed the per-theme split (brand-green doubling as --color-chart-success in HyperDX; runtime detectActiveTheme() branch required for the SSR fallback). - Replaces the green-first / blue-first "Per-theme considerations" section with a short note that theme branding now lives in Mantine accent + Click UI globals + chrome, not in chart colors. The "yellow accent stays out of the categorical palette" rationale moves here intact. - Simplifies the "Adding new entries" section: hex changes now land in two places (`_chart-tokens.scss` + `CHART_PALETTE` in `utils.ts`) instead of four. - Updates the pre-merge checklist: "HyperDX <-> ClickStack toggle should *not* change chart colors anymore"; the only legitimate visual diff is theme chrome. - Updates the file reference summary so the new `_chart-tokens.scss` partial appears as the primary entry, with the two `_tokens.scss` files listed as consumers. - Refreshes JS line-number references to match the slimmer `utils.ts`. Co-authored-by: Cursor <cursoragent@cursor.com>
Categorical chart slots are now addressed by hue name
(`--color-chart-blue`, `--color-chart-orange`, etc.) instead of by
index (`--color-chart-1`..`-10`). The numbered vars are removed.
Identity callers can now reference the var that means what they
actually want ("this pill is always cyan" -> `var(--color-chart-cyan)`)
without coupling to the categorical assignment order. Multi-series
charts still pick by index, but that ordering now lives in a single
JS array (`CATEGORICAL_ORDER` in `utils.ts`) rather than being baked
into the CSS var names. Reordering the default series colors no
longer requires SCSS edits.
- _chart-tokens.scss: rename categorical vars to named hues; semantic
vars unchanged
- utils.ts: introduce CATEGORICAL_ORDER, derive COLORS from it, rewire
getColorFromCSSVariable() to look up the named var via a
camelCase->kebab-case slug
- ChartColors.stories.tsx: replace COLOR_LABELS + index math with a
CATEGORICAL_SLOTS table that is name-by-position
- data_viz_colors.md: rewrite categorical sections to describe named
slots + the JS-side ordering layer; update file-reference and
add/reorder recipes
- hyperdx/_tokens.scss: drop the now-stale "1-indexed maps to 0-indexed
COLORS" comment
Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: c96ad57 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 4 — CriticalTouches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD. Why this tier:
Additional context: agent branch ( Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
PR ReviewSolid refactor — collapses the per-theme chart palettes into one shared Must-fix
Nits (optional)
Otherwise looks clean: |
E2E Test Results✅ All tests passed • 179 passed • 3 skipped • 1198s
Tests ran across 4 shards in parallel. |
Co-authored-by: Cursor <cursoragent@cursor.com>
Deep Review✅ No critical issues found. 🟡 P2 -- recommended
🔵 P3 nitpicks (9)
Reviewers (8): correctness, testing, maintainability, project-standards, adversarial, kieran-typescript, julik-frontend-races, agent-native. Testing gaps:
|
PR Review
No critical bugs or security issues found. |
Introduce brand-scoped --color-chart-info (HyperDX Mantine greens, ClickStack chart blue), apply the active theme class on <html> during AppThemeProvider render so DOM reads align with React state, and pass a HyperDX-specific logLevelColorFn from DBTimeChart (useMantineTheme) into formatResponseForTimeChart so info series hex matches Mantine immediately after a brand switch. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Unifies the categorical and semantic chart palettes across the HyperDX and ClickStack themes, then renames the categorical slots from numeric indices (
--color-chart-1..-10) to named hues (--color-chart-blue,--color-chart-orange, …). Theme branding still differentiates UI chrome (Mantine accent, Click UI globals); shared chart categorical colors stay one palette.Follow-up in this PR (latest commit):
--color-chart-info— Brand-specific log / chart “info” severity: HyperDX uses Mantine greens (green-6dark,green-7light) via tokens; ClickStack usesvar(--color-chart-blue). Documented in_chart-tokens.scss; values set in each theme_tokens.scssafter the sharedchart-tokensinclude.AppThemeProviderapplies the active.theme-*class on<html>during render (not only inuseEffect) so any same-passgetComputedStylereads see the correct brand.DBTimeChartpasses optionallogLevelColorFnintoformatResponseForTimeChart. On HyperDX, info-like severities resolve fromuseMantineTheme().colors.greenso hex matches Mantine immediately after ClickStack → HyperDX (avoids stale nested--mantine-color-green-*on the document before Mantine updates). ClickStack keeps usinglogLevelColor(CSS).getLogLevelColorOrderaccepts the same resolver so legend sort matches.Why (original PR)
Pre-PR, two coupled problems lived in the chart-color layer:
--color-chart-1was brand green, ClickStack’s was Observable blue. This required a runtimedetectActiveTheme()branch in the JS fallback path, an SSR/hydration mismatch caveat, and made HyperDX’s brand-green double as--color-chart-success(success pills and primary chart series shared a hue).After the refactor:
_chart-tokens.scss, included by both themes.var(--color-chart-cyan)) decoupled from the JS-sideCATEGORICAL_ORDERarray that drives multi-series index assignment.Brand impact
#437eef), matching ClickStack. Brand identity stays on Mantine accent, Click UI globals, sidebar, etc.Commits in this PR
refactor(theme): unify chart palette across themes via shared partial— extracts_chart-tokens.scss; both themes@include chart-tokens.chart-tokensin dark and light blocks.refactor(utils): single chart palette aligned with shared SCSS partial— oneCHART_PALETTE, removesdetectActiveTheme(), simplifies semantic reads.docs+story: align with unified chart palette— docs and Chart Colors story.refactor(theme): name categorical chart colors instead of indexing— named hue vars +CATEGORICAL_ORDERinutils.ts.chore: add changeset for unified+named chart palette— changeset for@hyperdx/app.fix(theme): log chart info colors and reliable theme switching—--color-chart-infoper brand, sync theme class on<html>,logLevelColorFn+ Mantine resolution inDBTimeChart,formatResponseForTimeChart/getLogLevelColorOrderplumbing, tests and story updates.Test plan
yarn ci:unit src/__tests__/ChartUtils.test.tsyarn ci:unit src/components/__tests__/DBTimeChart.test.tsx_chart-tokens.scss,hyperdx/_tokens.scss,clickstack/_tokens.scss--color-chart-info