rusackas commented on code in PR #42910:
URL: https://github.com/apache/superset/pull/42910#discussion_r4429338476
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -49,13 +63,20 @@ export interface ColorPickerControlProps {
ariaLabel?: string;
resolveThemeTokens?: boolean;
outputFormat?: ColorOutputFormat;
+ disabled?: boolean;
}
const normalizeColorToHex = (color: string): string => {
if (!color) return '';
if (color.startsWith('#')) {
- return color.toLowerCase();
+ const lower = color.toLowerCase();
+ // Expand 3/4-digit shorthand so it compares equal to the six/eight-digit
+ // hex the picker emits.
+ if (/^#[0-9a-f]{3,4}$/.test(lower)) {
Review Comment:
Good catch. Opaque eight-digit hex (including expanded four-digit) now drops
the trailing `ff`, so it compares equal to what the picker emits and the Red
chip maps back to `Red`.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:
##########
@@ -103,23 +130,57 @@ 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;
+ };
+ // Bands are drawn outermost first, so a band without its own color sits on
+ // top of the nearest outer custom fill; its label must contrast with that.
+ let outerCustomFill: string | undefined;
+ const bandLabelFills = sortedRanges.map(({ color }) => {
+ if (color) outerCustomFill = color;
+ return color ?? outerCustomFill;
+ });
Review Comment:
Applied your suggestion: only opaque-enough custom fills (non-null
luminance) count as the outer fill, so `#ffffff33` over `#000000` gets light
text.
--
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]
