rusackas commented on code in PR #43070: URL: https://github.com/apache/superset/pull/43070#discussion_r4028161540
########## superset-frontend/src/core/dashboard/chartTheme.ts: ########## @@ -0,0 +1,152 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview What the active theme means for a chart, stated without + * reference to any charting library. + * + * Blocks draw with whatever renderer they like — ECharts, Vega-Lite, plain SVG — + * and each library ships its own palette and its own near-black text. Left to + * itself, every block therefore looks like its library rather than like + * Superset, and two blocks on one dashboard disagree about what "the first + * series" is. The only affordance a contributed block had here was + * `getCategoricalColors()`, so each one hand-rolled the rest from raw tokens: + * three extensions, three different mappings, all destined to drift. + * + * The fix is that theme compatibility should be the default a block starts from + * rather than something its author remembers to implement. So the host states + * the theme once, semantically, and each renderer maps those few fields onto its + * own config — a mapping that is a handful of lines, unlike re-deriving what a + * theme means. A block that genuinely wants different colours still says so: its + * own spec is merged *over* this, never under it. + * + * Superset's sequential schemes are included because nothing was exposing them + * at all, so any continuous colour a block drew — a heatmap, a `visualMap`, a + * choropleth — fell back to its library's default ramp. That is the most + * visible mismatch of the lot, and the least excusable, since Superset has had + * the schemes all along. + */ + +import { + CategoricalColorNamespace, + getSequentialSchemeRegistry, +} from '@superset-ui/core'; +import type { useTheme } from '@apache-superset/core/theme'; + +type Theme = ReturnType<typeof useTheme>; + +export interface ChartTheme { + /** Transparent, so a chart sits on the block surface rather than over it. */ + background: string; + text: { + color: string; + /** Text that labels rather than states — axis ticks, legend entries. */ + mutedColor: string; + /** Text that is present but inactive — a toggled-off legend entry. */ + disabledColor: string; + fontFamily: string; + fontSize: number; + }; + axis: { + lineColor: string; + labelColor: string; + gridColor: string; + /** Minor gridlines, where a renderer draws them. */ + minorGridColor: string; + }; + tooltip: { + background: string; + color: string; + }; + /** Accent for hover markers, crosshairs, selection. */ + accent: string; + /** The active categorical scheme, in order. One colour per series. */ + categoricalColors: string[]; + /** + * The colour for a named series or category. + * + * By position, "EMEA" is the second colour in a chart that lists it second + * and the fifth in one that does not, so the same category comes out a + * different colour in every block it appears in — the thing that most makes + * a set of blocks read as unrelated charts rather than as one dashboard. + * Superset already solves this: the scale remembers what it gave a label and + * gives it the same one again, which is also how the v1 charts beside a + * canvas resolve theirs. + */ + getColor: (label: string) => string; + /** + * The active sequential scheme, light to dark — for a continuous measure + * (heatmap cells, a colour ramp), where a categorical palette is wrong. + */ + sequentialColors: string[]; +} + +/** + * The default sequential scheme's colours, or an empty list if none is + * registered. Empty rather than a hardcoded fallback ramp: a renderer that gets + * nothing keeps its own default, which is a better outcome than inventing a + * Superset-looking ramp that no Superset chart actually uses. + */ +function getSequentialColors(): string[] { + try { + return getSequentialSchemeRegistry().get()?.colors ?? []; + } catch { + return []; + } +} + +/** + * Reads live rather than being captured once: the active colour scheme and the + * light/dark theme can both change after any given module was imported. + * + * `scheme` is the canvas's own choice of categorical palette, stored on its + * root node; omitted, the deployment's default is used. Passed through to the + * scale rather than resolved here so that the label→colour memory is the + * shared one — a canvas and the v1 charts around it agree on what colour + * "EMEA" is. + */ +export function getChartTheme(theme: Theme, scheme?: string): ChartTheme { + const scale = CategoricalColorNamespace.getScale(scheme); + return { + background: 'transparent', + text: { + color: theme.colorText, + mutedColor: theme.colorTextSecondary, + disabledColor: theme.colorTextDisabled, + fontFamily: theme.fontFamily, + fontSize: theme.fontSize, + }, + axis: { + lineColor: theme.colorSplit, + labelColor: theme.colorTextSecondary, + gridColor: theme.colorSplit, + minorGridColor: theme.colorBorderSecondary, + }, + tooltip: { + background: theme.colorBgContainer, + color: theme.colorText, + }, + accent: theme.colorPrimary, + categoricalColors: scale.colors, + getColor: (label: string) => scale.getColor(label), + sequentialColors: getSequentialColors(), Review Comment: Agreed, this looks real. `getScale()` here never gets a `sliceId`, so `getColor` skips `addSlice` and the shared dashboard label map never gets written to from these blocks. Two blocks listing the same label in a different series order would end up with different colors. ########## superset-frontend/src/core/dashboard/widgets/ChartWidget.tsx: ########## @@ -223,20 +229,52 @@ export default function ChartBlock({ nodeId }: { nodeId: string }) { // eslint-disable-next-line react-hooks/exhaustive-deps }, [bindingKey]); + const colorScheme = provider.getRoot().props?.colorScheme; + const chartTheme = useMemo( + () => + getChartTheme( + theme, + typeof colorScheme === 'string' ? colorScheme : undefined, + ), + [theme, colorScheme], + ); const option = useMemo(() => { if (!rows) return undefined; const resolved = resolveBindings( (node?.props?.echartsOptions as Record<string, unknown>) ?? {}, - { rows, theme }, + { rows, chartTheme, theme }, ); // The chart's name is drawn by the block's header, which reads it from // this same option (see `blockLabel`). Leaving it here too would print it // twice, at two sizes, in two places — and the header's copy is the one // that sits where every other block's name sits. const withoutTitle = { ...resolved }; delete withoutTitle.title; - return withoutTitle; - }, [node?.props?.echartsOptions, rows, theme]); + + // Everything an AI-authored option doesn't say for itself comes from the + // theme: text/axis/legend/tooltip colors, the categorical palette, and + // whatever chart overrides the theme carries. Merged *under* the option + // (rightmost source wins in `mergeEchartsThemeOverrides`), so an explicit + // choice in the spec still takes precedence. + const merged = mergeEchartsThemeOverrides( + { + ...getEchartsTheme(theme, withoutTitle), + // The fallback for a series with no name of its own. A named one is + // coloured by that name in `applySeriesDefaults`, so it never reaches + // this list. + color: chartTheme.categoricalColors, + backgroundColor: 'transparent', + }, + withoutTitle, + theme.echartsOptionsOverrides ?? {}, Review Comment: Agreed. `Echart.tsx` keys off `vizType` for `echartsOptionsOverridesByChartType`, but this only ever merges the global `echartsOptionsOverrides`. A deployment with chart-type overrides configured would silently not get them here. ########## superset-frontend/src/core/dashboard/index.ts: ########## @@ -53,4 +69,10 @@ export const dashboard: typeof dashboardApi = { updateProps: provider.updateProps.bind(provider), onDidLayoutChange: provider.onDidLayoutChange, fetchQueryData, + // Both read per call rather than captured once: the canvas's scheme, the + // deployment's default and the light/dark theme can each change after this + // module is imported. + getCategoricalColors: () => + CategoricalColorNamespace.getScale(canvasColorScheme()).colors, + getChartTheme: () => getChartTheme(themeObject.theme, canvasColorScheme()), Review Comment: Pretty sure this isn't two sources in practice. `themeObject.theme` and `useTheme()` both come off the same `Theme` instance in Theme.tsx, `setConfig` sets `this.theme` and calls `updateProviders` with that same object in the same breath, which is what feeds the emotion `ThemeProvider` state. They shouldn't be able to disagree. -- 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]
