mirror of
https://github.com/apache/superset.git
synced 2026-05-21 15:55:10 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
46
SIP.md
46
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.
|
||||
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
@@ -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} />}
|
||||
/>
|
||||
);
|
||||
}
|
||||
Reference in New Issue
Block a user