This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch feat-granular-theming in repository https://gitbox.apache.org/repos/asf/superset.git
commit b70453212070323e6d016767abbef7dedf85b728 Author: Evan Rusackas <[email protected]> AuthorDate: Thu Dec 18 00:44:34 2025 -0800 feat(dashboard): add ComponentThemeProvider for granular theming Implements component-level theme application for dashboard components: - Add ComponentThemeProvider wrapper that loads and applies themes - Integrate theming into Row, Column, Tabs, Markdown, and ChartHolder - Wire up theme selector from SliceHeaderControls through Chart hierarchy - Fix re-render issues with stable callback dependencies using ref pattern - Add comprehensive tests for ComponentThemeProvider Theme inheritance now works: Instance → Dashboard → Tab → Row/Column → Chart 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> --- docs/GRANULAR_THEMING_PLAN.md | 232 ++++++++++++++++++ .../packages/superset-core/src/ui/theme/types.ts | 4 + .../ComponentThemeProvider.test.tsx | 267 +++++++++++++++++++++ .../components/ComponentThemeProvider/index.tsx | 237 ++++++++++++++++++ .../src/dashboard/components/SliceHeader/index.tsx | 4 + .../components/SliceHeaderControls/index.tsx | 44 +++- .../components/SliceHeaderControls/types.ts | 4 + .../components/gridComponents/Chart/Chart.jsx | 4 + .../gridComponents/ChartHolder/ChartHolder.tsx | 83 +++++-- .../components/gridComponents/Column/Column.jsx | 197 +++++++++++---- .../gridComponents/Column/Column.test.jsx | 63 ++--- .../gridComponents/Markdown/Markdown.jsx | 100 ++++---- .../components/gridComponents/Row/Row.test.tsx | 56 ++--- .../components/gridComponents/Row/Row.tsx | 200 +++++++++++---- .../components/gridComponents/Tabs/Tabs.jsx | 93 +++++-- .../components/gridComponents/Tabs/Tabs.test.tsx | 58 +++-- .../TabsRenderer/TabsRenderer.test.tsx | 2 + .../gridComponents/TabsRenderer/TabsRenderer.tsx | 47 +++- .../components/menu/ThemeSelectorModal/index.tsx | 18 +- superset-frontend/src/dashboard/types.ts | 1 + superset-frontend/src/theme/ThemeController.ts | 87 +++++++ superset-frontend/src/theme/ThemeProvider.tsx | 8 + 22 files changed, 1536 insertions(+), 273 deletions(-) diff --git a/docs/GRANULAR_THEMING_PLAN.md b/docs/GRANULAR_THEMING_PLAN.md index 1b1153bf5cd..77f01e4aec7 100644 --- a/docs/GRANULAR_THEMING_PLAN.md +++ b/docs/GRANULAR_THEMING_PLAN.md @@ -420,3 +420,235 @@ _Ongoing notes as we implement..._ **Note:** Theme selection is stored in component metadata (client-side). Backend persistence (Phase 3) will save this to dashboard `json_metadata.component_themes`. +### Session 3 - Bug Fix: setState Race Condition + +**Problem:** Clicking "Apply theme" menu item didn't open the modal. Debug logging showed: +- `handleOpenThemeSelector` was called and set `isThemeSelectorOpen: true` +- But the setState callback showed `isThemeSelectorOpen: false` +- ThemeSelectorModal received `show: false` + +**Root Cause:** `handleChangeEditorMode` used a non-functional setState pattern: +```javascript +const nextState = { ...this.state, editorMode: mode }; // Reads stale state +this.setState(nextState); // Overwrites pending isThemeSelectorOpen: true +``` + +When the dropdown menu closed, it triggered `handleChangeFocus → handleChangeEditorMode`, +which copied the old state (before React applied the pending `isThemeSelectorOpen: true`) +and overwrote it. + +**Fix:** Changed to functional setState: +```javascript +this.setState(prevState => ({ + ...prevState, + editorMode: mode, + ...(mode === 'preview' ? { hasError: false } : {}), +})); +``` + +**Status:** Modal now opens correctly. Theme selection is stored in component metadata. + +**Next Steps:** +1. ~~Phase 2.3: Add ComponentHeaderControls to Row/Column~~ DONE +2. Phase 2.4: Add ComponentHeaderControls to Tabs +3. Phase 1.2: Add theme selector to SliceHeaderControls (charts) +4. Phase 3: Backend persistence +5. Phase 4: Theme rendering/application + +### Session 3 - Phase 2.3: Row/Column Integration + +Updated Row and Column components to use `ComponentHeaderControls`: + +**Row.tsx changes:** +- Replaced `DeleteComponentButton` and gear `IconButton` with `ComponentHeaderControls` +- Removed `BackgroundStyleDropdown` from `WithPopoverMenu.menuItems` +- Added menu items: Background toggle (Transparent/Solid), Apply theme, Delete +- Added `ThemeSelectorModal` integration +- Fixed `backgroundStyle` variable ordering (moved before hooks that use it) + +**Column.jsx changes:** +- Same pattern as Row +- Replaced old UI elements with `ComponentHeaderControls` +- Added theme selector modal integration + +**Test updates:** +- Updated Row.test.tsx and Column.test.jsx to use new menu pattern +- Removed mocks for `DeleteComponentButton` +- Tests now click "More Options" menu button and select "Delete" from dropdown + +**Files modified:** +- `src/dashboard/components/gridComponents/Row/Row.tsx` +- `src/dashboard/components/gridComponents/Row/Row.test.tsx` +- `src/dashboard/components/gridComponents/Column/Column.jsx` +- `src/dashboard/components/gridComponents/Column/Column.test.jsx` + +**Status:** All tests pass (Row, Column, Markdown) + +### Session 4 - Phase 2.4: Tabs Integration + +Updated Tabs and TabsRenderer components to use `ComponentHeaderControls`: + +**TabsRenderer.tsx changes:** +- Replaced `DeleteComponentButton` with `ComponentHeaderControls` +- Added `handleOpenThemeSelector` prop to interface +- Added `handleMenuClick` callback for menu actions +- Added menu items: Apply theme, Delete +- Menu appears in HoverMenu alongside DragHandle + +**Tabs.jsx changes:** +- Added `ThemeSelectorModal` import and integration +- Added `isThemeSelectorOpen` state +- Added `handleOpenThemeSelector`, `handleCloseThemeSelector`, `handleApplyTheme` handlers +- Passed `handleOpenThemeSelector` to TabsRenderer +- Theme stored in `tabsComponent.meta.theme_id` + +**Test updates:** +- Updated TabsRenderer.test.tsx to include `handleOpenThemeSelector` in props +- Updated Tabs.test.tsx: + - Removed `DeleteComponentButton` mock (no longer used) + - Updated tests to check for `ComponentHeaderControls` via `[aria-label="More Options"]` + - Updated delete test to use menu pattern (click menu button, then "Delete" option) + +**Files modified:** +- `src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx` +- `src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.test.tsx` +- `src/dashboard/components/gridComponents/Tabs/Tabs.jsx` +- `src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx` + +**Status:** All tests pass (108 tests across 7 test suites) + +**Phase 2 Complete!** All dashboard components now have consistent `ComponentHeaderControls` menu: +- Markdown: Edit/Preview toggle, Apply theme, Delete +- Row: Background toggle, Apply theme, Delete +- Column: Background toggle, Apply theme, Delete +- Tabs: Apply theme, Delete + +**Next Steps:** +1. ~~Phase 1.2: Add theme selector to SliceHeaderControls (charts)~~ DONE +2. Phase 3: Backend persistence +3. Phase 4: Theme rendering/application + +### Session 4 - Phase 1.2: SliceHeaderControls (Chart) Theme Integration + +Added theme selector to chart menu in dashboard view: + +**types.ts changes:** +- Added `currentThemeId?: number | null` prop +- Added `onApplyTheme?: (themeId: number | null) => void` callback prop + +**dashboard/types.ts changes:** +- Added `ApplyTheme = 'apply_theme'` to MenuKeys enum + +**SliceHeaderControls/index.tsx changes:** +- Added `isThemeSelectorOpen` state +- Added `handleCloseThemeSelector` and `handleApplyTheme` handlers +- Added `ApplyTheme` case to handleMenuClick switch +- Added "Apply theme" menu item (only shown if `onApplyTheme` prop is provided) +- Added `ThemeSelectorModal` rendering + +**Design notes:** +- Theme selector only appears if parent passes `onApplyTheme` callback +- This allows gradual adoption - parent components can enable theming when ready +- Charts in dashboard view can have themes applied once parent wiring is complete + +**Files modified:** +- `src/dashboard/types.ts` +- `src/dashboard/components/SliceHeaderControls/types.ts` +- `src/dashboard/components/SliceHeaderControls/index.tsx` + +**Status:** All 33 tests pass + +**Remaining Work:** +1. ~~Wire up `onApplyTheme` callback in parent component (ChartHolder/SliceHeader)~~ DONE +2. Phase 3: Backend persistence for component themes +3. Phase 4: Theme rendering/application + +### Session 5 - Phase 4.2: ComponentThemeProvider Implementation + +Created `ComponentThemeProvider` for component-level theme application: + +**ComponentThemeProvider/index.tsx:** +- Wrapper component that loads and applies theme for dashboard components +- Fetches theme from ThemeController via `createTheme(themeId, parentThemeConfig)` +- Merges component theme with parent theme for proper inheritance +- Error boundary catches missing ThemeProvider (e.g., in tests) and gracefully falls back +- Uses ref for parent theme config to avoid infinite re-render loops + +**Integration:** +- Wrapped children of Row, Column, Tabs, Markdown, ChartHolder with ComponentThemeProvider +- Components pass `themeId={component.meta?.theme_id}` to the provider +- Theme inheritance works: Instance → Dashboard → Tab → Row/Column → Chart/Component + +**useComponentTheme hook:** +- Helper hook to get theme info for UI display +- Returns `{ themeId, themeName, hasTheme }` + +**Files created:** +- `src/dashboard/components/ComponentThemeProvider/index.tsx` +- `src/dashboard/components/ComponentThemeProvider/ComponentThemeProvider.test.tsx` + +**Status:** Theme application working. Themes now visually apply to components. + +### Session 5 - ChartHolder/SliceHeader Wiring Complete + +Wired up `onApplyTheme` from SliceHeaderControls through the component hierarchy: + +**ChartHolder.tsx changes:** +- Added `handleApplyTheme` callback that updates component.meta.theme_id +- Uses `getComponentById` pattern to avoid stale closures +- Passes `onApplyTheme` and `currentThemeId` to Chart component +- Wrapped Chart with `ComponentThemeProvider` + +**Chart.jsx and SliceHeader changes:** +- Props flow: ChartHolder → Chart → SliceHeader → SliceHeaderControls +- `onApplyTheme` and `currentThemeId` passed down the hierarchy +- Theme modal now accessible from chart's menu + +**Files modified:** +- `src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx` +- `src/dashboard/components/gridComponents/Chart/Chart.jsx` +- `src/dashboard/components/SliceHeader/index.tsx` + +### Session 5 - Stability Fixes for handleApplyTheme + +Fixed re-render issues caused by unstable callback dependencies: + +**Problem:** `handleApplyTheme` callbacks in Row, Column, Tabs had the full component object +or `props` in their dependency arrays, causing unnecessary re-renders. + +**Solution:** Applied ref pattern to all components: +1. Create ref: `const componentRef = useRef(component)` +2. Keep ref updated: `useEffect(() => { componentRef.current = component }, [component])` +3. Use ref in callback: `const currentComponent = componentRef.current` +4. Remove component from dependencies + +**Files modified:** +- `src/dashboard/components/gridComponents/Row/Row.tsx` +- `src/dashboard/components/gridComponents/Column/Column.jsx` +- `src/dashboard/components/gridComponents/Tabs/Tabs.jsx` +- `src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx` + +**ComponentThemeProvider re-render fix:** +- Fixed infinite re-render loop caused by `parentThemeConfig` in useEffect dependencies +- Changed to use ref pattern for parent theme config +- Simplified `useComponentTheme` hook to avoid setState in effect + +**Status:** All re-render issues resolved. Dashboard loads cleanly without excessive renders. + +### Current Status Summary + +**Completed:** +- Phase 1.2: Chart theme in dashboard (SliceHeaderControls) ✅ +- Phase 2.1: ComponentHeaderControls component ✅ +- Phase 2.2: Markdown menu integration ✅ +- Phase 2.3: Row/Column menu integration ✅ +- Phase 2.4: Tabs menu integration ✅ +- Phase 4.2: ComponentThemeProvider and theme application ✅ + +**In Progress:** +- Phase 3: Backend persistence (themes stored in component.meta, needs API save) + +**Not Started:** +- Phase 1.1: Chart theme in Explore view +- Theme visual indicators in edit mode + diff --git a/superset-frontend/packages/superset-core/src/ui/theme/types.ts b/superset-frontend/packages/superset-core/src/ui/theme/types.ts index ac905393f9a..192e6ea8a91 100644 --- a/superset-frontend/packages/superset-core/src/ui/theme/types.ts +++ b/superset-frontend/packages/superset-core/src/ui/theme/types.ts @@ -441,6 +441,10 @@ export interface ThemeContextType { canSetTheme: () => boolean; canDetectOSPreference: () => boolean; createDashboardThemeProvider: (themeId: string) => Promise<Theme | null>; + createTheme: ( + themeId: string, + parentThemeConfig?: AnyThemeConfig, + ) => Promise<Theme | null>; getAppliedThemeId: () => number | null; } diff --git a/superset-frontend/src/dashboard/components/ComponentThemeProvider/ComponentThemeProvider.test.tsx b/superset-frontend/src/dashboard/components/ComponentThemeProvider/ComponentThemeProvider.test.tsx new file mode 100644 index 00000000000..611fcc72d75 --- /dev/null +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/ComponentThemeProvider.test.tsx @@ -0,0 +1,267 @@ +/** + * 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 { ReactNode } from 'react'; +import { render, screen, waitFor, act } from '@testing-library/react'; +import { renderHook } from '@testing-library/react-hooks'; +import { Theme } from '@apache-superset/core/ui'; +import ComponentThemeProvider, { + useComponentTheme, +} from './index'; + +// Mock the ThemeProvider module +jest.mock('src/theme/ThemeProvider', () => ({ + useThemeContext: jest.fn(), +})); + +// Get reference to mocked useThemeContext +// eslint-disable-next-line @typescript-eslint/no-require-imports +const { useThemeContext } = require('src/theme/ThemeProvider'); + +// Mock theme object +const mockTheme = { + SupersetThemeProvider: ({ children }: { children: ReactNode }) => ( + <div data-testid="component-theme-provider">{children}</div> + ), + toSerializedConfig: jest.fn().mockReturnValue({ colorPrimary: '#1890ff' }), +} as unknown as Theme; + +const mockLoadedTheme = { + SupersetThemeProvider: ({ children }: { children: ReactNode }) => ( + <div data-testid="loaded-theme-provider">{children}</div> + ), +} as unknown as Theme; + +test('renders children directly when no themeId is provided', () => { + render( + <ComponentThemeProvider> + <div data-testid="test-child">Hello World</div> + </ComponentThemeProvider>, + ); + + expect(screen.getByTestId('test-child')).toBeInTheDocument(); + expect(screen.getByText('Hello World')).toBeInTheDocument(); +}); + +test('renders children directly when themeId is null', () => { + render( + <ComponentThemeProvider themeId={null}> + <div data-testid="test-child">Hello World</div> + </ComponentThemeProvider>, + ); + + expect(screen.getByTestId('test-child')).toBeInTheDocument(); +}); + +test('renders children when themeId is provided and theme loads successfully', async () => { + const mockCreateTheme = jest.fn().mockResolvedValue(mockLoadedTheme); + + useThemeContext.mockReturnValue({ + theme: mockTheme, + createTheme: mockCreateTheme, + }); + + await act(async () => { + render( + <ComponentThemeProvider themeId={123}> + <div data-testid="test-child">Themed Content</div> + </ComponentThemeProvider>, + ); + }); + + await waitFor(() => { + expect(screen.getByTestId('test-child')).toBeInTheDocument(); + }); + + expect(mockCreateTheme).toHaveBeenCalledWith('123', { colorPrimary: '#1890ff' }); +}); + +test('wraps children with theme provider when theme loads', async () => { + const mockCreateTheme = jest.fn().mockResolvedValue(mockLoadedTheme); + + useThemeContext.mockReturnValue({ + theme: mockTheme, + createTheme: mockCreateTheme, + }); + + await act(async () => { + render( + <ComponentThemeProvider themeId={456}> + <div data-testid="test-child">Themed Content</div> + </ComponentThemeProvider>, + ); + }); + + await waitFor(() => { + expect(screen.getByTestId('loaded-theme-provider')).toBeInTheDocument(); + }); + + expect(screen.getByTestId('test-child')).toBeInTheDocument(); +}); + +test('renders children without wrapper when theme loading fails', async () => { + const mockCreateTheme = jest + .fn() + .mockRejectedValue(new Error('Failed to load theme')); + + useThemeContext.mockReturnValue({ + theme: mockTheme, + createTheme: mockCreateTheme, + }); + + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(() => {}); + + await act(async () => { + render( + <ComponentThemeProvider themeId={789}> + <div data-testid="test-child">Content</div> + </ComponentThemeProvider>, + ); + }); + + await waitFor(() => { + expect(screen.getByTestId('test-child')).toBeInTheDocument(); + }); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Failed to load component theme 789:', + expect.any(Error), + ); + + consoleErrorSpy.mockRestore(); +}); + +test('gracefully handles missing ThemeProvider (error boundary fallback)', () => { + // Simulate useThemeContext throwing an error + useThemeContext.mockImplementation(() => { + throw new Error('useThemeContext must be used within a ThemeProvider'); + }); + + // Should not throw, should render children via error boundary + render( + <ComponentThemeProvider themeId={123}> + <div data-testid="test-child">Fallback Content</div> + </ComponentThemeProvider>, + ); + + expect(screen.getByTestId('test-child')).toBeInTheDocument(); +}); + +test('re-throws non-ThemeProvider errors', () => { + useThemeContext.mockImplementation(() => { + throw new Error('Some other error'); + }); + + // This should throw because it's not a ThemeProvider error + expect(() => { + render( + <ComponentThemeProvider themeId={123}> + <div>Content</div> + </ComponentThemeProvider>, + ); + }).toThrow('Some other error'); +}); + +test('useComponentTheme returns theme info when themeId is provided', () => { + const { result } = renderHook(() => useComponentTheme(42)); + + expect(result.current.themeId).toBe(42); + expect(result.current.themeName).toBe('Theme 42'); + expect(result.current.hasTheme).toBe(true); +}); + +test('useComponentTheme returns null values when themeId is not provided', () => { + const { result } = renderHook(() => useComponentTheme(null)); + + expect(result.current.themeId).toBeNull(); + expect(result.current.themeName).toBeNull(); + expect(result.current.hasTheme).toBe(false); +}); + +test('useComponentTheme returns null values when themeId is undefined', () => { + const { result } = renderHook(() => useComponentTheme(undefined)); + + expect(result.current.themeId).toBeUndefined(); + expect(result.current.themeName).toBeNull(); + expect(result.current.hasTheme).toBe(false); +}); + +test('shows fallback while loading when provided', async () => { + let resolveTheme: (theme: Theme) => void; + const themePromise = new Promise<Theme>(resolve => { + resolveTheme = resolve; + }); + const mockCreateTheme = jest.fn().mockReturnValue(themePromise); + + useThemeContext.mockReturnValue({ + theme: mockTheme, + createTheme: mockCreateTheme, + }); + + render( + <ComponentThemeProvider + themeId={999} + fallback={<div data-testid="loading">Loading...</div>} + > + <div data-testid="test-child">Content</div> + </ComponentThemeProvider>, + ); + + // During loading, fallback should be shown (or children if no theme yet) + expect(screen.getByTestId('test-child')).toBeInTheDocument(); + + // Resolve the theme + await act(async () => { + resolveTheme!(mockLoadedTheme); + }); + + await waitFor(() => { + expect(screen.getByTestId('loaded-theme-provider')).toBeInTheDocument(); + }); +}); + +test('cleans up properly when unmounted during loading', async () => { + let resolveTheme: (theme: Theme) => void; + const themePromise = new Promise<Theme>(resolve => { + resolveTheme = resolve; + }); + const mockCreateTheme = jest.fn().mockReturnValue(themePromise); + + useThemeContext.mockReturnValue({ + theme: mockTheme, + createTheme: mockCreateTheme, + }); + + const { unmount } = render( + <ComponentThemeProvider themeId={111}> + <div data-testid="test-child">Content</div> + </ComponentThemeProvider>, + ); + + // Unmount before theme loads + unmount(); + + // Resolve the theme after unmount - should not cause errors + await act(async () => { + resolveTheme!(mockLoadedTheme); + }); + + // No errors should occur - the isMounted check prevents state updates +}); 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..feda8f6db16 --- /dev/null +++ b/superset-frontend/src/dashboard/components/ComponentThemeProvider/index.tsx @@ -0,0 +1,237 @@ +/** + * 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 { + useState, + useEffect, + ReactNode, + useRef, + useMemo, + Component, +} from 'react'; +import { Theme } from '@apache-superset/core/ui'; + +interface ComponentThemeProviderProps { + /** The theme ID to apply (from component.meta.theme_id) */ + themeId?: number | null; + /** Child components to wrap with theme */ + children: ReactNode; + /** Optional fallback to render while loading */ + fallback?: ReactNode; +} + +/** + * Error boundary that catches useThemeContext errors when no ThemeProvider exists. + * Falls back to rendering children without theme wrapping. + */ +class ThemeContextErrorBoundary extends Component< + { children: ReactNode; fallbackChildren: ReactNode }, + { hasError: boolean } +> { + constructor(props: { children: ReactNode; fallbackChildren: ReactNode }) { + super(props); + this.state = { hasError: false }; + } + + static getDerivedStateFromError() { + return { hasError: true }; + } + + componentDidCatch(error: Error) { + // Only suppress "useThemeContext must be used within a ThemeProvider" errors + if ( + !error.message.includes( + 'useThemeContext must be used within a ThemeProvider', + ) + ) { + throw error; + } + } + + render() { + if (this.state.hasError) { + return <>{this.props.fallbackChildren}</>; + } + return <>{this.props.children}</>; + } +} + +/** + * Inner component that uses theme context - wrapped in error boundary + */ +const ComponentThemeProviderInner = ({ + themeId, + children, + fallback = null, +}: ComponentThemeProviderProps) => { + // Import useThemeContext dynamically to avoid issues when ThemeProvider not available + // eslint-disable-next-line @typescript-eslint/no-var-requires, global-require + const { useThemeContext } = require('src/theme/ThemeProvider'); + const { theme: parentTheme, createTheme } = useThemeContext(); + + const [theme, setTheme] = useState<Theme | null>(null); + const [isLoading, setIsLoading] = useState(false); + const [error, setError] = useState<Error | null>(null); + + // Use a ref for parent theme config to avoid infinite re-render loops. + // The parent theme config is captured once on mount and when themeId changes, + // preventing the theme loading effect from re-running due to parent theme + // reference changes. + const parentThemeConfigRef = useRef<ReturnType< + typeof parentTheme.toSerializedConfig + > | null>(null); + + // Capture parent theme config synchronously on render (before effect runs) + // This ensures we have the latest parent theme when loading the component theme + if (parentTheme?.toSerializedConfig) { + try { + parentThemeConfigRef.current = parentTheme.toSerializedConfig(); + } catch { + parentThemeConfigRef.current = null; + } + } + + useEffect(() => { + let isMounted = true; + + const loadTheme = async () => { + if (!themeId) { + setTheme(null); + return; + } + + setIsLoading(true); + setError(null); + + try { + // Create component theme merged with parent theme for proper inheritance + // Use the ref value to avoid dependency on parentThemeConfig object + const loadedTheme = await createTheme( + String(themeId), + parentThemeConfigRef.current, + ); + if (isMounted) { + setTheme(loadedTheme); + } + } catch (err) { + if (isMounted) { + setError( + err instanceof Error ? err : new Error('Failed to load theme'), + ); + // eslint-disable-next-line no-console + console.error(`Failed to load component theme ${themeId}:`, err); + } + } finally { + if (isMounted) { + setIsLoading(false); + } + } + }; + + loadTheme(); + + return () => { + isMounted = false; + }; + }, [themeId, createTheme]); + + // If no theme ID or error, just render children + if (!themeId || error) { + return <>{children}</>; + } + + // Show fallback while loading + if (isLoading && !theme) { + return <>{fallback || children}</>; + } + + // If theme loaded, wrap children with theme provider + if (theme) { + const ThemeProvider = theme.SupersetThemeProvider; + return <ThemeProvider>{children}</ThemeProvider>; + } + + // Fallback: render children without wrapper + return <>{children}</>; +}; + +/** + * A component-level theme provider for granular theming in dashboards. + * + * This component fetches and applies a specific theme to its children, + * enabling per-component theming within the dashboard hierarchy: + * Instance → Dashboard → Tab → Row/Column → Chart/Component + * + * **Hierarchical Inheritance**: When a component has a theme, it is merged + * with the parent component's theme. This allows each level to override + * specific tokens while inheriting others from its parent. + * + * When no themeId is provided, children are rendered without any theme + * wrapper, inheriting from the parent theme context. + * + * This component gracefully handles the case where no ThemeProvider exists + * in the component tree (e.g., in tests) by simply rendering children. + * + * @example + * ```tsx + * // Dashboard with Theme A + * <ComponentThemeProvider themeId={dashboardThemeId}> + * // Row with Theme B (inherits from Theme A, overrides specific tokens) + * <ComponentThemeProvider themeId={rowThemeId}> + * // Chart inherits Theme B's tokens + * <ChartContent /> + * </ComponentThemeProvider> + * </ComponentThemeProvider> + * ``` + */ +const ComponentThemeProvider = ({ + themeId, + children, + fallback = null, +}: ComponentThemeProviderProps) => { + // If no theme ID provided, skip all theme loading logic + if (!themeId) { + return <>{children}</>; + } + + return ( + <ThemeContextErrorBoundary fallbackChildren={children}> + <ComponentThemeProviderInner themeId={themeId} fallback={fallback}> + {children} + </ComponentThemeProviderInner> + </ThemeContextErrorBoundary> + ); +}; + +export default ComponentThemeProvider; + +/** + * Hook to get component theme info for debugging/display purposes + */ +export function useComponentTheme(themeId?: number | null) { + // For now, just generate a placeholder name synchronously + // In production, this could be extended to fetch the theme name from the API + return useMemo( + () => ({ + themeId, + themeName: themeId ? `Theme ${themeId}` : null, + hasTheme: !!themeId, + }), + [themeId], + ); +} diff --git a/superset-frontend/src/dashboard/components/SliceHeader/index.tsx b/superset-frontend/src/dashboard/components/SliceHeader/index.tsx index a0a6f702a4a..d8cc28fe58b 100644 --- a/superset-frontend/src/dashboard/components/SliceHeader/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeader/index.tsx @@ -166,6 +166,8 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>( width, height, exportPivotExcel = () => ({}), + onApplyTheme, + currentThemeId, }, ref, ) => { @@ -365,6 +367,8 @@ const SliceHeader = forwardRef<HTMLDivElement, SliceHeaderProps>( exploreUrl={exploreUrl} crossFiltersEnabled={isCrossFiltersEnabled} exportPivotExcel={exportPivotExcel} + onApplyTheme={onApplyTheme} + currentThemeId={currentThemeId} /> )} </> diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx index 6e73bec6f2b..36a629d6674 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx @@ -23,6 +23,7 @@ import { useState, useRef, RefObject, + useCallback, } from 'react'; import { RouteComponentProps, useHistory } from 'react-router-dom'; @@ -60,6 +61,7 @@ import { usePermissions } from 'src/hooks/usePermissions'; import { useDatasetDrillInfo } from 'src/hooks/apiResources/datasets'; import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; import { useCrossFiltersScopingModal } from '../nativeFilters/FilterBar/CrossFilters/ScopingModal/useCrossFiltersScopingModal'; +import ThemeSelectorModal from '../menu/ThemeSelectorModal'; import { ViewResultsModalTrigger } from './ViewResultsModalTrigger'; const RefreshTooltip = styled.div` @@ -139,6 +141,10 @@ export interface SliceHeaderControlsProps { supersetCanCSV?: boolean; crossFiltersEnabled?: boolean; + + // Theme-related props + currentThemeId?: number | null; + onApplyTheme?: (themeId: number | null) => void; } type SliceHeaderControlsPropsWithRouter = SliceHeaderControlsProps & RouteComponentProps; @@ -156,6 +162,7 @@ const SliceHeaderControls = ( const [drillModalIsOpen, setDrillModalIsOpen] = useState(false); // setting openKeys undefined falls back to uncontrolled behaviour const [isDropdownVisible, setIsDropdownVisible] = useState(false); + const [isThemeSelectorOpen, setIsThemeSelectorOpen] = useState(false); const [openScopingModal, scopingModal] = useCrossFiltersScopingModal( props.slice.slice_id, ); @@ -298,12 +305,28 @@ const SliceHeaderControls = ( } break; } + case MenuKeys.ApplyTheme: { + setIsThemeSelectorOpen(true); + break; + } default: break; } setIsDropdownVisible(false); }; + const handleCloseThemeSelector = useCallback(() => { + setIsThemeSelectorOpen(false); + }, []); + + const handleApplyTheme = useCallback( + (themeId: number | null) => { + props.onApplyTheme?.(themeId); + handleCloseThemeSelector(); + }, + [props.onApplyTheme, handleCloseThemeSelector], + ); + const { componentId, dashboardId, @@ -414,7 +437,15 @@ const SliceHeaderControls = ( }); } - if (canExplore || canEditCrossFilters) { + // Add theme selector if the callback is provided + if (props.onApplyTheme) { + newMenuItems.push({ + key: MenuKeys.ApplyTheme, + label: t('Apply theme'), + }); + } + + if (canExplore || canEditCrossFilters || props.onApplyTheme) { newMenuItems.push({ type: 'divider' }); } @@ -605,6 +636,17 @@ const SliceHeaderControls = ( /> {canEditCrossFilters && scopingModal} + + {props.onApplyTheme && ( + <ThemeSelectorModal + show={isThemeSelectorOpen} + onHide={handleCloseThemeSelector} + onApply={handleApplyTheme} + currentThemeId={props.currentThemeId ?? null} + componentId={componentId} + componentType="Chart" + /> + )} </> ); }; diff --git a/superset-frontend/src/dashboard/components/SliceHeaderControls/types.ts b/superset-frontend/src/dashboard/components/SliceHeaderControls/types.ts index 62d4e94e6bc..dffe24d3890 100644 --- a/superset-frontend/src/dashboard/components/SliceHeaderControls/types.ts +++ b/superset-frontend/src/dashboard/components/SliceHeaderControls/types.ts @@ -60,4 +60,8 @@ export interface SliceHeaderControlsProps { supersetCanCSV?: boolean; crossFiltersEnabled?: boolean; + + // Theme-related props + currentThemeId?: number | null; + onApplyTheme?: (themeId: number | null) => void; } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx index 9a429f5d223..a74bac265f8 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.jsx @@ -87,6 +87,8 @@ const propTypes = { isFullSize: PropTypes.bool, extraControls: PropTypes.object, isInView: PropTypes.bool, + onApplyTheme: PropTypes.func, + currentThemeId: PropTypes.number, }; const RESIZE_TIMEOUT = 500; @@ -665,6 +667,8 @@ const Chart = props => { width={width} height={getHeaderHeight()} exportPivotExcel={exportPivotExcel} + onApplyTheme={props.onApplyTheme} + currentThemeId={props.currentThemeId} /> {/* diff --git a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx index 7d68c8ceaec..5dd0b9c81b6 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx @@ -25,6 +25,7 @@ import { css, useTheme } from '@apache-superset/core/ui'; import { LayoutItem, RootState } from 'src/dashboard/types'; import AnchorLink from 'src/dashboard/components/AnchorLink'; import Chart from 'src/dashboard/components/gridComponents/Chart'; +import ComponentThemeProvider from 'src/dashboard/components/ComponentThemeProvider'; import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; import { Draggable } from 'src/dashboard/components/dnd/DragDroppable'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; @@ -231,6 +232,38 @@ const ChartHolder = ({ setFullSizeChartId(isFullSize ? null : chartId); }, [chartId, isFullSize, setFullSizeChartId]); + const handleApplyTheme = useCallback( + (themeId: number | null) => { + // Use getComponentById to get the latest component state + // This avoids having `component` in dependencies which causes re-renders + const currentComponent = getComponentById(component.id); + if (!currentComponent) return; + + if (themeId !== null) { + updateComponents({ + [component.id]: { + ...currentComponent, + meta: { + ...currentComponent.meta, + theme_id: themeId, + }, + }, + }); + } else { + // eslint-disable-next-line @typescript-eslint/no-unused-vars, camelcase + const { theme_id, ...metaWithoutTheme } = + currentComponent.meta as Record<string, unknown>; + updateComponents({ + [component.id]: { + ...currentComponent, + meta: metaWithoutTheme, + }, + }); + } + }, + [component.id, getComponentById, updateComponents], + ); + const handleExtraControl = useCallback((name: string, value: unknown) => { setExtraControls(current => ({ ...current, @@ -238,6 +271,7 @@ const ChartHolder = ({ })); }, []); + // eslint-disable-next-line react-hooks/exhaustive-deps -- individual meta properties are tracked const renderChild = useCallback( ({ dragSourceRef }) => ( <ResizableContainer @@ -283,23 +317,39 @@ const ChartHolder = ({ }`} </style> )} - <Chart - componentId={component.id} - id={component.meta.chartId} - dashboardId={dashboardId} - width={chartWidth} - height={chartHeight} - sliceName={ - component.meta.sliceNameOverride || component.meta.sliceName || '' + <ComponentThemeProvider + themeId={ + (component.meta as Record<string, unknown>).theme_id as + | number + | undefined } - updateSliceName={handleUpdateSliceName} - isComponentVisible={isComponentVisible} - handleToggleFullSize={handleToggleFullSize} - isFullSize={isFullSize} - setControlValue={handleExtraControl} - extraControls={extraControls} - isInView={isInView} - /> + > + <Chart + componentId={component.id} + id={component.meta.chartId} + dashboardId={dashboardId} + width={chartWidth} + height={chartHeight} + sliceName={ + component.meta.sliceNameOverride || + component.meta.sliceName || + '' + } + updateSliceName={handleUpdateSliceName} + isComponentVisible={isComponentVisible} + handleToggleFullSize={handleToggleFullSize} + isFullSize={isFullSize} + setControlValue={handleExtraControl} + extraControls={extraControls} + isInView={isInView} + onApplyTheme={handleApplyTheme} + currentThemeId={ + (component.meta as Record<string, unknown>).theme_id as + | number + | null + } + /> + </ComponentThemeProvider> {editMode && ( <HoverMenu position="top"> <div data-test="dashboard-delete-component-button"> @@ -340,6 +390,7 @@ const ChartHolder = ({ extraControls, isInView, handleDeleteComponent, + handleApplyTheme, ], ); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Column/Column.jsx b/superset-frontend/src/dashboard/components/gridComponents/Column/Column.jsx index d8047d08c3d..1fa8770a1a8 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Column/Column.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Column/Column.jsx @@ -16,22 +16,31 @@ * specific language governing permissions and limitations * under the License. */ -import { Fragment, useCallback, useState, useMemo, memo } from 'react'; +import { + Fragment, + useCallback, + useState, + useMemo, + useRef, + useEffect, + memo, +} from 'react'; import PropTypes from 'prop-types'; import cx from 'classnames'; import { t, css, styled } from '@apache-superset/core/ui'; -import { Icons } from '@superset-ui/core/components/Icons'; +import ComponentThemeProvider from 'src/dashboard/components/ComponentThemeProvider'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; -import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; import { Draggable, Droppable, } from 'src/dashboard/components/dnd/DragDroppable'; import DragHandle from 'src/dashboard/components/dnd/DragHandle'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; -import IconButton from 'src/dashboard/components/IconButton'; +import ComponentHeaderControls, { + ComponentMenuKeys, +} from 'src/dashboard/components/menu/ComponentHeaderControls'; +import ThemeSelectorModal from 'src/dashboard/components/menu/ThemeSelectorModal'; import ResizableContainer from 'src/dashboard/components/resizable/ResizableContainer'; -import BackgroundStyleDropdown from 'src/dashboard/components/menu/BackgroundStyleDropdown'; import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu'; import backgroundStyleOptions from 'src/dashboard/util/backgroundStyleOptions'; import { componentShape } from 'src/dashboard/util/propShapes'; @@ -142,6 +151,13 @@ const Column = props => { } = props; const [isFocused, setIsFocused] = useState(false); + const [isThemeSelectorOpen, setIsThemeSelectorOpen] = useState(false); + const columnComponentRef = useRef(columnComponent); + + // Keep ref updated with latest columnComponent to avoid stale closures + useEffect(() => { + columnComponentRef.current = columnComponent; + }, [columnComponent]); const handleDeleteComponent = useCallback(() => { deleteComponent(id, parentId); @@ -151,6 +167,11 @@ const Column = props => { setIsFocused(Boolean(nextFocus)); }, []); + const backgroundStyle = backgroundStyleOptions.find( + opt => + opt.value === (columnComponent.meta.background || BACKGROUND_TRANSPARENT), + ); + const handleChangeBackground = useCallback( nextValue => { const metaKey = 'background'; @@ -169,16 +190,102 @@ const Column = props => { [columnComponent, updateComponents], ); + const handleOpenThemeSelector = useCallback(() => { + setIsThemeSelectorOpen(true); + }, []); + + const handleCloseThemeSelector = useCallback(() => { + setIsThemeSelectorOpen(false); + }, []); + + const handleApplyTheme = useCallback( + themeId => { + // Use ref to get latest component state, avoiding stale closures + const currentComponent = columnComponentRef.current; + if (themeId !== null) { + updateComponents({ + [currentComponent.id]: { + ...currentComponent, + meta: { + ...currentComponent.meta, + theme_id: themeId, + }, + }, + }); + } else { + // Clear theme - omit theme_id from meta + // eslint-disable-next-line @typescript-eslint/no-unused-vars, camelcase + const { theme_id, ...metaWithoutTheme } = currentComponent.meta || {}; + updateComponents({ + [currentComponent.id]: { + ...currentComponent, + meta: metaWithoutTheme, + }, + }); + } + handleCloseThemeSelector(); + }, + [updateComponents, handleCloseThemeSelector], + ); + + const handleMenuClick = useCallback( + key => { + switch (key) { + case ComponentMenuKeys.BackgroundStyle: + // Toggle between transparent and solid + handleChangeBackground( + backgroundStyle.value === BACKGROUND_TRANSPARENT + ? backgroundStyleOptions[1].value + : BACKGROUND_TRANSPARENT, + ); + break; + case ComponentMenuKeys.ApplyTheme: + handleOpenThemeSelector(); + break; + case ComponentMenuKeys.Delete: + handleDeleteComponent(); + break; + default: + break; + } + }, + [ + backgroundStyle.value, + handleChangeBackground, + handleDeleteComponent, + handleOpenThemeSelector, + ], + ); + + const menuItems = useMemo( + () => [ + { + key: ComponentMenuKeys.BackgroundStyle, + label: + backgroundStyle.value === BACKGROUND_TRANSPARENT + ? t('Background: Transparent') + : t('Background: Solid'), + }, + { type: 'divider' }, + { + key: ComponentMenuKeys.ApplyTheme, + label: t('Apply theme'), + }, + { type: 'divider' }, + { + key: ComponentMenuKeys.Delete, + label: t('Delete'), + danger: true, + }, + ], + [backgroundStyle.value], + ); + const columnItems = useMemo( () => columnComponent.children || [], [columnComponent.children], ); - const backgroundStyle = backgroundStyleOptions.find( - opt => - opt.value === (columnComponent.meta.background || BACKGROUND_TRANSPARENT), - ); - const renderChild = useCallback( ({ dragSourceRef }) => ( <ResizableContainer @@ -200,33 +307,26 @@ const Column = props => { isFocused={isFocused} onChangeFocus={handleChangeFocus} disableClick - menuItems={[ - <BackgroundStyleDropdown - id={`${columnComponent.id}-background`} - value={columnComponent.meta.background} - onChange={handleChangeBackground} - />, - ]} + menuItems={[]} editMode={editMode} > {editMode && ( <HoverMenu innerRef={dragSourceRef} position="top"> <DragHandle position="top" /> - <DeleteComponentButton - iconSize="m" - onDelete={handleDeleteComponent} - /> - <IconButton - onClick={handleChangeFocus} - icon={<Icons.SettingOutlined iconSize="m" />} + <ComponentHeaderControls + componentId={columnComponent.id} + menuItems={menuItems} + onMenuClick={handleMenuClick} + editMode={editMode} /> </HoverMenu> )} - <ColumnStyles - className={cx('grid-column', backgroundStyle.className)} - editMode={editMode} - > - {editMode && ( + <ComponentThemeProvider themeId={columnComponent.meta?.theme_id}> + <ColumnStyles + className={cx('grid-column', backgroundStyle.className)} + editMode={editMode} + > + {editMode && ( <Droppable component={columnComponent} parentComponent={columnComponent} @@ -293,7 +393,8 @@ const Column = props => { </Fragment> )) )} - </ColumnStyles> + </ColumnStyles> + </ComponentThemeProvider> </WithPopoverMenu> </ResizableContainer> ), @@ -305,12 +406,12 @@ const Column = props => { columnWidth, depth, editMode, - handleChangeBackground, handleChangeFocus, handleComponentDrop, - handleDeleteComponent, + handleMenuClick, isComponentVisible, isFocused, + menuItems, minColumnWidth, onChangeTab, onResize, @@ -320,17 +421,27 @@ const Column = props => { ); return ( - <Draggable - component={columnComponent} - parentComponent={parentComponent} - orientation="column" - index={index} - depth={depth} - onDrop={handleComponentDrop} - editMode={editMode} - > - {renderChild} - </Draggable> + <> + <Draggable + component={columnComponent} + parentComponent={parentComponent} + orientation="column" + index={index} + depth={depth} + onDrop={handleComponentDrop} + editMode={editMode} + > + {renderChild} + </Draggable> + <ThemeSelectorModal + show={isThemeSelectorOpen} + onHide={handleCloseThemeSelector} + onApply={handleApplyTheme} + currentThemeId={columnComponent.meta?.theme_id || null} + componentId={columnComponent.id} + componentType="Column" + /> + </> ); }; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Column/Column.test.jsx b/superset-frontend/src/dashboard/components/gridComponents/Column/Column.test.jsx index e67be2c20d5..5c9188b1b5f 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Column/Column.test.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Column/Column.test.jsx @@ -16,10 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { fireEvent, render } from 'spec/helpers/testing-library'; - -import BackgroundStyleDropdown from 'src/dashboard/components/menu/BackgroundStyleDropdown'; -import IconButton from 'src/dashboard/components/IconButton'; +import { fireEvent, render, screen, waitFor } from 'spec/helpers/testing-library'; import { getMockStore } from 'spec/fixtures/mockStore'; import { dashboardLayout as mockLayout } from 'spec/fixtures/mockDashboardLayout'; @@ -50,19 +47,6 @@ jest.mock( () => ({ children }) => <div data-test="mock-with-popover-menu">{children}</div>, ); -jest.mock( - 'src/dashboard/components/DeleteComponentButton', - () => - ({ onDelete }) => ( - <button - type="button" - data-test="mock-delete-component-button" - onClick={onDelete} - > - Delete - </button> - ), -); const columnWithoutChildren = { ...mockLayout.present.COLUMN_ID, @@ -109,12 +93,11 @@ test('should render a Draggable', () => { expect(queryByTestId('mock-droppable')).not.toBeInTheDocument(); }); -test('should skip rendering HoverMenu and DeleteComponentButton when not in editMode', () => { - const { container, queryByTestId } = setup({ +test('should skip rendering HoverMenu when not in editMode', () => { + const { container } = setup({ component: columnWithoutChildren, }); expect(container.querySelector('.hover-menu')).not.toBeInTheDocument(); - expect(queryByTestId('mock-delete-component-button')).not.toBeInTheDocument(); }); test('should render a WithPopoverMenu', () => { @@ -147,33 +130,35 @@ test('should render a HoverMenu in editMode', () => { ); }); -test('should render a DeleteComponentButton in editMode', () => { - // we cannot set props on the Row because of the WithDragDropContext wrapper - const { getByTestId } = setup({ +test('should render ComponentHeaderControls in editMode', () => { + const { container } = setup({ component: columnWithoutChildren, editMode: true, }); - expect(getByTestId('mock-delete-component-button')).toBeInTheDocument(); + // ComponentHeaderControls renders a button with "More Options" aria-label + expect( + container.querySelector('[aria-label="More Options"]'), + ).toBeInTheDocument(); }); -test.skip('should render a BackgroundStyleDropdown when focused', () => { - let wrapper = setup({ component: columnWithoutChildren }); - expect(wrapper.find(BackgroundStyleDropdown)).toBeFalsy(); +test('should call deleteComponent when Delete menu item is clicked', async () => { + const deleteComponent = jest.fn(); + const { container } = setup({ + editMode: true, + deleteComponent, + component: columnWithoutChildren, + }); - // we cannot set props on the Row because of the WithDragDropContext wrapper - wrapper = setup({ component: columnWithoutChildren, editMode: true }); - wrapper - .find(IconButton) - .at(1) // first one is delete button - .simulate('click'); + // Click the "More Options" menu button + const menuButton = container.querySelector('[aria-label="More Options"]'); + fireEvent.click(menuButton); - expect(wrapper.find(BackgroundStyleDropdown)).toBeTruthy(); -}); + // Wait for menu to open and click Delete + await waitFor(() => { + const deleteOption = screen.getByText('Delete'); + fireEvent.click(deleteOption); + }); -test('should call deleteComponent when deleted', () => { - const deleteComponent = jest.fn(); - const { getByTestId } = setup({ editMode: true, deleteComponent }); - fireEvent.click(getByTestId('mock-delete-component-button')); expect(deleteComponent).toHaveBeenCalledTimes(1); }); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx b/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx index 4e34d94fd81..76fe3570dbd 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Markdown/Markdown.jsx @@ -28,6 +28,7 @@ import { EditorHost } from 'src/core/editors'; import ComponentHeaderControls, { ComponentMenuKeys, } from 'src/dashboard/components/menu/ComponentHeaderControls'; +import ComponentThemeProvider from 'src/dashboard/components/ComponentThemeProvider'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; import ThemeSelectorModal from 'src/dashboard/components/menu/ThemeSelectorModal'; import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils'; @@ -239,16 +240,17 @@ class Markdown extends PureComponent { } handleChangeEditorMode(mode) { - const nextState = { - ...this.state, - editorMode: mode, - }; if (mode === 'preview') { this.updateMarkdownContent(); - nextState.hasError = false; } - this.setState(nextState); + // Use functional setState to avoid overwriting concurrent state updates + // (e.g., isThemeSelectorOpen being set by handleOpenThemeSelector) + this.setState(prevState => ({ + ...prevState, + editorMode: mode, + ...(mode === 'preview' ? { hasError: false } : {}), + })); } updateMarkdownContent() { @@ -463,51 +465,53 @@ class Markdown extends PureComponent { menuItems={[]} editMode={editMode} > - <MarkdownStyles - data-test="dashboard-markdown-editor" - className={cx( - 'dashboard-markdown', - isEditing && 'dashboard-markdown--editing', - )} - id={component.id} - > - <ResizableContainer + <ComponentThemeProvider themeId={component.meta.theme_id}> + <MarkdownStyles + data-test="dashboard-markdown-editor" + className={cx( + 'dashboard-markdown', + isEditing && 'dashboard-markdown--editing', + )} id={component.id} - adjustableWidth={parentComponent.type === ROW_TYPE} - adjustableHeight - widthStep={columnWidth} - widthMultiple={widthMultiple} - heightStep={GRID_BASE_UNIT} - heightMultiple={component.meta.height} - minWidthMultiple={GRID_MIN_COLUMN_COUNT} - minHeightMultiple={GRID_MIN_ROW_UNITS} - maxWidthMultiple={availableColumnCount + widthMultiple} - onResizeStart={this.handleResizeStart} - onResize={onResize} - onResizeStop={onResizeStop} - editMode={isFocused ? false : editMode} > - <div - ref={dragSourceRef} - className="dashboard-component dashboard-component-chart-holder" - data-test="dashboard-component-chart-holder" + <ResizableContainer + id={component.id} + adjustableWidth={parentComponent.type === ROW_TYPE} + adjustableHeight + widthStep={columnWidth} + widthMultiple={widthMultiple} + heightStep={GRID_BASE_UNIT} + heightMultiple={component.meta.height} + minWidthMultiple={GRID_MIN_COLUMN_COUNT} + minHeightMultiple={GRID_MIN_ROW_UNITS} + maxWidthMultiple={availableColumnCount + widthMultiple} + onResizeStart={this.handleResizeStart} + onResize={onResize} + onResizeStop={onResizeStop} + editMode={isFocused ? false : editMode} > - {editMode && ( - <HoverMenu position="top"> - <ComponentHeaderControls - componentId={component.id} - menuItems={this.getMenuItems()} - onMenuClick={this.handleMenuClick} - editMode={editMode} - /> - </HoverMenu> - )} - {editMode && isEditing - ? this.renderEditMode() - : this.renderPreviewMode()} - </div> - </ResizableContainer> - </MarkdownStyles> + <div + ref={dragSourceRef} + className="dashboard-component dashboard-component-chart-holder" + data-test="dashboard-component-chart-holder" + > + {editMode && ( + <HoverMenu position="top"> + <ComponentHeaderControls + componentId={component.id} + menuItems={this.getMenuItems()} + onMenuClick={this.handleMenuClick} + editMode={editMode} + /> + </HoverMenu> + )} + {editMode && isEditing + ? this.renderEditMode() + : this.renderPreviewMode()} + </div> + </ResizableContainer> + </MarkdownStyles> + </ComponentThemeProvider> </WithPopoverMenu> )} </Draggable> diff --git a/superset-frontend/src/dashboard/components/gridComponents/Row/Row.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Row/Row.test.tsx index 0d1cf099f15..b8341d855ea 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Row/Row.test.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Row/Row.test.tsx @@ -22,6 +22,7 @@ import { render, RenderResult, screen, + waitFor, } from 'spec/helpers/testing-library'; import { DASHBOARD_GRID_ID } from 'src/dashboard/util/constants'; @@ -90,18 +91,6 @@ jest.mock('src/dashboard/components/menu/WithPopoverMenu', () => { ); }); -jest.mock('src/dashboard/components/DeleteComponentButton', () => { - return ({ onDelete }: { onDelete: () => void }) => ( - <button - type="button" - data-test="mock-delete-component-button" - onClick={onDelete} - > - Delete - </button> - ); -}); - const rowWithoutChildren = { ...mockLayout.present.ROW_ID, children: [], @@ -174,12 +163,11 @@ test('should render a Draggable', () => { expect(queryByTestId('mock-droppable')).not.toBeInTheDocument(); }); -test('should skip rendering HoverMenu and DeleteComponentButton when not in editMode', () => { - const { container, queryByTestId } = setup({ +test('should skip rendering HoverMenu when not in editMode', () => { + const { container } = setup({ component: rowWithoutChildren, }); expect(container.querySelector('.hover-menu')).not.toBeInTheDocument(); - expect(queryByTestId('mock-delete-component-button')).not.toBeInTheDocument(); }); test('should render a WithPopoverMenu', () => { @@ -205,31 +193,35 @@ test('should render a HoverMenu in editMode', () => { ); }); -test('should render a DeleteComponentButton in editMode', () => { - const { getByTestId } = setup({ +test('should render ComponentHeaderControls in editMode', () => { + const { container } = setup({ component: rowWithoutChildren, editMode: true, }); - expect(getByTestId('mock-delete-component-button')).toBeInTheDocument(); + // ComponentHeaderControls renders a button with "More Options" aria-label + expect( + container.querySelector('[aria-label="More Options"]'), + ).toBeInTheDocument(); }); -test.skip('should render a BackgroundStyleDropdown when focused', () => { - let { rerender } = setup({ component: rowWithoutChildren }); - expect(screen.queryByTestId('background-style-dropdown')).toBeFalsy(); +test('should call deleteComponent when Delete menu item is clicked', async () => { + const deleteComponent = jest.fn(); + const { container } = setup({ + editMode: true, + deleteComponent, + component: rowWithoutChildren, + }); - // we cannot set props on the Row because of the WithDragDropContext wrapper - rerender(<Row {...props} component={rowWithoutChildren} editMode={true} />); - const buttons = screen.getAllByRole('button'); - const settingsButton = buttons[1]; - fireEvent.click(settingsButton); + // Click the "More Options" menu button + const menuButton = container.querySelector('[aria-label="More Options"]'); + fireEvent.click(menuButton!); - expect(screen.queryByTestId('background-style-dropdown')).toBeTruthy(); -}); + // Wait for menu to open and click Delete + await waitFor(() => { + const deleteOption = screen.getByText('Delete'); + fireEvent.click(deleteOption); + }); -test('should call deleteComponent when deleted', () => { - const deleteComponent = jest.fn(); - const { getByTestId } = setup({ editMode: true, deleteComponent }); - fireEvent.click(getByTestId('mock-delete-component-button')); expect(deleteComponent).toHaveBeenCalledTimes(1); }); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx b/superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx index 28a274c49bb..d7f1a2c9184 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Row/Row.tsx @@ -30,17 +30,19 @@ import cx from 'classnames'; import { t } from '@apache-superset/core'; import { FeatureFlag, isFeatureEnabled, JsonObject } from '@superset-ui/core'; import { css, styled, SupersetTheme } from '@apache-superset/core/ui'; -import { Icons, Constants } from '@superset-ui/core/components'; +import { Constants } from '@superset-ui/core/components'; import { Draggable, Droppable, } from 'src/dashboard/components/dnd/DragDroppable'; import DragHandle from 'src/dashboard/components/dnd/DragHandle'; +import ComponentThemeProvider from 'src/dashboard/components/ComponentThemeProvider'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; -import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; import HoverMenu from 'src/dashboard/components/menu/HoverMenu'; -import IconButton from 'src/dashboard/components/IconButton'; -import BackgroundStyleDropdown from 'src/dashboard/components/menu/BackgroundStyleDropdown'; +import ComponentHeaderControls, { + ComponentMenuKeys, +} from 'src/dashboard/components/menu/ComponentHeaderControls'; +import ThemeSelectorModal from 'src/dashboard/components/menu/ThemeSelectorModal'; import WithPopoverMenu from 'src/dashboard/components/menu/WithPopoverMenu'; import backgroundStyleOptions from 'src/dashboard/util/backgroundStyleOptions'; import { BACKGROUND_TRANSPARENT } from 'src/dashboard/util/constants'; @@ -158,13 +160,20 @@ const Row = memo((props: RowProps) => { const [isInView, setIsInView] = useState(false); const [hoverMenuHovered, setHoverMenuHovered] = useState(false); const [containerHeight, setContainerHeight] = useState<number | null>(null); + const [isThemeSelectorOpen, setIsThemeSelectorOpen] = useState(false); const containerRef = useRef<HTMLDivElement | null>(null); const isComponentVisibleRef = useRef(isComponentVisible); + const rowComponentRef = useRef(rowComponent); useEffect(() => { isComponentVisibleRef.current = isComponentVisible; }, [isComponentVisible]); + // Keep ref updated with latest rowComponent to avoid stale closures + useEffect(() => { + rowComponentRef.current = rowComponent; + }, [rowComponent]); + // if chart not rendered - render it if it's less than 1 view height away from current viewport // if chart rendered - remove it if it's more than 4 view heights away from current viewport useEffect(() => { @@ -234,6 +243,12 @@ const Row = memo((props: RowProps) => { setIsFocused(Boolean(nextFocus)); }, []); + const backgroundStyle = + backgroundStyleOptions.find( + opt => + opt.value === (rowComponent.meta?.background ?? BACKGROUND_TRANSPARENT), + ) ?? backgroundStyleOptions[0]; + const handleChangeBackground = useCallback( (nextValue: string) => { const metaKey = 'background'; @@ -256,6 +271,97 @@ const Row = memo((props: RowProps) => { deleteComponent(rowComponent.id as string, parentId); }, [deleteComponent, rowComponent, parentId]); + const handleOpenThemeSelector = useCallback(() => { + setIsThemeSelectorOpen(true); + }, []); + + const handleCloseThemeSelector = useCallback(() => { + setIsThemeSelectorOpen(false); + }, []); + + const handleApplyTheme = useCallback( + (themeId: number | null) => { + // Use ref to get latest component state, avoiding stale closures + const currentComponent = rowComponentRef.current; + if (themeId !== null) { + updateComponents({ + [currentComponent.id as string]: { + ...currentComponent, + meta: { + ...currentComponent.meta, + theme_id: themeId, + }, + }, + }); + } else { + // Clear theme - omit theme_id from meta + // eslint-disable-next-line @typescript-eslint/no-unused-vars, camelcase + const { theme_id, ...metaWithoutTheme } = currentComponent.meta || {}; + updateComponents({ + [currentComponent.id as string]: { + ...currentComponent, + meta: metaWithoutTheme, + }, + }); + } + handleCloseThemeSelector(); + }, + [updateComponents, handleCloseThemeSelector], + ); + + const handleMenuClick = useCallback( + (key: string) => { + switch (key) { + case ComponentMenuKeys.BackgroundStyle: + // Toggle between transparent and solid + handleChangeBackground( + backgroundStyle.value === BACKGROUND_TRANSPARENT + ? backgroundStyleOptions[1].value + : BACKGROUND_TRANSPARENT, + ); + break; + case ComponentMenuKeys.ApplyTheme: + handleOpenThemeSelector(); + break; + case ComponentMenuKeys.Delete: + handleDeleteComponent(); + break; + default: + break; + } + }, + [ + backgroundStyle.value, + handleChangeBackground, + handleDeleteComponent, + handleOpenThemeSelector, + ], + ); + + const menuItems = useMemo( + () => [ + { + key: ComponentMenuKeys.BackgroundStyle, + label: + backgroundStyle.value === BACKGROUND_TRANSPARENT + ? t('Background: Transparent') + : t('Background: Solid'), + }, + { type: 'divider' as const }, + { + key: ComponentMenuKeys.ApplyTheme, + label: t('Apply theme'), + }, + { type: 'divider' as const }, + { + key: ComponentMenuKeys.Delete, + label: t('Delete'), + danger: true, + }, + ], + [backgroundStyle.value], + ); + const handleMenuHover = useCallback((hover: { isHovered: boolean }) => { setHoverMenuHovered(hover.isHovered); }, []); @@ -268,12 +374,6 @@ const Row = memo((props: RowProps) => { [rowComponent.children], ); - const backgroundStyle = - backgroundStyleOptions.find( - opt => - opt.value === (rowComponent.meta?.background ?? BACKGROUND_TRANSPARENT), - ) ?? backgroundStyleOptions[0]; - const remainColumnCount = availableColumnCount - occupiedColumnCount; const renderChild = useCallback( ({ dragSourceRef }: { dragSourceRef: RefObject<HTMLDivElement> }) => ( @@ -281,13 +381,7 @@ const Row = memo((props: RowProps) => { isFocused={isFocused} onChangeFocus={handleChangeFocus} disableClick - menuItems={[ - <BackgroundStyleDropdown - id={`${rowComponent.id}-background`} - value={backgroundStyle.value} - onChange={handleChangeBackground} - />, - ]} + menuItems={[]} editMode={editMode} > {editMode && ( @@ -297,24 +391,28 @@ const Row = memo((props: RowProps) => { position="left" > <DragHandle position="left" /> - <DeleteComponentButton onDelete={handleDeleteComponent} /> - <IconButton - onClick={() => handleChangeFocus(true)} - icon={<Icons.SettingOutlined iconSize="l" />} + <ComponentHeaderControls + componentId={rowComponent.id as string} + menuItems={menuItems} + onMenuClick={handleMenuClick} + editMode={editMode} /> </HoverMenu> )} - <GridRow - className={cx( - 'grid-row', - rowItems.length === 0 && 'grid-row--empty', - hoverMenuHovered && 'grid-row--hovered', - backgroundStyle.className, - )} - data-test={`grid-row-${backgroundStyle.className}`} - ref={containerRef} - editMode={editMode} + <ComponentThemeProvider + themeId={rowComponent.meta?.theme_id as number | undefined} > + <GridRow + className={cx( + 'grid-row', + rowItems.length === 0 && 'grid-row--empty', + hoverMenuHovered && 'grid-row--hovered', + backgroundStyle.className, + )} + data-test={`grid-row-${backgroundStyle.className}`} + ref={containerRef} + editMode={editMode} + > {editMode && ( <Droppable {...(rowItems.length === 0 @@ -399,25 +497,25 @@ const Row = memo((props: RowProps) => { )} </Fragment> ))} - </GridRow> + </GridRow> + </ComponentThemeProvider> </WithPopoverMenu> ), [ backgroundStyle.className, - backgroundStyle.value, columnWidth, containerHeight, depth, editMode, - handleChangeBackground, handleChangeFocus, handleComponentDrop, - handleDeleteComponent, + handleMenuClick, handleMenuHover, hoverMenuHovered, isComponentVisible, isFocused, isInView, + menuItems, onChangeTab, onResize, onResizeStart, @@ -429,17 +527,27 @@ const Row = memo((props: RowProps) => { ); return ( - <Draggable - component={rowComponent} - parentComponent={parentComponent} - orientation="row" - index={index} - depth={depth} - onDrop={handleComponentDrop} - editMode={editMode} - > - {renderChild} - </Draggable> + <> + <Draggable + component={rowComponent} + parentComponent={parentComponent} + orientation="row" + index={index} + depth={depth} + onDrop={handleComponentDrop} + editMode={editMode} + > + {renderChild} + </Draggable> + <ThemeSelectorModal + show={isThemeSelectorOpen} + onHide={handleCloseThemeSelector} + onApply={handleApplyTheme} + currentThemeId={(rowComponent.meta?.theme_id as number) || null} + componentId={rowComponent.id as string} + componentType="Row" + /> + </> ); }); diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.jsx index cc087af6ef9..3dd40fbe33f 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.jsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useMemo, useState, memo } from 'react'; +import { useCallback, useEffect, useMemo, useState, memo, useRef } from 'react'; import PropTypes from 'prop-types'; import { usePrevious } from '@superset-ui/core'; import { t, useTheme, styled } from '@apache-superset/core/ui'; @@ -25,8 +25,10 @@ import { Icons } from '@superset-ui/core/components/Icons'; import { LOG_ACTIONS_SELECT_DASHBOARD_TAB } from 'src/logger/LogUtils'; import { Modal } from '@superset-ui/core/components'; import { DROP_LEFT, DROP_RIGHT } from 'src/dashboard/util/getDropPosition'; +import ComponentThemeProvider from '../../ComponentThemeProvider'; import { Draggable } from '../../dnd/DragDroppable'; import DashboardComponent from '../../../containers/DashboardComponent'; +import ThemeSelectorModal from '../../menu/ThemeSelectorModal'; import findTabIndexByComponentId from '../../../util/findTabIndexByComponentId'; import getDirectPathToTabIndex from '../../../util/getDirectPathToTabIndex'; import getLeafComponentIdFromPath from '../../../util/getLeafComponentIdFromPath'; @@ -135,11 +137,18 @@ const Tabs = props => { const [dragOverTabIndex, setDragOverTabIndex] = useState(null); const [tabToDelete, setTabToDelete] = useState(null); const [isEditingTabTitle, setIsEditingTabTitle] = useState(false); + const [isThemeSelectorOpen, setIsThemeSelectorOpen] = useState(false); + const tabsComponentRef = useRef(props.component); const prevActiveKey = usePrevious(activeKey); const prevDashboardId = usePrevious(props.dashboardId); const prevDirectPathToChild = usePrevious(directPathToChild); const prevTabIds = usePrevious(props.component.children); + // Keep ref updated with latest component to avoid stale closures + useEffect(() => { + tabsComponentRef.current = props.component; + }, [props.component]); + useEffect(() => { if (prevActiveKey) { props.setActiveTab(activeKey, prevActiveKey); @@ -335,6 +344,44 @@ const Tabs = props => { setIsEditingTabTitle(isEditing); }, []); + const handleOpenThemeSelector = useCallback(() => { + setIsThemeSelectorOpen(true); + }, []); + + const handleCloseThemeSelector = useCallback(() => { + setIsThemeSelectorOpen(false); + }, []); + + const handleApplyTheme = useCallback( + themeId => { + // Use ref to get latest component state, avoiding stale closures + const currentComponent = tabsComponentRef.current; + if (themeId !== null) { + props.updateComponents({ + [currentComponent.id]: { + ...currentComponent, + meta: { + ...currentComponent.meta, + theme_id: themeId, + }, + }, + }); + } else { + // Clear theme - omit theme_id from meta + // eslint-disable-next-line @typescript-eslint/no-unused-vars, camelcase + const { theme_id, ...metaWithoutTheme } = currentComponent.meta || {}; + props.updateComponents({ + [currentComponent.id]: { + ...currentComponent, + meta: metaWithoutTheme, + }, + }); + } + handleCloseThemeSelector(); + }, + [props.updateComponents, handleCloseThemeSelector], + ); + const handleTabsReorder = useCallback( (oldIndex, newIndex) => { const { component, updateComponents } = props; @@ -489,28 +536,32 @@ const Tabs = props => { const renderChild = useCallback( ({ dragSourceRef: tabsDragSourceRef }) => ( - <TabsRenderer - tabItems={tabItems} - editMode={editMode} - renderHoverMenu={renderHoverMenu} - tabsDragSourceRef={tabsDragSourceRef} - handleDeleteComponent={handleDeleteComponent} - tabsComponent={tabsComponent} - activeKey={activeKey} - tabIds={tabIds} - handleClickTab={handleClickTab} - handleEdit={handleEdit} - tabBarPaddingLeft={tabBarPaddingLeft} - onTabsReorder={handleTabsReorder} - isEditingTabTitle={isEditingTabTitle} - onTabTitleEditingChange={handleTabTitleEditingChange} - /> + <ComponentThemeProvider themeId={tabsComponent.meta?.theme_id}> + <TabsRenderer + tabItems={tabItems} + editMode={editMode} + renderHoverMenu={renderHoverMenu} + tabsDragSourceRef={tabsDragSourceRef} + handleDeleteComponent={handleDeleteComponent} + handleOpenThemeSelector={handleOpenThemeSelector} + tabsComponent={tabsComponent} + activeKey={activeKey} + tabIds={tabIds} + handleClickTab={handleClickTab} + handleEdit={handleEdit} + tabBarPaddingLeft={tabBarPaddingLeft} + onTabsReorder={handleTabsReorder} + isEditingTabTitle={isEditingTabTitle} + onTabTitleEditingChange={handleTabTitleEditingChange} + /> + </ComponentThemeProvider> ), [ tabItems, editMode, renderHoverMenu, handleDeleteComponent, + handleOpenThemeSelector, tabsComponent, activeKey, tabIds, @@ -556,6 +607,14 @@ const Tabs = props => { </span> </Modal> )} + <ThemeSelectorModal + show={isThemeSelectorOpen} + onHide={handleCloseThemeSelector} + onApply={handleApplyTheme} + currentThemeId={tabsComponent.meta?.theme_id || null} + componentId={tabsComponent.id} + componentType="Tabs" + /> </> ); }; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx index 573fa268f64..5fd1706199e 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs/Tabs.test.tsx @@ -18,6 +18,7 @@ */ import { + fireEvent, render, screen, userEvent, @@ -25,7 +26,6 @@ import { } from 'spec/helpers/testing-library'; import { nativeFiltersInfo } from 'src/dashboard/fixtures/mockNativeFilters'; import DashboardComponent from 'src/dashboard/containers/DashboardComponent'; -import DeleteComponentButton from 'src/dashboard/components/DeleteComponentButton'; import getLeafComponentIdFromPath from 'src/dashboard/util/getLeafComponentIdFromPath'; import emptyDashboardLayout from 'src/dashboard/fixtures/emptyDashboardLayout'; import Tabs from './Tabs'; @@ -44,17 +44,6 @@ jest.mock('src/dashboard/containers/DashboardComponent', () => )), ); -jest.mock('src/dashboard/components/DeleteComponentButton', () => - jest.fn(props => ( - <button - type="button" - data-test="DeleteComponentButton" - onClick={props.onDelete} - > - DeleteComponentButton - </button> - )), -); jest.mock('src/dashboard/util/getLeafComponentIdFromPath', () => jest.fn()); jest.mock('src/dashboard/components/dnd/DragDroppable', () => ({ @@ -124,7 +113,10 @@ beforeEach(() => { test('Should render editMode:true', () => { const props = createProps(); - render(<Tabs {...props} />, { useRedux: true, useDnd: true }); + const { container } = render(<Tabs {...props} />, { + useRedux: true, + useDnd: true, + }); expect( screen .getAllByRole('tab') @@ -133,7 +125,10 @@ test('Should render editMode:true', () => { expect(screen.getAllByRole('tab', { name: 'remove' })).toHaveLength(3); expect(screen.getAllByRole('button', { name: 'Add tab' })).toHaveLength(1); expect(DashboardComponent).toHaveBeenCalledTimes(4); - expect(DeleteComponentButton).toHaveBeenCalledTimes(1); + // ComponentHeaderControls renders a button with "More Options" aria-label + expect( + container.querySelector('[aria-label="More Options"]'), + ).toBeInTheDocument(); }); test('Should render HoverMenu in editMode', () => { @@ -169,13 +164,16 @@ test('Should not render HoverMenu when renderHoverMenu is false', () => { test('Should render editMode:false', () => { const props = createProps(); props.editMode = false; - render(<Tabs {...props} />, { + const { container } = render(<Tabs {...props} />, { useRedux: true, useDnd: true, }); expect(screen.getAllByRole('tab')).toHaveLength(3); expect(DashboardComponent).toHaveBeenCalledTimes(4); - expect(DeleteComponentButton).not.toHaveBeenCalled(); + // ComponentHeaderControls should not be rendered + expect( + container.querySelector('[aria-label="More Options"]'), + ).not.toBeInTheDocument(); expect( screen.queryByRole('button', { name: 'remove' }), ).not.toBeInTheDocument(); @@ -188,26 +186,42 @@ test('Update component props', () => { const props = createProps(); (getLeafComponentIdFromPath as jest.Mock).mockResolvedValueOnce('none'); props.editMode = false; - const { rerender } = render(<Tabs {...props} />, { + const { rerender, container } = render(<Tabs {...props} />, { useRedux: true, useDnd: true, }); - expect(DeleteComponentButton).not.toHaveBeenCalled(); + // ComponentHeaderControls should not be rendered + expect( + container.querySelector('[aria-label="More Options"]'), + ).not.toBeInTheDocument(); props.editMode = true; rerender(<Tabs {...props} />); - expect(DeleteComponentButton).toHaveBeenCalledTimes(1); + // ComponentHeaderControls should now be rendered + expect( + container.querySelector('[aria-label="More Options"]'), + ).toBeInTheDocument(); }); -test('Clicking on "DeleteComponentButton"', () => { +test('Clicking Delete menu item deletes tabs container', async () => { const props = createProps(); - render(<Tabs {...props} />, { + const { container } = render(<Tabs {...props} />, { useRedux: true, useDnd: true, }); expect(props.deleteComponent).not.toHaveBeenCalled(); - userEvent.click(screen.getByTestId('DeleteComponentButton')); + + // Click the "More Options" menu button + const menuButton = container.querySelector('[aria-label="More Options"]'); + fireEvent.click(menuButton!); + + // Wait for menu to open and click Delete + await waitFor(() => { + const deleteOption = screen.getByText('Delete'); + fireEvent.click(deleteOption); + }); + expect(props.deleteComponent).toHaveBeenCalledWith( 'TABS-L-d9eyOE-b', 'GRID_ID', diff --git a/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.test.tsx b/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.test.tsx index cd5605b9500..f501094f2d7 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.test.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.test.tsx @@ -40,6 +40,7 @@ const mockProps: TabsRendererProps = { renderHoverMenu: true, tabsDragSourceRef: undefined, handleDeleteComponent: jest.fn(), + handleOpenThemeSelector: jest.fn(), tabsComponent: { id: 'test-tabs-id' }, activeKey: 'tab-1', tabIds: ['tab-1', 'tab-2'], @@ -166,6 +167,7 @@ describe('TabsRenderer', () => { tabItems: mockProps.tabItems, editMode: false, handleDeleteComponent: mockProps.handleDeleteComponent, + handleOpenThemeSelector: mockProps.handleOpenThemeSelector, tabsComponent: mockProps.tabsComponent, activeKey: mockProps.activeKey, tabIds: mockProps.tabIds, diff --git a/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx b/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx index 906150a5f32..336a0029e7f 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/TabsRenderer/TabsRenderer.tsx @@ -22,9 +22,11 @@ import { ReactElement, RefObject, useCallback, + useMemo, useRef, useState, } from 'react'; +import { t } from '@superset-ui/core'; import { styled } from '@apache-superset/core/ui'; import { LineEditableTabs, @@ -44,7 +46,9 @@ import { } from '@dnd-kit/sortable'; import HoverMenu from '../../menu/HoverMenu'; import DragHandle from '../../dnd/DragHandle'; -import DeleteComponentButton from '../../DeleteComponentButton'; +import ComponentHeaderControls, { + ComponentMenuKeys, +} from '../../menu/ComponentHeaderControls'; const StyledTabsContainer = styled.div<{ isDragging?: boolean }>` width: 100%; @@ -100,6 +104,7 @@ export interface TabsRendererProps { renderHoverMenu?: boolean; tabsDragSourceRef?: RefObject<HTMLDivElement>; handleDeleteComponent: () => void; + handleOpenThemeSelector: () => void; tabsComponent: TabsComponent; activeKey: string; tabIds: string[]; @@ -162,6 +167,7 @@ const TabsRenderer = memo<TabsRendererProps>( renderHoverMenu = true, tabsDragSourceRef, handleDeleteComponent, + handleOpenThemeSelector, tabsComponent, activeKey, tabIds, @@ -206,6 +212,38 @@ const TabsRenderer = memo<TabsRendererProps>( setActiveId(null); }, []); + const handleMenuClick = useCallback( + (key: string) => { + switch (key) { + case ComponentMenuKeys.ApplyTheme: + handleOpenThemeSelector(); + break; + case ComponentMenuKeys.Delete: + handleDeleteComponent(); + break; + default: + break; + } + }, + [handleDeleteComponent, handleOpenThemeSelector], + ); + + const menuItems = useMemo( + () => [ + { + key: ComponentMenuKeys.ApplyTheme, + label: t('Apply theme'), + }, + { type: 'divider' as const }, + { + key: ComponentMenuKeys.Delete, + label: t('Delete'), + danger: true, + }, + ], + [], + ); + const isDragging = activeId !== null; return ( @@ -217,7 +255,12 @@ const TabsRenderer = memo<TabsRendererProps>( {editMode && renderHoverMenu && tabsDragSourceRef && ( <HoverMenu innerRef={tabsDragSourceRef} position="left"> <DragHandle position="left" /> - <DeleteComponentButton onDelete={handleDeleteComponent} /> + <ComponentHeaderControls + componentId={tabsComponent.id} + menuItems={menuItems} + onMenuClick={handleMenuClick} + editMode={editMode} + /> </HoverMenu> )} diff --git a/superset-frontend/src/dashboard/components/menu/ThemeSelectorModal/index.tsx b/superset-frontend/src/dashboard/components/menu/ThemeSelectorModal/index.tsx index 7ded1d16317..29ba3612a3b 100644 --- a/superset-frontend/src/dashboard/components/menu/ThemeSelectorModal/index.tsx +++ b/superset-frontend/src/dashboard/components/menu/ThemeSelectorModal/index.tsx @@ -55,7 +55,7 @@ const StyledModalContent = styled.div` .theme-selector-label { display: block; margin-bottom: ${theme.sizeUnit * 2}px; - font-weight: ${theme.fontWeightMedium}; + font-weight: ${theme.fontWeightStrong}; } .theme-selector-help { @@ -211,24 +211,28 @@ const ThemeSelectorModal = ({ > <StyledModalContent> <div className="theme-selector-field"> - <label className="theme-selector-label" htmlFor="theme-select"> + <span className="theme-selector-label"> {t('Select a theme for this %s', componentType)} - </label> + </span> <Select - id="theme-select" + ariaLabel={t('Select a theme')} data-test="theme-selector-select" value={selectedThemeId} - onChange={(value: number | null) => setSelectedThemeId(value)} + onChange={value => + setSelectedThemeId(value === undefined ? null : (value as number)) + } options={themeOptions} placeholder={t('Select a theme...')} allowClear onClear={handleClear} loading={isLoading} disabled={isLoading} - style={{ width: '100%' }} + css={css` + width: 100%; + `} notFoundContent={ error ? ( - <span style={{ color: theme.colorError }}>{error}</span> + <span css={{ color: theme.colorError }}>{error}</span> ) : ( t('No themes available') ) diff --git a/superset-frontend/src/dashboard/types.ts b/superset-frontend/src/dashboard/types.ts index b2bde968368..3f9661da83d 100644 --- a/superset-frontend/src/dashboard/types.ts +++ b/superset-frontend/src/dashboard/types.ts @@ -310,4 +310,5 @@ export enum MenuKeys { ManageEmailReports = 'manage_email_reports', ExportPivotXlsx = 'export_pivot_xlsx', EmbedCode = 'embed_code', + ApplyTheme = 'apply_theme', } diff --git a/superset-frontend/src/theme/ThemeController.ts b/superset-frontend/src/theme/ThemeController.ts index ae798dfc97a..706b7743378 100644 --- a/superset-frontend/src/theme/ThemeController.ts +++ b/superset-frontend/src/theme/ThemeController.ts @@ -277,6 +277,93 @@ export class ThemeController { this.dashboardThemes.delete(themeId); } + // Cache for raw theme configs (for hierarchical component theming) + private themeConfigCache: Map<string, AnyThemeConfig> = new Map(); + + /** + * Fetches and caches the raw theme configuration from the API. + * Unlike createDashboardThemeProvider, this returns the raw config + * so it can be merged with any parent theme. + * @param themeId - The theme ID to fetch + * @returns The raw theme configuration or null if not found + */ + public async fetchThemeConfig( + themeId: string, + ): Promise<AnyThemeConfig | null> { + try { + // Check cache first + if (this.themeConfigCache.has(themeId)) { + return this.themeConfigCache.get(themeId)!; + } + + // Fetch from API + const getTheme = makeApi<void, { result: { json_data: string } }>({ + method: 'GET', + endpoint: `/api/v1/theme/${themeId}`, + }); + + const { result } = await getTheme(); + const themeConfig = JSON.parse(result.json_data); + + if (themeConfig) { + const normalizedConfig = this.normalizeTheme(themeConfig); + + // Load custom fonts if specified + const fontUrls = (normalizedConfig?.token as Record<string, unknown>) + ?.fontUrls as string[] | undefined; + this.loadFonts(fontUrls); + + // Cache the raw config + this.themeConfigCache.set(themeId, normalizedConfig); + + return normalizedConfig; + } + return null; + } catch (error) { + console.error('Failed to fetch theme config:', error); + return null; + } + } + + /** + * Creates a Theme by merging a theme config with an optional parent theme. + * This enables hierarchical theming where themes inherit from their parent. + * + * Use this method for component-level themes that need to inherit from + * their parent in the component tree. For dashboard-level themes that + * don't have a parent, omit the parentThemeConfig parameter. + * + * @param themeId - The theme ID to apply + * @param parentThemeConfig - Optional parent theme's configuration to use as base. + * If not provided, uses the global default/dark theme. + * @returns A Theme object merged with the parent, or null if not found + */ + public async createTheme( + themeId: string, + parentThemeConfig?: AnyThemeConfig, + ): Promise<Theme | null> { + try { + const componentConfig = await this.fetchThemeConfig(themeId); + if (!componentConfig) return null; + + // Import Theme dynamically + const { Theme } = await import('@apache-superset/core/ui'); + + // Use parent theme as base if provided, otherwise fall back to default/dark + const isDarkMode = isThemeConfigDark(componentConfig); + const fallbackBase = isDarkMode ? this.darkTheme : this.defaultTheme; + const baseTheme = parentThemeConfig || fallbackBase || undefined; + + // Create theme with proper inheritance + const componentTheme = Theme.fromConfig(componentConfig, baseTheme); + + return componentTheme; + } catch (error) { + console.error('Failed to create component theme:', error); + return null; + } + } + /** * Clears all cached dashboard themes. */ diff --git a/superset-frontend/src/theme/ThemeProvider.tsx b/superset-frontend/src/theme/ThemeProvider.tsx index 68dae34635b..25bc301364d 100644 --- a/superset-frontend/src/theme/ThemeProvider.tsx +++ b/superset-frontend/src/theme/ThemeProvider.tsx @@ -118,6 +118,12 @@ export function SupersetThemeProvider({ [themeController], ); + const createTheme = useCallback( + (themeId: string, parentThemeConfig?: AnyThemeConfig) => + themeController.createTheme(themeId, parentThemeConfig), + [themeController], + ); + const getAppliedThemeId = useCallback( () => themeController.getAppliedThemeId(), [themeController], @@ -138,6 +144,7 @@ export function SupersetThemeProvider({ canSetTheme, canDetectOSPreference, createDashboardThemeProvider, + createTheme, getAppliedThemeId, }), [ @@ -154,6 +161,7 @@ export function SupersetThemeProvider({ canSetTheme, canDetectOSPreference, createDashboardThemeProvider, + createTheme, getAppliedThemeId, ], );
