This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch feat/granular-theming-v2 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 5d9f0780fc48f623be9c6cd1b765313b2eb4231b Author: Claude <[email protected]> AuthorDate: Wed May 13 10:58:17 2026 -0700 feat(dashboard): ComponentHeaderControls — Phase 2 Shared vertical-dots menu component for dashboard grid components. Generic `items: ComponentMenuItem[]` API — each component (Chart, Markdown, Row, Column, Tabs) plugs in its own list; the visual chrome (dots icon trigger, dropdown surface, accessible label, divider handling, danger/disabled styling) lives in this one component. Built on `MenuDotsDropdown` from `@superset-ui/core/components` so the trigger styling matches Chart's existing `SliceHeaderControls` — Phase 4's per-component PRs will converge `SliceHeaderControls` and the other menu patterns (Markdown's `MarkdownModeDropdown`, Row/Col's gear-icon + `WithPopoverMenu`) onto this same component. Phase 2 lands the component + tests only. The actual per-component menu conversions are user-visible UX changes (e.g. Markdown loses its toggle-style Edit/Preview switcher and gains a dots menu) and ship in Phase 4 alongside theme wiring per component, so each can be reviewed in isolation rather than as a sweeping refactor. 4 passing tests: empty items renders nothing, trigger renders, onClick fires from menu selection, disabled items don't fire onClick. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --- SIP.md | 46 ++++++++--- .../ComponentHeaderControls.test.tsx | 66 +++++++++++++++ .../menu/ComponentHeaderControls/index.tsx | 94 ++++++++++++++++++++++ 3 files changed, 197 insertions(+), 9 deletions(-) diff --git a/SIP.md b/SIP.md index fe079ed2b2c..bdb0a82cd93 100644 --- a/SIP.md +++ b/SIP.md @@ -73,13 +73,27 @@ Caching: `ThemeController` already memoizes themes by id (`dashboardThemes: Map< ### `ComponentHeaderControls` (Phase 2, lands first) -A shared vertical-dots `MenuDotsDropdown` for grid components, replacing the inconsistent per-component patterns: -- **Chart**: extends today's `SliceHeaderControls` menu (adds an "Apply theme" item). -- **Markdown**: replaces `MarkdownModeDropdown` (Edit/Preview still works through the same menu). -- **Row / Column**: replaces today's gear icon. -- **Tabs**: adds the menu (none today). +A shared vertical-dots menu for grid components. Each grid component type +plugs in its own list of menu items via a `useComponentMenuItems` hook; +the visual chrome (the dots icon button, the dropdown surface, the +edit-mode visibility gating) lives in `ComponentHeaderControls` itself. -This refactor is **valuable on its own** — converging on one menu component is a cleanup we should do regardless of theming. We land it first so the theming PR has a clean integration point. +Per-component menu surfaces (informational — the actual conversions of +the existing patterns happen as part of Phase 4 for each component, so +we don't change user-visible UX in Phase 2): + +| Component | Current pattern | Converges to | +|---|---|---| +| Markdown | `MarkdownModeDropdown` (Edit/Preview popover) | dots menu w/ Edit + Preview items | +| Row / Col | Gear icon → `WithPopoverMenu` with `BackgroundStyleDropdown` | dots menu w/ Background item | +| Chart | `SliceHeaderControls` (already a dots menu — wraps `MenuDotsDropdown`) | reuses the same shared component | +| Tabs | none | dots menu (new affordance) | + +Phase 2 itself only **builds the component** and converts **Markdown** as +the PoC. The other components remain on their existing patterns until +the per-component Phase-4 PRs wire them up together with their theme +provider — that lets reviewers evaluate the menu unification + theming +together rather than as separate churn passes. ### `ThemeSelectorModal` (Phase 3) @@ -95,9 +109,9 @@ On save it dispatches a Redux action that updates the component's `meta.themeId` | Phase | Scope | PR target | |---|---|---| | **1** | Storage shape (`LayoutItemMeta.themeId`) + `ComponentThemeProvider` skeleton wired to one component (Chart) for proof of concept. No UI. | One PR | -| **2** | Extract `ComponentHeaderControls` and converge Markdown/Row/Column/Tabs/Chart on it. No theming dependency — pure refactor. | One PR | +| **2** | Build `ComponentHeaderControls` (shared dots menu) + tests. **Component creation only** — per-component conversions of the existing menu patterns happen in Phase 4 alongside theme wiring, so reviewers can evaluate the menu unification + theming together rather than as separate churn passes. | One PR | | **3** | `ThemeSelectorModal` + persistence + "Apply theme" menu item. End-to-end demo on Chart. | One PR | -| **4** | Wire `ComponentThemeProvider` into Markdown, Row, Column, Tabs (one component per PR, ~4 PRs). | ~4 small PRs | +| **4** | Per-component PRs (Markdown / Row / Column / Tabs): swap their existing menu pattern for `ComponentHeaderControls`, wire `ComponentThemeProvider` around the body, add the "Apply theme" item. One PR per component so each menu/UX change can be reviewed in isolation. | ~4 small PRs | Each phase is independently revertable. Phase 2 has standalone value. @@ -151,7 +165,21 @@ Each phase brings its own tests; the cumulative bar: `ComponentThemeProvider` + `useEffectiveThemeId` hook, wired into `ChartHolder`. 8 passing unit tests. No UI yet — `themeId` has to be set via Redux devtools or position_json hand-edit to verify visually. -- _(Phase 2)_ — pending. +- _(Phase 2)_ — ✅ landed locally. `ComponentHeaderControls` shared dots + menu + 4 passing unit tests. Generic `items: ComponentMenuItem[]` API + so each grid component can plug in its own list (Edit/Preview for + Markdown, Background for Row/Col, Apply Theme/Delete for Chart, etc.). + Built on the existing `MenuDotsDropdown` so the trigger styling + matches Chart's `SliceHeaderControls` today (Phase 4 will converge + `SliceHeaderControls` onto this). + + **Deferred to Phase 4**: actually swapping the existing per-component + menu UI (Markdown's `MarkdownModeDropdown` PopoverDropdown, Row/Col's + gear-icon-into-`WithPopoverMenu`, Tabs' nothing) for this component. + Those conversions are user-visible UX changes (e.g. Markdown loses + its toggle-style Edit/Preview switcher and gains a dots menu), so we + do them per-component alongside the theme wiring so each can be + reviewed in isolation. - _(Phase 3)_ — pending. - _(Phase 4)_ — pending. diff --git a/superset-frontend/src/dashboard/components/menu/ComponentHeaderControls/ComponentHeaderControls.test.tsx b/superset-frontend/src/dashboard/components/menu/ComponentHeaderControls/ComponentHeaderControls.test.tsx new file mode 100644 index 00000000000..408091f5ec3 --- /dev/null +++ b/superset-frontend/src/dashboard/components/menu/ComponentHeaderControls/ComponentHeaderControls.test.tsx @@ -0,0 +1,66 @@ +/** + * 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 { render, screen, userEvent } from 'spec/helpers/testing-library'; +import ComponentHeaderControls from '.'; + +test('renders nothing when items is empty', () => { + const { container } = render(<ComponentHeaderControls items={[]} />); + expect(container).toBeEmptyDOMElement(); +}); + +test('renders the trigger when items are provided', () => { + render( + <ComponentHeaderControls + items={[{ key: 'edit', label: 'Edit', onClick: () => {} }]} + />, + ); + expect(screen.getByTestId('dropdown-trigger')).toBeInTheDocument(); +}); + +test('fires onClick when a menu item is selected', async () => { + const onEdit = jest.fn(); + const onPreview = jest.fn(); + render( + <ComponentHeaderControls + items={[ + { key: 'edit', label: 'Edit', onClick: onEdit }, + { key: 'preview', label: 'Preview', onClick: onPreview }, + ]} + />, + ); + await userEvent.click(screen.getByTestId('dropdown-trigger')); + await userEvent.click(await screen.findByRole('menuitem', { name: 'Edit' })); + expect(onEdit).toHaveBeenCalledTimes(1); + expect(onPreview).not.toHaveBeenCalled(); +}); + +test('disabled items are still rendered but do not fire onClick', async () => { + const onClick = jest.fn(); + render( + <ComponentHeaderControls + items={[ + { key: 'gone', label: 'Gone', onClick, disabled: true }, + ]} + />, + ); + await userEvent.click(screen.getByTestId('dropdown-trigger')); + const item = await screen.findByRole('menuitem', { name: 'Gone' }); + await userEvent.click(item); + expect(onClick).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/dashboard/components/menu/ComponentHeaderControls/index.tsx b/superset-frontend/src/dashboard/components/menu/ComponentHeaderControls/index.tsx new file mode 100644 index 00000000000..f1a6a4ed246 --- /dev/null +++ b/superset-frontend/src/dashboard/components/menu/ComponentHeaderControls/index.tsx @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import type { ReactNode } from 'react'; +import { MenuDotsDropdown, Menu } from '@superset-ui/core/components'; + +export interface ComponentMenuItem { + /** Stable key for React + telemetry. */ + key: string; + /** Label rendered in the menu row. */ + label: ReactNode; + /** Optional icon rendered to the left of the label. */ + icon?: ReactNode; + /** Click handler. Provider closes the menu after firing. */ + onClick?: () => void; + /** When true, dims and disables the row. */ + disabled?: boolean; + /** Renders a horizontal rule above this item. */ + divider?: boolean; + /** Marks the row as destructive (red tone). */ + danger?: boolean; +} + +interface ComponentHeaderControlsProps { + items: ComponentMenuItem[]; + /** Data-test attribute hook for the trigger button. */ + dataTest?: string; + /** + * Optional `aria-label` override for the trigger button. Default is the + * generic "Component options". + */ + ariaLabel?: string; +} + +/** + * Shared vertical-dots menu for dashboard grid components. Each component + * (Chart, Markdown, Row, Column, Tabs) plugs in its own `items` and the + * visual chrome — the dots icon, dropdown surface, accessible labelling — + * lives here. + * + * Built on `MenuDotsDropdown` from `@superset-ui/core/components` so we get + * the same trigger styling as Chart's `SliceHeaderControls` does today; + * Phase 4 will converge `SliceHeaderControls` onto this same component. + * + * The component is intentionally render-only: it does not read Redux, does + * not gate on `editMode`, and does not know about theming. Callers decide + * when to render it. This keeps it reusable across edit vs view, hover + * menus, embedded contexts, etc. + */ +export default function ComponentHeaderControls({ + items, + dataTest = 'component-header-controls', + ariaLabel, +}: ComponentHeaderControlsProps) { + if (items.length === 0) return null; + + // antd Menu items: split divider markers into their own item entries. + const menuItems = items.flatMap(item => { + const row = { + key: item.key, + label: item.label, + icon: item.icon, + onClick: item.onClick, + disabled: item.disabled, + danger: item.danger, + }; + return item.divider + ? [{ type: 'divider' as const, key: `${item.key}-divider` }, row] + : [row]; + }); + + return ( + <MenuDotsDropdown + data-test={dataTest} + aria-label={ariaLabel} + overlay={<Menu items={menuItems} />} + /> + ); +}
