sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4254358648
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberPeriodOverPeriod/controlPanel.ts:
##########
@@ -100,21 +125,24 @@ const config: ControlPanelConfig = {
],
[
{
- name: 'comparison_color_scheme',
+ name: 'increase_color',
Review Comment:
`increase_color`/`decrease_color` are also the Waterfall and Candlestick
control names, which expect `{r, g, b}` objects. Setting `increase_color:
'#336699'` here and then switching the chart type to Waterfall keeps the
string, and `rgbToHex(increaseColor.r, ...)` is called with `undefined`, so the
chart throws. The guard added for the other direction doesn't cover this one;
should these controls get distinct names, or the Waterfall/Candlestick readers
tolerate strings?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -102,23 +107,40 @@ export default function transformProps(
theme.colorFillSecondary,
theme.colorFill,
];
+ // Custom colors (if any) are authored positionally against the original,
+ // pre-sort `ranges` order via the `range_colors` control -- capture each
+ // range's color here, before the descending sort below reorders them.
const sortedRanges = [...ranges]
- .map((value, i) => ({ value, label: rangeLabels[i] }))
+ .map((value, i) => ({
+ value,
+ label: rangeLabels[i],
+ color: rangeColors?.[i] || undefined,
+ }))
.filter(({ value }) => value > axisMin)
.sort((a, b) => b.value - a.value);
// Each band's label sits inside its own top-right corner: the visible strip
// of a nested band ends at its threshold, so the label lands inside the
// range it names and the rightmost label cannot clip at the chart edge.
- const bands = sortedRanges.map(({ value, label }, i) => ({
+ const getBandLabelColor = (fill?: string): string => {
+ const luminance = fill ? getColorLuminance(fill) : null;
+ if (luminance === null) return theme.colorTextSecondary;
+ return luminance < 0.4 ? theme.colorTextLightSolid : theme.colorText;
Review Comment:
A custom band color can make its own label unreadable: a white fill in the
dark theme takes this `theme.colorText` branch, which is also light, and a
translucent fill such as `#000000fc` returns `null` luminance and keeps the
dark `colorTextSecondary` on a near-black band. With Show labels on, those
labels disappear. Should the label color be derived from the fill itself
(including its alpha) rather than from a theme token?
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -1196,6 +1197,12 @@ def merge_gauge_update_form_data( # noqa: C901
patch.pop(MCP_DASHBOARD_TIME_FILTER_SUBJECT, None)
merged.update(patch)
+ if (
+ "interval_color_indices" in fields_set
+ and "interval_colors" in existing_form_data
+ ):
+ # Explicit palette indices supersede saved literal interval colors.
+ merged.pop("interval_colors", None)
Review Comment:
This only runs in `merge_gauge_update_form_data`, which Gauge reaches on a
dataset rebind. A same-dataset `update_chart` goes through
`overlay_update_form_data` instead, so a saved Gauge with `interval_colors:
['#ff0000', '#00ff00']` that is updated with `interval_color_indices: '2,3'`
keeps the literal colors and the requested indices are ignored by the renderer.
Should the same pop be applied on the overlay path?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/controlPanel.tsx:
##########
@@ -59,6 +62,28 @@ const config: ControlPanelConfig = {
},
},
],
+ [
+ {
+ name: 'range_colors',
Review Comment:
Saving a Bullet chart from Explore now persists `range_colors` (even as
`[]`), but `BulletChartConfig` neither models nor strips it. A later MCP
`update_chart` that only changes, say, the metric merges the saved form data
and `validate_merged_bullet_form_data` rejects `range_colors` as an unknown
field, so the update fails. Should `range_colors` be modeled on the config or
added to the preserved-key set?
##########
superset/mcp_service/chart/preview_utils.py:
##########
@@ -1185,9 +1185,22 @@ def numeric_bound(field_name: str) -> float | None:
}
)
- colors = [
- _GAUGE_COLORS[(int(index) - 1) % len(_GAUGE_COLORS)] for index in
color_indices
- ] or list(_GAUGE_COLORS[: max(1, len(interval_bounds))])
+ explicit_colors = form_data.get("interval_colors")
+ if not isinstance(explicit_colors, list):
+ explicit_colors = []
+ colors = []
+ for position in range(max(1, len(interval_bounds))):
+ # Explicit per-interval colors win; blanks fall back to the legacy
+ # palette index for that band, else the palette cycled by position.
+ custom = explicit_colors[position] if position < len(explicit_colors)
else None
+ if isinstance(custom, str) and custom:
+ colors.append(custom)
+ elif position < len(color_indices):
Review Comment:
The frontend ignores legacy indices once `interval_colors` is non-empty and
uses the positional palette color for a blank entry
(`Gauge/transformProps.ts`). Here a blank entry still falls back to
`color_indices[position]`, so with bounds `50,100`, indices `1,4` and
`interval_colors: ['#ff0000', '']` the preview paints the second band with
palette color 4 while the saved chart renders color 2. Should this fall back
only to the positional palette color when explicit colors are present?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
