This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch feat/granular-theming-v2 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 96e8ddc95c1d5e891179c99b2a21a34b9cee64a4 Author: Claude <[email protected]> AuthorDate: Wed May 13 10:28:03 2026 -0700 feat(dashboard): per-component theme provider — Phase 1 (Chart PoC) Adds the skeleton for granular (per-component) theming on dashboard grid components, with the inheritance chain: Instance theme -> Dashboard theme -> Tab theme -> Row/Col theme -> Chart/Markdown theme This commit lands Phase 1 from the SIP (`SIP.md` at repo root): the storage shape and the resolver, wired into `ChartHolder` as the proof-of-concept call site. No UI yet — `themeId` must be set via Redux devtools / position_json hand-edit to verify visually. Phase 2 will introduce the `ComponentHeaderControls` menu and Phase 3 the `ThemeSelectorModal` that drives this from a real UI. Surface: - `LayoutItemMeta.themeId?: number | null` — optional CRUD theme id stored per-component in `position_json` meta (no schema migration; the meta map is already open-ended). `null` and `undefined` both mean "inherit from parent". - `pickEffectiveThemeId(layoutId, layout)` — pure resolver. Walks `parents` up the layout map from the given node until it finds a numeric `themeId` or hits `DASHBOARD_ROOT_ID`. Hop-capped at 32 to defend against malformed parents chains. - `useEffectiveThemeId(layoutId)` — Redux hook variant. - `<ComponentThemeProvider layoutId={...}>` — wraps children in the resolved theme's `SupersetThemeProvider`. Lazy-fetches via the existing `ThemeController.createDashboardThemeProvider`, which already caches by id so N components sharing one theme = 1 fetch. Pass-through when no ancestor sets a `themeId`. - `ChartHolder.tsx` — wraps the existing `<AntdThemeProvider>` (which is a popup-container shim for fullscreen mode, not a token provider) so per-component tokens are set before antd's ConfigProvider for popup targeting. Tests: 8 unit cases for `pickEffectiveThemeId` covering own / inherited / null-skip / no-ancestor / root-stop / malformed-parents / other-meta / missing-id. Closes the spirit of the closed PR #36749 (which became unrebasable after .jsx -> .tsx + React 18 + theme controller churn). Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- SIP.md | 44 +++++++- .../components/ComponentThemeProvider/index.tsx | 84 ++++++++++++++ .../useEffectiveThemeId.test.ts | 121 +++++++++++++++++++++ .../ComponentThemeProvider/useEffectiveThemeId.ts | 69 ++++++++++++ .../gridComponents/ChartHolder/ChartHolder.tsx | 103 +++++++++--------- superset-frontend/src/dashboard/types.ts | 6 + 6 files changed, 371 insertions(+), 56 deletions(-) diff --git a/SIP.md b/SIP.md index ee08de1c724..fe079ed2b2c 100644 --- a/SIP.md +++ b/SIP.md @@ -147,15 +147,47 @@ Each phase brings its own tests; the cumulative bar: ## Implementation log -- _(Phase 1)_ — in progress. See section below for status notes. +- _(Phase 1)_ — ✅ landed locally. `LayoutItemMeta.themeId`, + `ComponentThemeProvider` + `useEffectiveThemeId` hook, wired into + `ChartHolder`. 8 passing unit tests. No UI yet — `themeId` has to be + set via Redux devtools or position_json hand-edit to verify visually. - _(Phase 2)_ — pending. - _(Phase 3)_ — pending. - _(Phase 4)_ — pending. ### Phase 1 status -- [ ] Add optional `themeId` field to `LayoutItemMeta`. -- [ ] Build `ComponentThemeProvider` (Redux selector + parents walk + AntD theme injection). -- [ ] Wire into `ChartHolder` as the proof of concept (Chart is the leaf with the most existing theme integration, so it's the riskiest first target). -- [ ] Add unit tests for the resolution hook. -- [ ] Update this SIP with any architecture surprises uncovered during the wiring. +- [x] Add optional `themeId` field to `LayoutItemMeta`. (`src/dashboard/types.ts`) +- [x] Build `ComponentThemeProvider` — `pickEffectiveThemeId` resolver (pure + function, walks `parents` up the layout map until it finds a non-null + `themeId` or hits `DASHBOARD_ROOT_ID`) + `useEffectiveThemeId` Redux hook + + `ComponentThemeProvider` that lazy-fetches the resolved theme via the + existing `ThemeController.createDashboardThemeProvider` (which caches by + id, so N components referencing the same theme = 1 fetch). Renders as a + pass-through when no ancestor sets a `themeId`. +- [x] Wire into `ChartHolder` — wrapped around the existing + `<AntdThemeProvider>` so per-component theme tokens apply to the chart + body while the existing popup-container `ConfigProvider` continues to + work in fullscreen mode. +- [x] Add unit tests — 8 cases for `pickEffectiveThemeId` covering own-id / + inheritance / null-skip / no-ancestor / root-stop / malformed-parents / + other-meta-keys / missing-id. +- [x] Update SIP with surprises uncovered during wiring (none significant — + the existing `createDashboardThemeProvider` did exactly what we needed, + including caching by id; the only structural decision was treating the + ChartHolder's `<AntdThemeProvider>` as a popup-container shim rather than + a token provider, and nesting our provider outside it). + +#### Phase 1 surprises / notes + +- `ThemeController.createDashboardThemeProvider` already does Theme.fromConfig + with the right dark/light base + font loading + caching. We did not need + to duplicate any of that logic in the component-level provider. +- The provider is `useState` + `useEffect` rather than `useMemo` because the + fetch is async. That means there's a one-frame flash of the parent theme + before the component theme kicks in. Probably acceptable; if not, we can + Suspense-ify in Phase 4. +- `useEffectiveThemeId` re-runs on every Redux state change because the + selector returns a primitive `number | null` — that's fine for now, but + if dashboards get bigger we may want a memoized selector via reselect + keyed on `(layoutId, layout)` — file in the open questions section. diff --git a/superset-frontend/src/dashboard/components/ComponentThemeProvider/index.tsx b/superset-frontend/src/dashboard/components/ComponentThemeProvider/index.tsx new file mode 100644 index 00000000000..0822beaabd5 --- /dev/null +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/index.tsx @@ -0,0 +1,84 @@ +/** + * 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. + */ +import { type ReactNode, useEffect, useState } from 'react'; +import type { Theme } from '@apache-superset/core/theme'; +import { useThemeContext } from 'src/theme/ThemeProvider'; +import { useEffectiveThemeId } from './useEffectiveThemeId'; + +interface ComponentThemeProviderProps { + /** + * Layout item id (the key into `dashboardLayout.present`). The provider + * walks up the parents tree from this node to compute the effective + * theme override. + */ + layoutId: string | undefined; + children: ReactNode; +} + +/** + * Per-component theme override wrapper. When the component (or any + * ancestor up to but not including the dashboard root) sets a `themeId` + * in its `LayoutItemMeta`, this provider loads that CRUD theme and + * applies it as a `SupersetThemeProvider` around the children, overriding + * the dashboard-level (and, transitively, the instance-level) theme for + * this subtree. + * + * When no ancestor sets a `themeId` — the default — the component renders + * as a pass-through. The outer `CrudThemeProvider` (mounted by + * `DashboardPage`) continues to provide the dashboard-level theme. + */ +export default function ComponentThemeProvider({ + layoutId, + children, +}: ComponentThemeProviderProps) { + const effectiveThemeId = useEffectiveThemeId(layoutId); + const { createDashboardThemeProvider } = useThemeContext(); + const [componentTheme, setComponentTheme] = useState<Theme | null>(null); + + useEffect(() => { + if (effectiveThemeId == null) { + setComponentTheme(null); + return undefined; + } + let cancelled = false; + // `createDashboardThemeProvider` caches by id internally, so per-component + // calls for the same theme are deduplicated to a single fetch. + createDashboardThemeProvider(String(effectiveThemeId)).then(theme => { + if (!cancelled) setComponentTheme(theme); + }); + return () => { + cancelled = true; + }; + }, [effectiveThemeId, createDashboardThemeProvider]); + + if (!componentTheme) { + return <>{children}</>; + } + + return ( + <componentTheme.SupersetThemeProvider> + {children} + </componentTheme.SupersetThemeProvider> + ); +} + +export { + useEffectiveThemeId, + pickEffectiveThemeId, +} from './useEffectiveThemeId'; diff --git a/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.test.ts b/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.test.ts new file mode 100644 index 00000000000..1deb7d05745 --- /dev/null +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.test.ts @@ -0,0 +1,121 @@ +/** + * 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. + */ +import type { DashboardLayout } from 'src/dashboard/types'; +import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants'; +import { pickEffectiveThemeId } from './useEffectiveThemeId'; + +/** + * Helper to build a minimally-shaped `DashboardLayout` for these tests. + * The real reducer carries many more fields per node; only `parents` and + * `meta` are read by the resolver. + */ +const buildLayout = ( + nodes: Record< + string, + { + parents?: string[]; + themeId?: number | null; + extraMeta?: Record<string, unknown>; + } + >, +): DashboardLayout => + Object.fromEntries( + Object.entries(nodes).map(([id, { parents, themeId, extraMeta }]) => [ + id, + { + id, + type: 'CHART', + children: [], + parents, + meta: { + ...(extraMeta ?? {}), + ...(themeId !== undefined ? { themeId } : {}), + }, + }, + ]), + ) as unknown as DashboardLayout; + +test('returns null for missing layoutId', () => { + expect(pickEffectiveThemeId(undefined, buildLayout({}))).toBeNull(); + expect(pickEffectiveThemeId('CHART-1', buildLayout({}))).toBeNull(); +}); + +test("returns the node's own themeId when set", () => { + const layout = buildLayout({ + 'CHART-1': { + parents: [DASHBOARD_ROOT_ID, 'ROW-1'], + themeId: 42, + }, + 'ROW-1': { parents: [DASHBOARD_ROOT_ID], themeId: 7 }, + }); + expect(pickEffectiveThemeId('CHART-1', layout)).toBe(42); +}); + +test("inherits the closest ancestor's themeId when own is unset", () => { + const layout = buildLayout({ + 'CHART-1': { parents: [DASHBOARD_ROOT_ID, 'TAB-1', 'ROW-1'] }, + 'ROW-1': { parents: [DASHBOARD_ROOT_ID, 'TAB-1'], themeId: 7 }, + 'TAB-1': { parents: [DASHBOARD_ROOT_ID], themeId: 99 }, + }); + expect(pickEffectiveThemeId('CHART-1', layout)).toBe(7); +}); + +test('skips ancestors whose themeId is null and continues walking', () => { + // A literal `null` on a node means "I don't override" — keep walking. + const layout = buildLayout({ + 'CHART-1': { parents: [DASHBOARD_ROOT_ID, 'TAB-1', 'ROW-1'] }, + 'ROW-1': { parents: [DASHBOARD_ROOT_ID, 'TAB-1'], themeId: null }, + 'TAB-1': { parents: [DASHBOARD_ROOT_ID], themeId: 99 }, + }); + expect(pickEffectiveThemeId('CHART-1', layout)).toBe(99); +}); + +test('returns null when no ancestor sets a themeId', () => { + const layout = buildLayout({ + 'CHART-1': { parents: [DASHBOARD_ROOT_ID, 'ROW-1'] }, + 'ROW-1': { parents: [DASHBOARD_ROOT_ID] }, + }); + expect(pickEffectiveThemeId('CHART-1', layout)).toBeNull(); +}); + +test('stops at DASHBOARD_ROOT_ID — root-level theme is the dashboard CRUD theme, handled separately', () => { + const layout = buildLayout({ + 'CHART-1': { parents: [DASHBOARD_ROOT_ID] }, + [DASHBOARD_ROOT_ID]: { parents: [], themeId: 999 }, + }); + expect(pickEffectiveThemeId('CHART-1', layout)).toBeNull(); +}); + +test('does not loop on a malformed parents chain', () => { + // Self-referential parent shouldn't hang the resolver. + const layout = buildLayout({ + 'CHART-1': { parents: ['CHART-1'] }, + }); + expect(pickEffectiveThemeId('CHART-1', layout)).toBeNull(); +}); + +test('ignores other meta keys', () => { + const layout = buildLayout({ + 'CHART-1': { + parents: [DASHBOARD_ROOT_ID], + extraMeta: { sliceName: 'Foo', width: 4, background: 'gray' }, + }, + }); + expect(pickEffectiveThemeId('CHART-1', layout)).toBeNull(); +}); diff --git a/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.ts b/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.ts new file mode 100644 index 00000000000..be42aa2b05a --- /dev/null +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.ts @@ -0,0 +1,69 @@ +/** + * 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. + */ +import { useSelector } from 'react-redux'; +import type { DashboardLayout, RootState } from 'src/dashboard/types'; +import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants'; + +/** + * Walks up the dashboard layout tree from `layoutId` and returns the first + * `themeId` it finds, or `null` if no ancestor sets one. + * + * Inheritance order (closest wins): + * Chart/Markdown -> Row/Column -> Tab -> Dashboard root -> (null = inherit + * from dashboard CRUD theme or instance theme, applied by CrudThemeProvider + * higher in the tree). + * + * `themeId: null` on a node means "explicitly clear my override" — we treat + * the property as absent (and continue walking) iff it is undefined; a literal + * `null` is also treated as "no override" since the dashboard-level theme is + * applied by a different provider. + */ +export function pickEffectiveThemeId( + layoutId: string | undefined, + layout: DashboardLayout, +): number | null { + if (!layoutId || !layout) return null; + let cursorId: string | undefined = layoutId; + // Defensive cap — dashboards shouldn't nest deeper than this, and the cap + // protects against malformed `parents` arrays causing infinite loops. + let hops = 0; + while (cursorId && cursorId !== DASHBOARD_ROOT_ID && hops < 32) { + const node = layout[cursorId]; + if (!node) return null; + const themeId = (node.meta as { themeId?: number | null } | undefined) + ?.themeId; + if (typeof themeId === 'number') return themeId; + cursorId = node.parents?.[node.parents.length - 1]; + hops += 1; + } + return null; +} + +/** + * Redux hook variant of `pickEffectiveThemeId`. Memoizes on the layout + * reference; consumers that only care about the resolved id (not the layout + * map itself) won't re-render when sibling components change their meta. + */ +export function useEffectiveThemeId( + layoutId: string | undefined, +): number | null { + return useSelector<RootState, number | null>(state => + pickEffectiveThemeId(layoutId, state.dashboardLayout?.present), + ); +} diff --git a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx index 7894796ed62..cc5b7fe9b77 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx @@ -33,6 +33,7 @@ import ResizableContainer from 'src/dashboard/components/resizable/ResizableCont import getChartAndLabelComponentIdFromPath from 'src/dashboard/util/getChartAndLabelComponentIdFromPath'; import useFilterFocusHighlightStyles from 'src/dashboard/util/useFilterFocusHighlightStyles'; import { AntdThemeProvider } from '@superset-ui/core/components'; +import ComponentThemeProvider from 'src/dashboard/components/ComponentThemeProvider'; import { COLUMN_TYPE, ROW_TYPE } from 'src/dashboard/util/componentTypes'; import { GRID_BASE_UNIT, @@ -284,59 +285,61 @@ const ChartHolder = ({ outlinedComponentId ? 'fade-in' : 'fade-out', )} > - <AntdThemeProvider - getPopupContainer={(triggerNode: HTMLElement) => - document.fullscreenElement - ? (triggerNode?.closest?.( - '[data-test="dashboard-component-chart-holder"]', - ) as HTMLElement) || document.body - : document.body - } - > - {!editMode && ( - <AnchorLink - id={component.id} - scrollIntoView={outlinedComponentId === component.id} - /> - )} - {!!outlinedComponentId && ( - <style> - {`label[for=${outlinedColumnName}] + .Select .Select__control { + <ComponentThemeProvider layoutId={component.id}> + <AntdThemeProvider + getPopupContainer={(triggerNode: HTMLElement) => + document.fullscreenElement + ? (triggerNode?.closest?.( + '[data-test="dashboard-component-chart-holder"]', + ) as HTMLElement) || document.body + : document.body + } + > + {!editMode && ( + <AnchorLink + id={component.id} + scrollIntoView={outlinedComponentId === component.id} + /> + )} + {!!outlinedComponentId && ( + <style> + {`label[for=${outlinedColumnName}] + .Select .Select__control { border-color: ${theme.colorPrimary}; transition: border-color 1s ease-in-out; }`} - </style> - )} - <Chart - componentId={component.id} - id={component.meta.chartId ?? 0} - dashboardId={dashboardId} - width={chartWidth} - height={chartHeight} - sliceName={ - component.meta.sliceNameOverride || - component.meta.sliceName || - '' - } - updateSliceName={(_sliceId: number, name: string) => - handleUpdateSliceName(name) - } - isComponentVisible={isComponentVisible} - handleToggleFullSize={handleToggleFullSize} - isFullSize={isFullSize} - setControlValue={handleExtraControl} - extraControls={extraControls} - isInView={isInView} - chartHolderRef={chartHolderRef} - /> - {editMode && ( - <HoverMenu position="top"> - <div data-test="dashboard-delete-component-button"> - <DeleteComponentButton onDelete={handleDeleteComponent} /> - </div> - </HoverMenu> - )} - </AntdThemeProvider> + </style> + )} + <Chart + componentId={component.id} + id={component.meta.chartId ?? 0} + dashboardId={dashboardId} + width={chartWidth} + height={chartHeight} + sliceName={ + component.meta.sliceNameOverride || + component.meta.sliceName || + '' + } + updateSliceName={(_sliceId: number, name: string) => + handleUpdateSliceName(name) + } + isComponentVisible={isComponentVisible} + handleToggleFullSize={handleToggleFullSize} + isFullSize={isFullSize} + setControlValue={handleExtraControl} + extraControls={extraControls} + isInView={isInView} + chartHolderRef={chartHolderRef} + /> + {editMode && ( + <HoverMenu position="top"> + <div data-test="dashboard-delete-component-button"> + <DeleteComponentButton onDelete={handleDeleteComponent} /> + </div> + </HoverMenu> + )} + </AntdThemeProvider> + </ComponentThemeProvider> </div> </ResizableContainer> ), diff --git a/superset-frontend/src/dashboard/types.ts b/superset-frontend/src/dashboard/types.ts index 2a7056bebab..74e8815179b 100644 --- a/superset-frontend/src/dashboard/types.ts +++ b/superset-frontend/src/dashboard/types.ts @@ -278,6 +278,12 @@ export type LayoutItemMeta = { code?: string; /** Background style value for columns and rows */ background?: string; + /** + * Optional per-component theme override (CRUD `theme` resource id). + * `null` means "explicitly inherit"; a missing key means the same thing + * semantically. See ComponentThemeProvider for the inheritance walk. + */ + themeId?: number | null; /** Allow additional meta properties used by different component types */ [key: string]: unknown; };
