Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42910 +/- ##
===========================================
+ Coverage 65.62% 84.64% +19.02%
===========================================
Files 3059 3065 +6
Lines 202434 205256 +2822
Branches 47448 48125 +677
===========================================
+ Hits 132852 173746 +40894
+ Misses 66587 28207 -38380
- Partials 2995 3303 +308
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:
|
|
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). |
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. |
96ba402 to
43e60b5
Compare
|
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. |
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 |
9b6b41e to
5ec5ad2
Compare
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 |
5ec5ad2 to
3d4241f
Compare
… are unresolved Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…colors and unresolvable legacy indices Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… value array and unresolved legacy indices Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
There was a problem hiding this comment.
Three new issues with unset and legacy color states, inline.
| // resolve the correct color from that legacy scheme at render time. A | ||
| // hardcoded default here would win over the legacy fallback via | ||
| // `applyDefaultFormData` and silently repaint old dashboards. | ||
| const comparisonColorControlConfig: CustomControlItem['config'] = { |
There was a problem hiding this comment.
With no increase_color/decrease_color saved (any new chart), both pickers read 'Transparent' while PopKPI paints the theme success/error colors, so the editor never shows what the chart draws.
| legacyIndexCount > 0 && | ||
| legacyColors.length > 0 && | ||
| legacyColors.every(color => color === ''); | ||
| const editable = boundsComplete && !legacyUnresolved; |
There was a problem hiding this comment.
When an in-use legacy index is past the static palette, every row is disabled with no explanation and the legacy field is hidden, so these bands can't be recolored except by switching scheme, while the help text still says colors follow the scheme until one is edited.
…onsolidation Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ounds; pin non-positional legacy Gauge indices Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
There was a problem hiding this comment.
One formatting fix inline.
Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… opt-in 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>
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>
aminghadersohi
left a comment
There was a problem hiding this comment.
Repeating saved Bullet thresholds still drops range_colors when Explore saved them in a different spelling.
…abel contrast, ranges comparison Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…er shorthand hex theme Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
There was a problem hiding this comment.
One regression in the latest shorthand-hex change; details inline.
…t over translucent bands Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
There was a problem hiding this comment.
Both latest fixes behave as described, but neither has a test that fails when it is removed; suggested tests inline, plus one edge in the Bullet label-fill change.
| let outerCustomFill: string | undefined; | ||
| const bandLabelFills = sortedRanges.map(({ color }) => { | ||
| if (color && getColorLuminance(color) !== null) outerCustomFill = color; | ||
| return outerCustomFill; | ||
| }); |
There was a problem hiding this comment.
Nit: My earlier suggestion missed this: getColorLuminance is also null for opaque colors it can't parse, so an API-saved range_colors entry like yellow inside a black band now gets white text (~1.07:1). Defer outward only for fills known to be translucent.
| let outerCustomFill: string | undefined; | |
| const bandLabelFills = sortedRanges.map(({ color }) => { | |
| if (color && getColorLuminance(color) !== null) outerCustomFill = color; | |
| return outerCustomFill; | |
| }); | |
| let outerCustomFill: string | undefined; | |
| const bandLabelFills = sortedRanges.map(({ color }) => { | |
| const translucent = | |
| !!color && | |
| getColorLuminance(color) === null && | |
| /^(#[0-9a-f]{8}|rgba\()/i.test(color.trim()); | |
| if (color && !translucent) outerCustomFill = color; | |
| return outerCustomFill; | |
| }); |
| '#ffffff', | ||
| '#ffffff', | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
Nit: Reverting the label-fill change to color ?? outerCustomFill keeps every Bullet test green: the only translucent case has no opaque band outside it. This test pins a translucent band over an opaque one.
| }); | |
| }); | |
| test('labels a translucent band from the opaque custom fill beneath it', () => { | |
| const { echartOptions } = transformProps( | |
| chartProps({ showLabels: true, rangeColors: ['#ffffff33', '#000000', ''] }), | |
| ); | |
| const labelColors = (echartOptions as any).series[0].markArea.data.map( | |
| (d: any) => d[0].label.color, | |
| ); | |
| // drawn largest-first: unset, black, translucent white over black | |
| expect(labelColors).toEqual([ | |
| supersetTheme.colorTextSecondary, | |
| '#ffffff', | |
| '#ffffff', | |
| ]); | |
| }); |
| // fallback. | ||
| expect(toDisplayHex('Green', {}, true)).toBe('#00960033'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Nit: Removing either normalizeOpaqueAlpha call leaves this suite green, so the opaque four-digit colorError → Red fix is unpinned. This test fails with the shorthand-path call removed.
| }); | |
| }); | |
| test('stores "Red" when the theme colorError is an opaque four-digit hex', async () => { | |
| const onChange = jest.fn(); | |
| jest | |
| .spyOn(require('@apache-superset/core/theme'), 'useTheme') | |
| .mockReturnValue({ colors: { colorError: '#F00F' } }); | |
| render( | |
| <ColorPickerControl | |
| {...defaultProps} | |
| onChange={onChange} | |
| resolveThemeTokens | |
| presets={[{ label: 'Special Colors', colors: ['Green', 'Red'] }]} | |
| />, | |
| ); | |
| await userEvent.click( | |
| document.querySelector('.ant-color-picker-trigger') as HTMLElement, | |
| ); | |
| await waitFor(() => { | |
| expect( | |
| document.querySelector('.ant-color-picker-presets-color'), | |
| ).toBeInTheDocument(); | |
| }); | |
| await userEvent.click( | |
| document.querySelectorAll('.ant-color-picker-presets-color')[1], | |
| ); | |
| expect(onChange).toHaveBeenCalledWith('Red'); | |
| }); |
…se theme tokens Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
There was a problem hiding this comment.
Not ready: in conditional formatting with time comparison, a plain Success/Error swatch pick now saves a Trend sentinel.
| const specialEntry = resolveThemeTokens | ||
| ? Object.entries(SPECIAL_COLORS).find( | ||
| ([, rgba]) => rgbaToHex(rgba).toLowerCase() === hex, | ||
| ) | ||
| ? Object.keys(SPECIAL_COLORS) | ||
| .filter(key => offeredSpecialKeys.has(key)) | ||
| .map(key => [key, toDisplayHex(key, themeColors, true)] as const) | ||
| .find( | ||
| ([, displayHex]) => | ||
| displayHex !== undefined && | ||
| normalizeColorToHex(displayHex) === hex, | ||
| ) |
There was a problem hiding this comment.
With time comparison on, Table/AG Grid offer both the colorSuccess/colorError swatches and the Trend Green/Red chips; they now share one hex and this match runs first, so picking the plain Success swatch saves Green and hides the operator fields (base saves colorSuccess).
| }); | ||
|
|
||
| test('stores the theme token, not "Green", when Green is not among the presets', async () => { | ||
| const onChange = jest.fn(); |
There was a problem hiding this comment.
This test sets no theme, so in a full-file run it inherits the colorSuccess: '#0f0' spy from the test above (clearAllMocks keeps mockReturnValue); with shorthand no longer reverse-mapped it gets an RGB object instead of colorSuccess.
| const onChange = jest.fn(); | |
| const onChange = jest.fn(); | |
| jest | |
| .spyOn(require('@apache-superset/core/theme'), 'useTheme') | |
| .mockReturnValue({ colors: { colorSuccess: '#00ff00' } }); |
| // Parse with the exact same semantics `getIntervalBoundsAndColors` in | ||
| // Gauge/transformProps.ts uses for this same legacy fallback (via | ||
| // `parseNumbersList`, not reimported here to avoid pulling the whole | ||
| // `@superset-ui/plugin-chart-echarts` package -- and every chart plugin it | ||
| // eagerly registers -- into `explore/store.ts`'s import graph): one | ||
| // unparseable (or blank) token discards every legacy index for the whole | ||
| // list, rather than resolving the other tokens independently. A more | ||
| // lenient parser here would let this one-time migration (and this | ||
| // display-only preview) disagree with how an untouched legacy chart has | ||
| // always rendered -- see #42910 review thread on this file. |
There was a problem hiding this comment.
Nit: explore/store.ts no longer imports this module or runs a migration, and its only importer already pulls in @superset-ui/plugin-chart-echarts, so this rationale no longer holds.
| // Parse with the exact same semantics `getIntervalBoundsAndColors` in | |
| // Gauge/transformProps.ts uses for this same legacy fallback (via | |
| // `parseNumbersList`, not reimported here to avoid pulling the whole | |
| // `@superset-ui/plugin-chart-echarts` package -- and every chart plugin it | |
| // eagerly registers -- into `explore/store.ts`'s import graph): one | |
| // unparseable (or blank) token discards every legacy index for the whole | |
| // list, rather than resolving the other tokens independently. A more | |
| // lenient parser here would let this one-time migration (and this | |
| // display-only preview) disagree with how an untouched legacy chart has | |
| // always rendered -- see #42910 review thread on this file. | |
| // Parse with the exact same semantics `getIntervalBoundsAndColors` in | |
| // Gauge/transformProps.ts uses for this same legacy fallback: one | |
| // unparseable (or blank) token discards every legacy index for the whole | |
| // list, rather than resolving the other tokens independently. A more | |
| // lenient parser here would let this display-only preview disagree with | |
| // how an untouched legacy chart has always rendered. |
…ure black/white Co-Authored-By: Evan Rusackas <evan@preset.io> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
aminghadersohi
left a comment
There was a problem hiding this comment.
The latest commit maps shorthand custom-theme tokens back to token names, which Table formatting and the PoP pill cannot render.
| const hex = normalizeColorToHex(value); | ||
| // Shorthand pure black/white (e.g. `colorTextBase: '#000'`) would make a | ||
| // plain black or white pick match a base token and save the token name | ||
| // instead of a usable color value. |
There was a problem hiding this comment.
Expanding shorthand again maps any custom-theme token like colorSuccess: '#0f0' back by name, so that swatch saves colorSuccess (base saves #00ff00); Table formatting then emits invalid #0f087 and the PoP pill text matches its background.
| if ( | ||
| /^#[0-9a-f]{3,4}$/i.test(value) && | ||
| (hex === '#000000' || hex === '#ffffff') | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nit: The '#000000'/'#ffffff' string literals trip the custom-rules literal-color check; a regex on value matches exactly the same shorthand inputs without them.
| if ( | |
| /^#[0-9a-f]{3,4}$/i.test(value) && | |
| (hex === '#000000' || hex === '#ffffff') | |
| ) { | |
| return; | |
| } | |
| if (/^#(?:000|fff)f?$/i.test(value)) { | |
| return; | |
| } |
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