Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42910 +/- ##
==========================================
+ Coverage 79.22% 79.29% +0.06%
==========================================
Files 2888 2890 +2
Lines 166415 166373 -42
Branches 38514 38519 +5
==========================================
+ Hits 131842 131919 +77
+ Misses 32080 31971 -109
+ Partials 2493 2483 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Nice consolidation, and the one-commit-per-item structure keeps it bisectable with real backward-compat handling on each swap (legacy interval_color_indices / comparison_color_scheme fallbacks, additive optional fields). One process question: this bundles four unrelated areas (Gauge, Bullet, Big Number PoP, Theme editor), each with its own new control dir and backward-compat surface — might be worth confirming maintainers are fine reviewing it as a single PR vs splitting per area. Minor: ThemeColorPickers re-parses the JSON textarea on each render (the useMemo keyed on jsonData mitigates it). |
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates legacy chart and theme color selection onto the shared ColorPickerControl.
Changes:
- Adds per-interval Gauge and per-range Bullet color controls.
- Adds independent Big Number comparison colors with legacy fallback logic.
- Adds curated theme-token pickers and related tests.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/features/themes/ThemeModal.tsx |
Integrates curated theme color pickers. |
src/features/themes/ThemeModal.test.tsx |
Tests modal/picker synchronization. |
src/features/themes/ThemeColorPickers.tsx |
Implements theme token color editing. |
src/features/themes/ThemeColorPickers.test.tsx |
Tests parsing, patching, and interactions. |
src/explore/components/controls/IntervalColorsControl/types.ts |
Defines Gauge color-control props. |
src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx |
Tests Gauge interval color UI. |
src/explore/components/controls/IntervalColorsControl/index.tsx |
Implements Gauge interval color UI. |
src/explore/components/controls/index.ts |
Registers the new controls. |
src/explore/components/controls/ColorPickerControl.tsx |
Exports semantic color definitions. |
src/explore/components/controls/BulletRangeColorsControl/types.ts |
Defines Bullet color-control props. |
src/explore/components/controls/BulletRangeColorsControl/index.tsx |
Implements Bullet range color UI. |
src/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsx |
Tests Bullet range color UI. |
plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts |
Tests Gauge color rendering and fallback. |
plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts |
Tests Bullet custom band colors. |
plugins/plugin-chart-echarts/src/Gauge/types.ts |
Adds Gauge interval color form data. |
plugins/plugin-chart-echarts/src/Gauge/transformProps.ts |
Applies Gauge colors with legacy fallback. |
plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx |
Replaces the legacy Gauge color field. |
plugins/plugin-chart-echarts/src/Bullet/types.ts |
Adds Bullet range color form data. |
plugins/plugin-chart-echarts/src/Bullet/transformProps.ts |
Applies colors to sorted Bullet bands. |
plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx |
Adds the Bullet range color control. |
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts |
Adds comparison color resolution helpers. |
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.test.ts |
Tests comparison color resolution. |
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts |
Adds new comparison color properties. |
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts |
Passes comparison colors to the component. |
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx |
Applies customizable comparison colors. |
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.test.tsx |
Tests comparison rendering variants. |
plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts |
Adds increase/decrease color pickers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rusackas
force-pushed
the
feat/color-picker-consolidation
branch
from
August 30, 2026 00:46
96ba402 to
43e60b5
Compare
rusackas
marked this pull request as ready for review
August 30, 2026 19:51
dosubot
Bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
9 tasks
Member
Author
|
Heya @sha174n, thanks for the read-through! Given each swap already lands as its own bisectable commit with its own backward-compat handling, I'm inclined to keep this bundled rather than split it four ways at this point. Happy to reconsider if another maintainer feels strongly about it. And yeah, the useMemo already covers the re-parse cost, as you noted. |
Contributor
Code Review Agent Run #c05116Actionable Suggestions - 0Additional Suggestions - 7
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
github-actions
Bot
added
the
requires:rebase
The Gauge chart's "Interval colors" control asked users to enter comma-separated 1-indexed positions into the chosen color scheme (e.g. "1,2,4"), forcing them to count swatches by hand with no visual feedback and silently discarding malformed input. Replace it with IntervalColorsControl, a new control that renders one ColorPickerControl per interval bound (parsed from the existing `intervals` control) and stores real hex colors in a new `interval_colors` form-data field, positionally matched to those bounds. Backward compatibility: charts saved before this control existed only have `interval_color_indices` (the old scheme-index strings). `Gauge/transformProps.ts#getIntervalBoundsAndColors` still resolves those indices against the chart's categorical scheme at render time whenever `interval_colors` is absent or empty, so existing dashboards render identically without a data migration. The new control also resolves legacy indices to real colors for display the first time such a chart's control panel is opened, purely as an editor convenience. Design note: bounds stay owned by the existing `intervals` text control rather than being folded into the new control's row list, so there's a single source of truth for bounds and no risk of the two controls drifting out of sync.
The Bullet chart's background range bands were hardcoded to a 4-step
theme-token ramp (colorFillQuaternary -> colorFill) with no way to
customize them, unlike the rest of the chart (ranges, markers, marker
lines) which are all user-configurable.
Add an optional `range_colors` control (BulletRangeColorsControl) that
renders one ColorPickerControl per threshold parsed from the existing
`ranges` control. Each row starts unset ("use default") with a "Use
default" link to clear a customization once made; unset rows keep
using the theme-token ramp exactly as before.
`Bullet/transformProps.ts` captures each range's chosen color
(matched by its original, pre-sort position in `ranges`) before the
existing largest-first band sort reorders them for nested drawing, so
colors stay pinned to the correct threshold regardless of draw order.
Backward compatible by construction: `range_colors` is optional and
defaults to empty, so Bullet charts saved before this control existed
have no such field and render with the exact same default ramp.
The Period-over-Period Big Number's "color scheme for comparison"
control only offered two fixed choices ("Green for increase, red for
decrease" and its reverse), bound directly to theme.colorSuccess /
theme.colorError with no room for a brand-specific color.
Replace it with two ColorPickerControls, `increase_color` and
`decrease_color` (defaulting to the 'Green' / 'Red' semantic tokens,
matching the historical default), using the same resolveThemeTokens +
outputFormat="hex" pattern #42053 introduced for
FormattingPopoverContent: picking the "Green"/"Red" preset swatch
stores the token name (so the UI still reads the same as before for
users who just want the classic behavior), while any other pick stores
a literal hex color.
`increaseColor`/`decreaseColor` and the color->style resolution move
to two small, independently unit-tested pure functions in utils.ts
(`resolveComparisonColorKeys`, `getComparisonColorTokens`) rather than
living inline in PopKPI's render body, since jsdom doesn't reliably
expose emotion's injected styles to `toHaveStyle` for direct
component-level assertions.
Backward compatibility: `resolveComparisonColorKeys` falls back to the
legacy `comparisonColorScheme` field (still read, marked @deprecated in
types.ts) whenever the new fields are absent, including correctly
reversing increase/decrease for charts saved with the old "Red for
increase, green for decrease" choice -- the case a naive
default-to-Green migration would have silently broken.
Also exports `SPECIAL_COLORS/SpecialColorKey` from ColorPickerControl
so other call sites (like this one) don't need to redefine the
Green/Red semantic color mapping.
The admin Theme editor (ThemeModal) only exposed antd theming as a single JSON textarea, requiring admins to paste in a whole token object generated by an external tool just to change, say, the brand color. Add a "Colors" section (ThemeColorPickers) above the JSON textarea with one ColorPickerControl per curated antd token: the 5 SEED colors (colorPrimary, colorSuccess, colorWarning, colorError, colorInfo) plus 6 load-bearing map/alias tokens (colorLink, colorText, colorTextSecondary, colorBgBase, colorBgContainer, colorBorder). This intentionally does not attempt to cover the full 100+ token surface -- anything else stays fully editable via the JSON textarea, which remains the source of truth. Token names are taken directly from antd's own SeedToken/MapToken types, not invented. Sync is two-way and implemented as two small, independently tested pure functions (`tryParseThemeJson`, `patchThemeJsonToken`): - Picker -> JSON: patches just that one key into the JSON's `token` object and re-serializes with the same 2-space indent used elsewhere in this modal, preserving every other key (curated or not) and their values. - JSON -> pickers: each render re-parses the JSON textarea's current value and re-derives picker values from it, so typing in the textarea updates the matching swatches live. - Invalid/mid-edit JSON: `tryParseThemeJson` returns null instead of throwing (matching the file's existing `isValidJson` convention); the section shows a small notice and pickers stop persisting edits until the JSON is valid again, rather than crashing or silently clobbering the textarea. The section is hidden for read-only system themes, matching the existing Format/Apply button visibility, and both directions of sync, invalid-JSON handling, and "uncurated token survives a picker edit" are covered in ThemeColorPickers.test.tsx (unit) and ThemeModal.test.tsx (integration).
- Drop the static default on increase_color/decrease_color so the legacy comparison_color_scheme fallback in resolveComparisonColorKeys still applies to old dashboards instead of getting preempted by applyDefaultFormData. - Read the legacy interval_color_indices field from form_data instead of state.controls in Gauge's IntervalColorsControl mapStateToProps, since it's no longer a registered control. - Resolve theme token names and strip pre-existing hex alpha before tinting in getComparisonColorTokens, so it stays valid CSS for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ange colors tokenizeToNumericArray only dropped NaN, so a range value overflowing to Infinity survived while BulletRangeColorsControl's own parser (which sets up one color row per range) already dropped it -- shifting later range_colors entries onto the wrong band. Filter on Number.isFinite in both places so the two stay index-aligned. Also reject a non-object `token` in ThemeColorPickers' tryParseThemeJson; patchThemeJsonToken spreads it, so a malformed `token` (a string or array) was silently rewritten into numeric-keyed junk on the next picker edit instead of surfacing the existing invalid-JSON notice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rusackas
force-pushed
the
feat/color-picker-consolidation
branch
from
September 16, 2026 22:26
9b6b41e to
5ec5ad2
Compare
Contributor
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
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.
SUMMARY
Follow-up to #42053, which upgraded
ColorPickerControl(custom presets,resolveThemeTokens,outputFormat) and used it to fixConditionalFormattingControl. This PR finds and fixes the four remaining places in the app that still used a legacy/degraded color-selection UI instead of the shared picker, one commit per item.1. Gauge chart interval colors
Gauge/controlPanel.tsx's "Interval colors" control asked users to type comma-separated 1-indexed positions into the chosen color scheme (e.g.1,2,4), with zero visual feedback and silent discarding of malformed input.Replaced with
IntervalColorsControl: oneColorPickerControlper interval bound (parsed from the existingintervalscontrol), storing real hex colors in a newinterval_colorsfield, positionally matched to those bounds.Design decision: bounds stay owned by the existing
intervalstext control rather than being folded into the new control's own row list (which the row-list add/remove pattern in the task brief technically implied). This keeps a single source of truth for bounds and avoids needing two-way sync between two independent controls — the new control's row count simply tracks whateverintervalscurrently contains.Backward compatibility: charts saved before this control existed only have
interval_color_indices(the old index strings).getIntervalBoundsAndColorsintransformProps.tsstill resolves those indices against the categorical scheme at render time wheneverinterval_colorsis empty, so existing dashboards render identically with no migration. The control also resolves legacy indices to real colors for display the first time such a chart's panel is reopened (editor convenience only, not required for correct rendering).2. Bullet chart band colors
Bullet chart background bands were hardcoded to a 4-step theme-token ramp with no color control at all — a genuinely new feature, not a swap.
Added an optional
range_colorscontrol (BulletRangeColorsControl): oneColorPickerControlper threshold parsed from the existingrangescontrol, each starting unset ("use default") with a "Use default" link to clear a customization.transformProps.tscaptures each range's chosen color by its original (pre-sort) position inrangesbefore the existing largest-first band sort reorders them for nested drawing, so colors stay pinned to the correct threshold regardless of draw order.Backward compatible by construction:
range_colorsis optional and defaults to empty, so charts saved before this control existed have no such field and keep rendering with the exact default ramp.3. Big Number Period-over-Period comparison colors
The comparison-color control was a 2-choice
SelectControl("Green for increase, red for decrease" / reverse) bound directly totheme.colorSuccess/theme.colorError.Replaced with two
ColorPickerControls,increase_color/decrease_color, using the exactresolveThemeTokens+outputFormat="hex"pattern #42053 introduced: picking the Green/Red preset swatch stores the token name (so it still reads the same as before for users who just want the classic behavior), while any other pick stores a literal hex color. ExportedSPECIAL_COLORS/SpecialColorKeyfromColorPickerControl.tsxso this call site doesn't redefine the Green/Red mapping.The color→style resolution moved into two small, independently unit-tested pure functions in
utils.ts(resolveComparisonColorKeys,getComparisonColorTokens) rather than living inline inPopKPI's render body — jsdom doesn't reliably expose emotion's injected styles totoHaveStylefor direct component assertions, so the logic needed to be testable on its own.Backward compatibility:
resolveComparisonColorKeysfalls back to the legacycomparisonColorSchemefield (kept,@deprecatedintypes.ts) whenever the new fields are absent — including correctly reversing increase/decrease for charts saved with the old "Red for increase, green for decrease" choice, the case a naive default-to-Green migration would have silently broken.4. Admin Theme editor curated colors
ThemeModal.tsxonly exposed antd theming as a single JSON textarea, requiring admins to paste in a whole token object from an external tool to change even one color.Added a "Colors" section (
ThemeColorPickers) above the JSON textarea with oneColorPickerControlper curated antd token — the 5 SEED colors (colorPrimary,colorSuccess,colorWarning,colorError,colorInfo) plus 6 load-bearing map/alias tokens (colorLink,colorText,colorTextSecondary,colorBgBase,colorBgContainer,colorBorder). This intentionally does not attempt the full 100+ token surface — everything else stays fully editable via the JSON textarea, which remains the source of truth. Names are taken directly from antd's ownSeedToken/MapTokentypes, not invented.Sync is two-way, via two small pure functions (
tryParseThemeJson,patchThemeJsonToken):tokenobject and re-serializes with the modal's existing 2-space indent, preserving every other key (curated or not).tryParseThemeJsonreturnsnullinstead of throwing (matching the file's existingisValidJsonconvention); the section shows a small notice and pickers stop persisting edits until the JSON is valid again.The section is hidden for read-only system themes, matching the existing Format/Apply button visibility.
No backward-compat concern — additive UI over the same JSON, nothing about existing saved themes changes.
TESTING INSTRUCTIONS
npm run testinsuperset-frontend/— new/updated suites:plugins/plugin-chart-echarts/test/Gauge/transformProps.test.tssrc/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsxplugins/plugin-chart-echarts/test/Bullet/transformProps.test.tssrc/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsxplugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/{utils,PopKPI}.test.tsxsrc/features/themes/{ThemeModal,ThemeColorPickers}.test.tsxinterval_color_indicesonly, an existing Bullet chart with norange_colors, and a Big Number PoP chart saved with onlycomparison_color_scheme(including theRedvalue) — all three should render identically to before this PR.ADDITIONAL INFORMATION