[HDX-1360] feat(app): refactor ColorSwatchInput to palette tokens#2265
Draft
alex-fedotyev wants to merge 2 commits into
Draft
[HDX-1360] feat(app): refactor ColorSwatchInput to palette tokens#2265alex-fedotyev wants to merge 2 commits into
alex-fedotyev wants to merge 2 commits into
Conversation
PR-1 of the issue #1360 series. The existing ColorSwatchInput (orphan in OSS since #440) used a free-form ColorInput plus a hand-picked 9-value hex array. Refactor it to a palette-only picker driven by the unified 13-token palette landed in #1627: ten categorical tokens (chart-1..chart-10) and three semantic tokens (chart-success, chart-warning, chart-error). Storing tokens (not hex) lets user color choices reflow across themes and color modes without losing contrast or breaking the WCAG ratios baked into the palette. The component remains internal in this PR. PRs 2b through 6b in the issue #1360 series wire it into ChartSeriesEditor, ChartDisplaySettingsDrawer, and the number-tile thresholds / reference-line editors. PR 7 mirrors the schema fields on the external dashboards API. PR 8 documents the feature. Highlights: - New ChartPaletteToken type + CHART_PALETTE_TOKENS, CATEGORICAL_PALETTE_TOKENS, SEMANTIC_PALETTE_TOKENS arrays, isChartPaletteToken type guard in utils.ts. - New getColorFromCSSToken(token) reads the matching --color-chart-* CSS variable, with SSR-safe fallbacks. - New resolveChartColor(token, fallbackIndex, level) for PR 2b to plug into setLineColors / formatResponseForPieChart. - ColorSwatchInput popover split into categorical / semantic sections; trigger uses --color-outline-focus for focus-visible; swatch buttons use aria-pressed; legacy non-token values are guarded to treat as unset. - Storybook stories matrix: six isolated stories (picker mechanics) and five in-context mocks (InSeriesRow, InLineChart, InStackedBar, InNumberTile, InReferenceLineEditor). The in-context stories are storybook-only mocks; they do not import the real editor components. - 13 RTL tests cover the rejection rules and keyboard nav.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
resolveChartColor and ColorSwatchInputProps had no consumers on this branch and tripped the knip CI check. Drop resolveChartColor entirely (the next PR in the issue #1360 series adds it back alongside its first consumer in ChartUtils.setLineColors), and make ColorSwatchInputProps an internal type alias (consumers infer the prop shape from typeof ColorSwatchInput).
Contributor
E2E Test Results✅ All tests passed • 174 passed • 3 skipped • 1276s
Tests ran across 4 shards in parallel. |
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
1 participant
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.
Summary
First PR in the issue #1360 ("Support configuring colors in line graphs, bar charts, and numbers") series. Refactors the existing
ColorSwatchInputto a palette-only picker driven by the unified 13-token chart palette landed in PR #1627. The component remains internal in this PR; subsequent PRs in the series wire it into the chart editor, the number-tile display drawer, and the external dashboards API.The existing component (originally PR #440, currently orphaned in both OSS and EE) used a free-form Mantine
ColorInputplus a hand-picked 9-value hex array. Storing free-form hex values doesn't reflow with theme switches and risks WCAG contrast drift. The palette approach (PR #1627) was designed to be theme-agnostic and accessible; this PR makes the picker speak that vocabulary.ChartPaletteTokentype (10 categorical + 3 semantic tokens) withCHART_PALETTE_TOKENS,CATEGORICAL_PALETTE_TOKENS,SEMANTIC_PALETTE_TOKENSexports and anisChartPaletteTokentype guard inpackages/app/src/utils.ts.getColorFromCSSToken(token)helper that reads the matching--color-chart-*CSS variable with an SSR-safe palette fallback.ColorSwatchInputrewritten: palette-only popover split into Categorical (5x2 grid) and Semantic (1x3 row) sections, accessible trigger witharia-haspopup="dialog"and per-swatcharia-pressed, keyboard-navigable, focus ring via--color-outline-focus, legacy non-token values guarded and treated as unset. No production imports of the realChartSeriesEditororChartDisplaySettingsDrawer.Notes for reviewers
Design ratification gate. This PR is the design review surface for the picker UX before the chart-editor / drawer PRs open. Once it's running locally (
yarn workspace @hyperdx/app storybook) I'll attach a Loom walkthrough + screenshots in all four theme combos (HyperDX light, HyperDX dark, ClickStack light, ClickStack dark) and tag @elizabetdev. The in-context stories (InSeriesRow,InLineChart,InStackedBar,InNumberTile,InReferenceLineEditor) are the placement-framing mocks; the isolated stories cover swatch sizing, hover, selected affordance, and keyboard nav. Holding the consumer PRs (chart-editor wiring, number-tile drawer, reference-line editor) until design sign-off.Tier 3, not Tier 2. Predicted by
pr-triage-classify.jsat 840 production lines (Tier 2 max: 250; Tier 3 escalation: 1000). The earlier plan estimated Tier 2 under the assumption that.stories.tsxfiles are excluded from the prod-line count; they're not (.test.tsxand__tests__/are). I trimmed the in-context stories to keep the component placement-framing intact without growing into Tier 4. If you'd prefer the in-context stories split into a separate follow-up PR, I'm happy to do that.No changeset. Foundation PR: the component is not wired into any user-visible UI yet (consumers land in follow-up PRs in the same series). The first consumer PR will carry the changeset.
[no-changeset: allow]
[ui-check: allow]
Test plan
npx tsc --noEmitclean inpackages/appnpx eslint --quietclean on all changed filesnpx jest src/components/ColorSwatchInput.test.tsx --no-coverage(13/13 pass)deltaChartUtils.test.tscleared on retry)npx storybook buildsucceeds; storybook-static renders the pickerorigin/mainresolveChartColorhelper that's actually used by the next PR ships in that PR alongside its consumer;ColorSwatchInputPropskept internal since callers infer the prop shape fromtypeof ColorSwatchInput).Resolves part of #1360.