diff --git a/SIP.md b/SIP.md index 306beff3ad3..b7fc86069e1 100644 --- a/SIP.md +++ b/SIP.md @@ -100,7 +100,12 @@ together rather than as separate churn passes. Modal triggered from "Apply theme" in `ComponentHeaderControls`. Shows: - A theme picker populated from the CRUD `/api/v1/theme/` endpoint (same source the dashboard-level picker uses). - A "Clear override (inherit)" button when `themeId` is already set. -- Preview is **deferred** — initial scope is just save/cancel. +- **Live preview**: as the user picks options the targeted component + re-renders with the candidate theme tokens immediately, *without* + marking the dashboard dirty. Cancel reverts; Apply commits to Redux. + Implemented via a tiny module-level subscribable `previewThemeStore` + + `useSyncExternalStore` in `useEffectiveThemeId` (preview wins over + the Redux-resolved id when present). On save it dispatches a Redux action that updates the component's `meta.themeId` and marks the dashboard dirty. @@ -155,7 +160,6 @@ Each phase brings its own tests; the cumulative bar: ## Out-of-scope (potential follow-ups) - **Theme presets** — "apply this theme to all charts of viz type X" via a dashboard-level rule. -- **Live preview** in `ThemeSelectorModal`. - **Theme inheritance debugger** — devtools view that shows which level set each token for a hovered component. - **Bulk operations** — multi-select components and apply a theme to all. diff --git a/superset-frontend/src/dashboard/components/ComponentThemeProvider/previewThemeStore.test.ts b/superset-frontend/src/dashboard/components/ComponentThemeProvider/previewThemeStore.test.ts new file mode 100644 index 00000000000..4c7f1071eea --- /dev/null +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/previewThemeStore.test.ts @@ -0,0 +1,70 @@ +/** + * 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 { previewThemeStore } from './previewThemeStore'; + +afterEach(() => { + // Defensive — module-level state would leak between tests otherwise. + previewThemeStore.clear('CHART-a'); + previewThemeStore.clear('CHART-b'); +}); + +test('get returns undefined for unknown layoutId', () => { + expect(previewThemeStore.get('CHART-a')).toBeUndefined(); +}); + +test('set stores a numeric preview readable by get', () => { + previewThemeStore.set('CHART-a', 7); + expect(previewThemeStore.get('CHART-a')).toBe(7); +}); + +test('set stores explicit null (distinct from "no preview")', () => { + previewThemeStore.set('CHART-a', null); + expect(previewThemeStore.get('CHART-a')).toBeNull(); + // Distinct from the unknown-key case + expect(previewThemeStore.get('CHART-b')).toBeUndefined(); +}); + +test('clear removes the entry; subsequent get returns undefined', () => { + previewThemeStore.set('CHART-a', 7); + previewThemeStore.clear('CHART-a'); + expect(previewThemeStore.get('CHART-a')).toBeUndefined(); +}); + +test('subscribers fire on set and clear, do not fire on no-op set', () => { + const listener = jest.fn(); + const unsubscribe = previewThemeStore.subscribe(listener); + previewThemeStore.set('CHART-a', 7); + previewThemeStore.set('CHART-a', 7); // no-op (same value) + previewThemeStore.set('CHART-a', 9); + previewThemeStore.clear('CHART-a'); + previewThemeStore.clear('CHART-a'); // no-op + unsubscribe(); + previewThemeStore.set('CHART-a', 1); // not observed (unsubscribed) + expect(listener).toHaveBeenCalledTimes(3); +}); + +test('multiple layoutIds are tracked independently', () => { + previewThemeStore.set('CHART-a', 1); + previewThemeStore.set('CHART-b', 2); + expect(previewThemeStore.get('CHART-a')).toBe(1); + expect(previewThemeStore.get('CHART-b')).toBe(2); + previewThemeStore.clear('CHART-a'); + expect(previewThemeStore.get('CHART-a')).toBeUndefined(); + expect(previewThemeStore.get('CHART-b')).toBe(2); +}); diff --git a/superset-frontend/src/dashboard/components/ComponentThemeProvider/previewThemeStore.ts b/superset-frontend/src/dashboard/components/ComponentThemeProvider/previewThemeStore.ts new file mode 100644 index 00000000000..50f6d096f25 --- /dev/null +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/previewThemeStore.ts @@ -0,0 +1,74 @@ +/** + * 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. + */ + +/** + * Module-level subscribable store for transient per-component theme + * previews. Used by `ThemeSelectorModal` to make a draft selection + * visually applied without committing to Redux (which would mark the + * dashboard dirty). `ComponentThemeProvider` subscribes via + * `useSyncExternalStore` and prefers a present preview over the + * resolved-from-Redux `themeId`. + * + * `null` means "explicitly clear the override during preview" — the + * provider treats it the same way it treats a Redux `null`. Absence + * (key not in the map) means "no preview active; use Redux". + */ + +type PreviewValue = number | null; +type Listener = () => void; + +const previewMap = new Map(); +const listeners = new Set(); + +const emit = (): void => { + listeners.forEach(l => l()); +}; + +export const previewThemeStore = { + /** Sets a transient preview for `layoutId`. Replaces any prior preview. */ + set(layoutId: string, themeId: PreviewValue): void { + if (previewMap.get(layoutId) === themeId) return; + previewMap.set(layoutId, themeId); + emit(); + }, + + /** Clears any preview for `layoutId`. No-op when none is active. */ + clear(layoutId: string): void { + if (!previewMap.has(layoutId)) return; + previewMap.delete(layoutId); + emit(); + }, + + /** + * Returns the previewed value for `layoutId`, or `undefined` when no + * preview is active. Used by `ComponentThemeProvider` via + * `useSyncExternalStore`. Returning `undefined` (vs `null`) lets + * callers distinguish "no preview" from "preview the cleared state". + */ + get(layoutId: string): PreviewValue | undefined { + return previewMap.has(layoutId) ? previewMap.get(layoutId) : undefined; + }, + + subscribe(listener: Listener): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; + }, +}; diff --git a/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.ts b/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.ts index be42aa2b05a..97cbfd8c06c 100644 --- a/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.ts +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/useEffectiveThemeId.ts @@ -16,9 +16,11 @@ * specific language governing permissions and limitations * under the License. */ +import { useSyncExternalStore } from 'react'; import { useSelector } from 'react-redux'; import type { DashboardLayout, RootState } from 'src/dashboard/types'; import { DASHBOARD_ROOT_ID } from 'src/dashboard/util/constants'; +import { previewThemeStore } from './previewThemeStore'; /** * Walks up the dashboard layout tree from `layoutId` and returns the first @@ -59,11 +61,22 @@ export function pickEffectiveThemeId( * 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. + * + * If `ThemeSelectorModal` has registered a preview override for this + * `layoutId` via `previewThemeStore`, the preview wins — that's how the + * modal applies a draft selection visually without committing to Redux. */ export function useEffectiveThemeId( layoutId: string | undefined, ): number | null { - return useSelector(state => + const reduxResolved = useSelector(state => pickEffectiveThemeId(layoutId, state.dashboardLayout?.present), ); + const preview = useSyncExternalStore( + previewThemeStore.subscribe, + () => + layoutId === undefined ? undefined : previewThemeStore.get(layoutId), + () => undefined, + ); + return preview === undefined ? reduxResolved : preview; } diff --git a/superset-frontend/src/dashboard/components/ThemeSelectorModal/index.tsx b/superset-frontend/src/dashboard/components/ThemeSelectorModal/index.tsx index 19ec62b2411..b6e747c0f2b 100644 --- a/superset-frontend/src/dashboard/components/ThemeSelectorModal/index.tsx +++ b/superset-frontend/src/dashboard/components/ThemeSelectorModal/index.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useEffect, useMemo, useState } from 'react'; +import { useEffect, useMemo, useRef, useState } from 'react'; import rison from 'rison'; import { useDispatch } from 'react-redux'; import { SupersetClient } from '@superset-ui/core'; @@ -24,6 +24,7 @@ import { t } from '@apache-superset/core/translation'; import { Button, Modal, Select } from '@superset-ui/core/components'; import { useToasts } from 'src/components/MessageToasts/withToasts'; import { useEffectiveThemeId } from 'src/dashboard/components/ComponentThemeProvider'; +import { previewThemeStore } from 'src/dashboard/components/ComponentThemeProvider/previewThemeStore'; import { setComponentThemeId } from 'src/dashboard/actions/setComponentThemeId'; interface ThemeOption { @@ -57,17 +58,43 @@ export default function ThemeSelectorModal({ const currentThemeId = useEffectiveThemeId(layoutId); // Modal-local draft of the selection. Synced from the resolved id when - // the modal opens; only committed to Redux on save. + // the modal opens; live-previewed via the previewThemeStore as the user + // picks options; only committed to Redux on Apply. const [selectedId, setSelectedId] = useState(currentThemeId); const [themes, setThemes] = useState([]); const [loading, setLoading] = useState(false); - // Keep the draft in sync if the resolved id changes while the modal is - // open (e.g. another tab updated the dashboard). Cheap because the - // selector returns a primitive. + // Snapshot the resolved id at open-time so we can revert correctly when + // the user cancels — `currentThemeId` itself is reactive (and would + // already reflect the in-flight preview), so we can't use it directly. + const initialIdRef = useRef(currentThemeId); + useEffect(() => { - if (show) setSelectedId(currentThemeId); - }, [show, currentThemeId]); + if (show) { + initialIdRef.current = currentThemeId; + setSelectedId(currentThemeId); + } + // No `show` cleanup here — the close handlers below clear the preview + // explicitly so we don't fight with the Apply path (which keeps the + // theme applied). + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [show]); + + // Push the user's draft selection through the preview store. The + // ComponentThemeProvider prefers preview > Redux, so the targeted + // component re-renders with the candidate theme as soon as this updates. + useEffect(() => { + if (!show) return undefined; + previewThemeStore.set(layoutId, selectedId); + return () => { + // Cleanup runs on close + on every selectedId change; the next + // effect call re-sets it. On unmount/close we want the preview + // gone so the provider re-resolves from Redux. Safe because Apply + // commits to Redux *before* hiding the modal, so the post-clear + // resolution lands on the saved value, not the original. + previewThemeStore.clear(layoutId); + }; + }, [show, layoutId, selectedId]); useEffect(() => { if (!show) return;