sadpandajoe commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4414067529
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -193,10 +234,25 @@ export default function ColorPickerControl({
const rgb = color.toRgb();
const hex = rgbaToHex(rgb).toLowerCase();
+ // Match against the same hex the preset chip and trigger display, so a
+ // special color picked from a theme-resolved chip maps back to its key.
+ // Only keys the caller actually offered as presets are candidates;
+ // otherwise an ordinary theme swatch that shares a hex with Green/Red
+ // (e.g. `colorSuccess`) would be misread as the special key.
+ const offeredSpecialKeys = new Set(
+ (customPresets ?? []).flatMap(item =>
+ item.colors.filter(color => color in SPECIAL_COLORS),
+ ),
+ );
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,
Review Comment:
The special-key match still misses a shorthand theme hex. With
`colorSuccess: '#0f0'`, `normalizeColorToHex` returns it unexpanded (the `#`
branch only lowercases) while `hex` here is the six-digit `#00ff00`, so
clicking the Green preset falls through to the `colorSuccess` reverse lookup
(or a literal hex) instead of `Green`. In Table time comparison that turns a
trend rule into ordinary conditional formatting, and BigNumber saves a fixed
color rather than the increase/decrease key. Could the `#` branch expand
3/4-digit shorthand to full hex before comparing, with a test selecting Green
under a shorthand-hex theme?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -103,23 +130,50 @@ 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],
+ // Colors only apply to user-entered ranges; the automatic fallback band
+ // must not pick up a stale `range_colors` entry kept from earlier edits.
+ color: explicitRanges ? rangeColors?.[i] || undefined : 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;
+ // Fixed light/dark foregrounds, not `theme.colorText`: that token flips
+ // to a light color in dark themes and would vanish on a light custom
+ // fill. Pick whichever foreground actually contrasts better against
+ // this fill, rather than a single magic luminance cutoff that doesn't
+ // match where white and the dark foreground really break even.
+ return wcagContrast(luminance, BAND_LABEL_LUMINANCE.light) >=
+ wcagContrast(luminance, BAND_LABEL_LUMINANCE.dark)
+ ? BAND_LABEL_COLORS.light
+ : BAND_LABEL_COLORS.dark;
+ };
+ const bands = sortedRanges.map(({ value, label, color }, i) => ({
name: label,
itemStyle: {
- color: bandFills[Math.min(i, bandFills.length - 1)],
+ // 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)],
},
label: {
show: Boolean(showLabels) && Boolean(label),
position: 'insideTopRight',
distance: theme.sizeUnit,
- color: theme.colorTextSecondary,
+ // Labels sit inside the band, so a custom fill needs a foreground that
+ // contrasts with it; theme-ramp fills keep the default label color.
+ color: getBandLabelColor(color),
Review Comment:
A band with no custom color keeps the default dark label even when it sits
on top of a dark custom band. With the light theme, `ranges="50,100"`,
`range_labels="Low,High"`, Show labels on, and `range_colors=["", "#000000"]`,
the outer band is opaque black while the inner band's translucent default fill
lets it show through, so "Low" renders as dark text on black. Should the label
color account for the nested bands underneath (the nearest custom outer fill)
rather than only this band's own color?
--
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]
