Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42910 +/- ##
==========================================
+ Coverage 82.09% 82.21% +0.12%
==========================================
Files 2995 3000 +5
Lines 184988 185283 +295
Branches 42814 42898 +84
==========================================
+ Hits 151872 152339 +467
+ Misses 30357 30182 -175
- Partials 2759 2762 +3
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}
3 of 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
rusackas
force-pushed
the
feat/color-picker-consolidation
branch
from
September 16, 2026 22:26
9b6b41e to
5ec5ad2
Compare
Contributor
Code Review Agent Run #e29137Actionable 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 |
rusackas
force-pushed
the
feat/color-picker-consolidation
branch
from
September 21, 2026 16:53
5ec5ad2 to
3d4241f
Compare
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>
userEvent.type() parses `{` and `[` as key descriptors, so typing raw
JSON into the textarea throws before the assertion runs. Use the same
clear-then-paste pattern the file's helpers already rely on.
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…eme pickers on invalid JSON - PopKPI: hoist a single `comparisonColorValue` shared by the arrow indicator and the pill background/text so they cannot diverge. - BigNumberPeriodOverPeriod controlPanel: extract a shared `comparisonColorControlConfig` spread into both `increase_color` and `decrease_color`, leaving only label/description per control. - ColorPickerControl: add a `disabled` prop passed through to the antd ColorPicker. - ThemeColorPickers: disable the pickers when the theme JSON is invalid or the editor is read-only, so the UI matches the dropped-edit behaviour, and cover it in tests. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Prevents an order-dependent assertion / act() warning since userEvent.click is async. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two bito findings: - BulletRangeColorsControl's parseRanges duplicated tokenizeToNumericArray in Bullet/utils.ts, the exact split/trim/ Number/isFinite pipeline already flagged once in this PR as needing to stay in sync (colors are matched positionally against it). Now imports the shared function instead of reimplementing it. - PopKPI's down-arrow test asserted the arrow but never the valueDifference text next to it, so a regression dropping the delta would still pass. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rebased onto master; the prior pot-regenerate commit conflicted with master's own translation drift since this branch was last synced. Re-derives the same 20 added / 2 removed msgids for this PR's new color-picker controls against the current master baseline. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…olors Extract the shared row layout and color-array-update logic used by IntervalColorsControl and BulletRangeColorsControl into controls/shared/RangeColorRow.tsx. Guard getComparisonColorTokens' alpha-tint suffix so it's only applied to 6/8-digit hex colors, leaving non-hex theme tokens (e.g. an rgba() value) passed through unchanged instead of producing invalid CSS. Replace a misleading jsdom-coverage comment in PopKPI.test.tsx with a real @emotion/jest toHaveStyleRule assertion on the arrow indicator's resolved color. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…heck
emotion's css prop styles aren't reliably observable through jsdom's
computed styles here; the toHaveStyleRule assertion added in the prior
commit fails in CI ("Property not found: color") even though the
underlying color-resolution logic is correct and already covered by
direct unit tests in utils.test.ts.
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses review feedback on the color-picker consolidation: - Migrate comparison_color_scheme into increase_color/decrease_color, and interval_color_indices into interval_colors, in handleDeprecatedControls so the legacy value survives the first save instead of being dropped by getFormDataFromControls (which only serializes registered controls). - Resolve a non-hex (rgb/rgba) comparison color into a real low-alpha background tint instead of reusing the same value for text and background, which made the comparison symbol invisible. - Block BulletRangeColorsControl color edits while the ranges input has a blank token between two numbers (e.g. "20,,60"), which previously let an edit get positionally misaligned once the blank was filled back in. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…on edit
The Gauge interval-colors control's original description ("Comma-separated
color picks for the intervals...") was superseded by the current
IntervalColorsControl wording without re-running babel_update.sh, leaving
the old string in messages.pot with no corresponding source reference.
Verified against a fresh pybabel extraction: removing it leaves zero
missing/stale entries.
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… and bullet range control `resolveLegacyIntervalColors` fell back to the registry's default scheme when an explicit color_scheme name was unregistered, instead of leaving colors unresolved; and used the bound's absolute position instead of a counter for bounds missing an explicit legacy index, misassigning colors whenever only some bounds had one. Also fixed a BulletRangeColorsControl test asserting `toBeDisabled` on AntD's inner text <span> instead of the actual <button> (jest-dom's matcher only recognizes form elements). Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ignment IntervalColorsControl parsed `intervals` with the same lenient tokenizer BulletRangeColorsControl uses, but never guarded against a transient mid-edit value (e.g. "20,,60") the way Bullet's control does -- a color edit made while the list was incomplete could permanently misalign once the blank was filled back in. Reuse the existing `isRangesInputComplete` guard and disable the picker until the bounds list is complete. Also adds the test coverage bito-code-review flagged as missing for `tokenizeToNumericArray`/`isRangesInputComplete`'s non-numeric, non-finite, and blank-token-before-the-tolerated-trailing-one paths. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…, i18n label, and theme cast Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #c5f638
Actionable Suggestions - 1
-
superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx - 1
- Inconsistent i18n label interpolation · Line 84-84
Additional Suggestions - 9
-
superset-frontend/src/explore/store.test.tsx - 1
-
any-typed test formData · Line 398-465The seven new tests declare `const formData: any` (lines 398, 409, 420, 432, 440, 452, 465), which violates the repo's "NO `any` types" standard in AGENTS.md and .cursor/rules/dev-standard.mdc; the pre-existing `eslint-disable-next-line @typescript-eslint/no-explicit-any` at line 31 shows this rule is enforced here. A local type alias for the legacy form-data shape (or exporting `FormData` from `src/explore/store`) keeps these tests typed.
-
-
superset-frontend/src/features/themes/ThemeModal.tsx - 2
-
Inconsistent readOnly guard · Line 637-644The new `Colors` block is fully unmounted when `isReadOnly` is true, so the `disabled` prop `ThemeColorPickers` already supports is never used here. The sibling JSON textarea at line 684 instead stays mounted with `readOnly={isReadOnly}`. Prefer passing `disabled={isReadOnly}` and dropping the conditional wrapper so the guard lives in one place, matching the sibling control's pattern.
-
Inline prop defeats memo · Line 640-640`currentTheme?.json_data || ''` is re-evaluated inline on every render, so when `json_data` is empty a fresh `''` is produced each time. `ThemeColorPickers` feeds this into `useMemo(() => tryParseThemeJson(jsonData), [jsonData])` (ThemeColorPickers.tsx line 133); string literals compare by value so behavior is correct, but a memoized prop would keep the parse memo stable. Minor; align with the memoized `onJsonDataChange` callback.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx - 3
-
Eager t() in label · Line 70-70The repo's `i18n-strings/no-eager-t-in-config` ESLint rule (enabled for `**/controlPanel.*` in `eslint.config.minimal.js`) and the `BaseControlConfig` docs in `superset-ui-chart-controls/src/types.ts` prescribe the lazy form: eager `t()` evaluates at module load, before i18n initializes, so the label can stay in the fallback language. Use `label: () => t('Range colors')`.
-
Eager t() in description · Line 73-75Same eager-`t()` issue as the `label` on line 70: `description: t(...)` is evaluated at module load, before i18n initializes, per the `no-eager-t-in-config` rule enabled for `**/controlPanel.*` and the `BaseControlConfig` docs in `superset-ui-chart-controls/src/types.ts`. Wrap it as `description: () => t(...)` so the description re-resolves at render time.
-
Duplicated visibility predicate · Line 76-76This `visibility` predicate is byte-identical to the one on the sibling `range_labels` control (line 61), giving the file three copies of the same condition. Extracting a shared predicate beside `config` keeps `range_labels` and `range_colors` in sync if the `ranges` visibility rule ever changes.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts - 2
-
Duplicated tint opacity constant · Line 181-181The ~10% pill tint is encoded twice: `COMPARISON_TINT_ALPHA_HEX` ('1A', line 123) for hex colors and the literal `0.1` here for rgb()/rgba() colors. If the tint is ever tuned, the two paths diverge and pills built from hex vs. rgb theme tokens render different opacities. Consider deriving both from one constant, e.g. `COMPARISON_TINT_ALPHA = 0x1a / 0xff`.
-
Fallback yields invisible pill text · Line 187-191If `resolvedColor` is an unrecognized format (named CSS color, 3/4-digit hex), all three tokens become the same value, so the pill renders its text in the same color as its background — invisible text. No current producer emits such values (picker emits 6/8-digit hex; tokens resolve to hex/rgb), so this is future-proofing: consider a theme-neutral background fallback.
-
-
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx - 1
-
Exported mutable shared constant · Line 30-30Exporting `SPECIAL_COLORS` makes it a shared mutable module binding: `as const` enforces readonly only at type level, so an importer could still mutate it at runtime (e.g. via `Object.assign`), corrupting color resolution in `toDisplayHex` and `handleChange`. Wrapping in `Object.freeze` keeps literal types while making the shared map immutable. No importer exists yet, so this is forward-looking.
-
Review Details
-
Files reviewed - 35 · Commit Range:
0b9b0fe..8f9a239- superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.test.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/PopKPI.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/utils.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Bullet/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Bullet/utils.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Gauge/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/index.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Bullet/utils.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Gauge/transformProps.test.ts
- superset-frontend/src/explore/components/controls/BulletRangeColorsControl/BulletRangeColorsControl.test.tsx
- superset-frontend/src/explore/components/controls/BulletRangeColorsControl/index.tsx
- superset-frontend/src/explore/components/controls/BulletRangeColorsControl/types.ts
- superset-frontend/src/explore/components/controls/ColorPickerControl.tsx
- superset-frontend/src/explore/components/controls/IntervalColorsControl/IntervalColorsControl.test.tsx
- superset-frontend/src/explore/components/controls/IntervalColorsControl/index.tsx
- superset-frontend/src/explore/components/controls/IntervalColorsControl/legacyColors.ts
- superset-frontend/src/explore/components/controls/IntervalColorsControl/types.ts
- superset-frontend/src/explore/components/controls/index.ts
- superset-frontend/src/explore/components/controls/shared/RangeColorRow.tsx
- superset-frontend/src/explore/store.test.tsx
- superset-frontend/src/explore/store.ts
- superset-frontend/src/features/themes/ThemeColorPickers.test.tsx
- superset-frontend/src/features/themes/ThemeColorPickers.tsx
- superset-frontend/src/features/themes/ThemeModal.test.tsx
- superset-frontend/src/features/themes/ThemeModal.tsx
- superset/translations/messages.pot
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers an incremental AI Review. -
/review full- Manually triggers a full AI Review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
sadpandajoe
reviewed
Oct 2, 2026
| (!formData.interval_colors || formData.interval_colors.length === 0) | ||
| ) { | ||
| const bounds = parseIntervalBounds(formData.intervals); | ||
| const resolved = resolveLegacyIntervalColors( |
Member
There was a problem hiding this comment.
Dashboard hydration runs this migration before applying the dashboard’s color_scheme override, so a legacy Gauge saved under palette A now keeps A’s literal interval colors on a dashboard using palette B. Could dashboard rendering retain the palette-relative indices rather than freezing them before the effective palette is known?
…color migration Keep positional scheme colors for bounds without a legacy index, resolve against the default scheme when the saved scheme is unregistered, and use a placeholder for the interval label so translators control word order. Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
sadpandajoe
reviewed
Oct 3, 2026
| name: 'interval_colors', | ||
| config: { | ||
| type: 'TextControl', | ||
| type: 'IntervalColorsControl', |
Member
There was a problem hiding this comment.
The old text input was keyboard-editable, but these rows now use a click-only color-picker trigger with no tab stop or Enter/Space handler, so keyboard-only users cannot change interval colors. Could the picker expose a focusable, keyboard-activatable trigger?
Contributor
Code Review Agent Run #8f4edfActionable Suggestions - 0Additional Suggestions - 1
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 |
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
Code Review Agent Run #5dc91fActionable Suggestions - 0Review 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 |
… cleared Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
sadpandajoe
reviewed
Oct 5, 2026
| * control panel row. Takes precedence over `intervalColorIndices` when | ||
| * present. | ||
| */ | ||
| intervalColors?: string[]; |
Member
There was a problem hiding this comment.
Rebinding a Gauge's dataset through MCP update_chart drops these custom colors: _GAUGE_PRESENTATION_FORM_DATA_KEYS preserves interval_color_indices but not interval_colors, and the replacement params are saved without them. Could the new field be preserved during rebinding, with a regression asserting the picked colors survive?
| // whenever it's populated. | ||
| if (Array.isArray(intervalColors) && intervalColors.length > 0) { | ||
| return intervalBounds.map((val, idx) => [ | ||
| val, |
Member
There was a problem hiding this comment.
A saved Gauge now uses interval_colors here, but its MCP Vega-Lite preview still reads only interval_color_indices in _prepare_gauge_preview, so custom red/green bands appear in the default palette instead. Could the preview honor the same explicit-color precedence?
|
|
||
| const colorAt = (index: number): string => | ||
| value?.[index] || | ||
| legacyColors[index] || |
Member
There was a problem hiding this comment.
With interval_colors: ['#ff0000'] and legacy indices 3,1, the renderer uses the second palette color for the missing second entry, but this picker displays the first; editing only the first row then persists that different color into the untouched second band. Could populated new arrays use the renderer's positional fallback instead of resurrecting legacy indices?
| // A custom `range_colors` entry wins; otherwise fall back to the | ||
| // default theme-token ramp exactly as before this control existed, so | ||
| // Bullet charts saved without `range_colors` are unaffected. | ||
| color: color || bandFills[Math.min(i, bandFills.length - 1)], |
Member
There was a problem hiding this comment.
Choosing a black range fill with Show labels enabled makes its label unreadable in the light theme: the label sits inside the band and still uses the dark colorTextSecondary foreground. Could custom fills get a contrasting label color or place the label outside the fill?
Contributor
Code Review Agent Run #ed721dActionable Suggestions - 0Review 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 |
This branch has not been deployed
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