diff --git a/superset-frontend/packages/superset-core/src/chat/index.ts b/superset-frontend/packages/superset-core/src/chat/index.ts index ad91761f150d..1fb689361e06 100644 --- a/superset-frontend/packages/superset-core/src/chat/index.ts +++ b/superset-frontend/packages/superset-core/src/chat/index.ts @@ -151,6 +151,57 @@ export declare const onDidChangeDisplayMode: Event; */ export declare const onDidResizePanel: Event<{ width: number }>; -// TODO: client actions API — tool availability functions will be added here -// once the client_actions SIP is finalized. The chat namespace is the -// intended integration point between the two SIPs. +/** The normalized answer returned to the assistant by a browser-owned tool. */ +export interface ClientToolResult { + content: string; + isError?: boolean; +} + +/** The part of a browser-owned tool definition that is safe to send to a model. */ +export interface ClientToolSpec { + /** Unique tool name. Later registrations take precedence on collisions. */ + name: string; + /** Explain when the model should use the tool and what it returns. */ + description: string; + /** JSON Schema describing the tool's argument object. */ + inputSchema: Record; +} + +/** + * A tool implemented by the page in the user's browser. + * + * The handler is never sent to the chat extension's backend. It executes in + * the active Superset page and should return concise, model-readable content. + */ +export interface ClientTool extends ClientToolSpec { + execute: ( + args: Record, + ) => Promise | ClientToolResult; +} + +/** + * Registers browser-owned tools for the lifetime of a page or component. + * Dispose the returned registration when that owner unmounts. If names + * collide, the most recently registered group wins until it is disposed. + */ +export declare function registerClientTools( + tools: readonly ClientTool[], +): Disposable; + +/** Returns the available client-tool descriptions without their handlers. */ +export declare function getClientTools(): ClientToolSpec[]; + +/** + * Executes a registered browser-owned tool. + * + * This promise always resolves. Unknown tools, thrown handlers, rejected + * handlers, and invalid results are normalized to `isError: true` so an + * assistant can always continue its tool-call loop. + */ +export declare function executeClientTool( + name: string, + args: Record, +): Promise; + +/** Fires whenever the resolved set of browser-owned tools changes. */ +export declare const onDidChangeClientTools: Event; diff --git a/superset-frontend/packages/superset-core/src/dashboard/index.ts b/superset-frontend/packages/superset-core/src/dashboard/index.ts index e8cb136ff7c7..eee98aa10a9d 100644 --- a/superset-frontend/packages/superset-core/src/dashboard/index.ts +++ b/superset-frontend/packages/superset-core/src/dashboard/index.ts @@ -53,6 +53,35 @@ import type { Event } from '../common'; +/** + * How a container arranges its own children. + * + * - `grid` (the default, and what every node written before this field + * existed still means) — children occupy cells of the container's column + * grid and are compacted upward, so the space a removed or moved block + * leaves behind closes itself. + * - `free` — the same cells, uncompacted. A child stays exactly where it was + * put and may overlap a sibling. It reads the same four child fields + * `grid` does, so switching a container between the two never discards a + * position an author or an agent set. + * - `flex` — children flow along a line and wrap, sharing that line in + * proportion to their `colSpan` instead of occupying named cells. Their + * position is their order in `children`, not a coordinate. + * + * A flow is still not a mode, and the two that are here are the two that + * genuinely could not be expressed as a grid: `free` because compaction is a + * property of the container rather than of any child's coordinates, and + * `flex` because a proportional line has no cells to name. + */ +export type LayoutMode = 'grid' | 'free' | 'flex'; + +/** Where `flex` children sit along the line they flow down. */ +export type FlexJustify = + 'start' | 'center' | 'end' | 'space-between' | 'space-around'; + +/** Where `flex` children sit across the line they flow down. */ +export type FlexAlign = 'start' | 'center' | 'end' | 'stretch'; + /** * Layout of a single node: a grid it lays out its own children in (only * meaningful when the node is a container — ignored on leaf nodes), plus @@ -60,14 +89,16 @@ import type { Event } from '../common'; * at once — a `canvas` nested inside another `canvas` both holds a grid for * its own children and occupies cells in its parent's. * - * There is no separate "flow" or "absolute" mode: a single-column grid with - * every child left at its default full-width span behaves like a plain - * top-to-bottom stack — it falls out of the same schema rather than - * requiring a different one. + * There is no separate "flow" mode: a single-column grid with every child + * left at its default full-width span behaves like a plain top-to-bottom + * stack — it falls out of the same schema rather than requiring a different + * one. {@link LayoutMode} exists only for the arrangements that do not. */ export interface LayoutProps { // --- Container side: how this node arranges its own children. Ignored // on a node with no `children`. --- + /** How this node arranges its children. Default: `grid`. */ + mode?: LayoutMode; /** Number of equal fractional column tracks. Default: 24. */ columns?: number; gap?: number; @@ -79,8 +110,22 @@ export interface LayoutProps { */ rowUnit?: number; + // --- Container side, `flex` only. Ignored in every other mode, rather + // than quietly changing what a grid does. --- + /** Which way `flex` children flow. Default: `row`. */ + direction?: 'row' | 'column'; + /** Whether a `flex` line wraps once it is full. Default: true. */ + wrap?: boolean; + justify?: FlexJustify; + align?: FlexAlign; + // --- Child side: where this node sits within its parent's grid. --- - /** How many of the parent's columns this node spans. Default: every column (full width). */ + /** + * How many of the parent's columns this node spans. Default: every column + * (full width). In a `flex` parent there are no columns to occupy, so this + * is read as the node's share of the line instead — two siblings at 6 and + * 12 take a third and two thirds of it. + */ colSpan?: number; /** How many row tracks this node spans. Default: 1. */ rowSpan?: number; @@ -112,7 +157,16 @@ export interface DashboardNode { * repositioning a node on the canvas never changes this array. */ children?: string[]; - /** Leaf/building-block nodes only — functional/content config. */ + /** + * Functional/content config. Leaf/building-block nodes carry whatever + * their renderer reads. + * + * The root `canvas` is the one container that also carries some: it is the + * only node a fact about the dashboard *itself* can belong to, so that is + * where its `title` lives. A title placed as a `markdown` block is a + * different thing — that one is content, arranged like any other block; + * this one is what the dashboard is called. + */ props?: Record; /** Leaf/building-block nodes only — visual customization. */ style?: Record; diff --git a/superset-frontend/packages/superset-core/src/navigation/index.ts b/superset-frontend/packages/superset-core/src/navigation/index.ts index 585a87f2e9fc..bf92ee179465 100644 --- a/superset-frontend/packages/superset-core/src/navigation/index.ts +++ b/superset-frontend/packages/superset-core/src/navigation/index.ts @@ -31,8 +31,8 @@ import { Event } from '../common'; /** * The set of top-level application surfaces. * - * `'explore'`, `'dashboard'` and `'dataset'` are the single-entity - * editing/viewing surfaces. `'chart_list'`, `'dashboard_list'` and + * `'explore'`, `'dashboard'`, `'dashboard_v2'` and `'dataset'` are the + * single-entity editing/viewing surfaces. `'chart_list'`, `'dashboard_list'` and * `'dataset_list'` are the browse/list surfaces, distinct from those because no * single entity is active. `'sqllab'` is the SQL editor where * `sqlLab.getCurrentTab()` resolves; `'query_history'` and `'saved_queries'` @@ -41,6 +41,7 @@ import { Event } from '../common'; */ export type Page = | 'dashboard' + | 'dashboard_v2' | 'dashboard_list' | 'explore' | 'chart_list' diff --git a/superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx b/superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx index 81c22d711d9b..dbd41a66b76b 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/Icons/AntdEnhanced.tsx @@ -86,6 +86,7 @@ import { FundProjectionScreenOutlined, FunctionOutlined, HighlightOutlined, + HolderOutlined, HomeOutlined, InfoCircleOutlined, InfoCircleFilled, @@ -121,6 +122,7 @@ import { PushpinFilled, PushpinOutlined, QuestionCircleOutlined, + RedoOutlined, ReloadOutlined, RightOutlined, SaveOutlined, @@ -137,6 +139,7 @@ import { TagsOutlined, TableOutlined, LockOutlined, + UndoOutlined, UnlockOutlined, UploadOutlined, UpOutlined, @@ -245,6 +248,7 @@ const AntdIcons = { GoogleOutlined, GroupOutlined, HighlightOutlined, + HolderOutlined, HomeOutlined, InfoCircleOutlined, InfoCircleFilled, @@ -281,6 +285,7 @@ const AntdIcons = { PushpinOutlined, ReloadOutlined, QuestionCircleOutlined, + RedoOutlined, RightOutlined, SaveOutlined, SearchOutlined, @@ -296,6 +301,7 @@ const AntdIcons = { TagsOutlined, TableOutlined, LockOutlined, + UndoOutlined, UploadOutlined, UnlockOutlined, UpOutlined, diff --git a/superset-frontend/src/core/chat/ChatProvider.test.ts b/superset-frontend/src/core/chat/ChatProvider.test.ts index 392f50a114f8..accc2986b652 100644 --- a/superset-frontend/src/core/chat/ChatProvider.test.ts +++ b/superset-frontend/src/core/chat/ChatProvider.test.ts @@ -230,6 +230,101 @@ test('setDisplayMode updates mode and fires event only on change', () => { expect(modeChanged).toHaveBeenCalledWith('panel'); }); +test('client tools are exposed without their browser handlers', () => { + const provider = ChatProvider.getInstance(); + const execute = jest.fn(() => ({ content: 'outline' })); + + provider.registerClientTools([ + { + name: 'dashboard_get_state', + description: 'Reads the active dashboard.', + inputSchema: { type: 'object', properties: {} }, + execute, + }, + ]); + + expect(provider.getClientTools()).toEqual([ + { + name: 'dashboard_get_state', + description: 'Reads the active dashboard.', + inputSchema: { type: 'object', properties: {} }, + }, + ]); +}); + +test('the latest client-tool registration wins until it is disposed', async () => { + const provider = ChatProvider.getInstance(); + provider.registerClientTools([ + { + name: 'dashboard_get_state', + description: 'Earlier tool.', + inputSchema: { type: 'object' }, + execute: () => ({ content: 'earlier' }), + }, + ]); + const latest = provider.registerClientTools([ + { + name: 'dashboard_get_state', + description: 'Latest tool.', + inputSchema: { type: 'object' }, + execute: () => ({ content: 'latest' }), + }, + ]); + + await expect( + provider.executeClientTool('dashboard_get_state', {}), + ).resolves.toEqual({ content: 'latest' }); + + latest.dispose(); + await expect( + provider.executeClientTool('dashboard_get_state', {}), + ).resolves.toEqual({ content: 'earlier' }); +}); + +test('client-tool changes fire after registration and disposal', () => { + const provider = ChatProvider.getInstance(); + const listener = jest.fn(); + provider.onDidChangeClientTools(listener); + const registration = provider.registerClientTools([ + { + name: 'dashboard_get_state', + description: 'Reads the active dashboard.', + inputSchema: { type: 'object' }, + execute: () => ({ content: 'outline' }), + }, + ]); + + expect(listener).toHaveBeenLastCalledWith([ + expect.objectContaining({ name: 'dashboard_get_state' }), + ]); + + registration.dispose(); + expect(listener).toHaveBeenLastCalledWith([]); +}); + +test('client-tool execution always resolves with a model-readable result', async () => { + const provider = ChatProvider.getInstance(); + provider.registerClientTools([ + { + name: 'throws', + description: 'Throws.', + inputSchema: { type: 'object' }, + execute: () => { + throw new Error('bad arguments'); + }, + }, + ]); + + await expect(provider.executeClientTool('missing', {})).resolves.toEqual({ + content: 'Unknown client tool "missing".', + isError: true, + }); + await expect(provider.executeClientTool('throws', {})).resolves.toEqual({ + content: 'Client tool "throws" failed: bad arguments', + isError: true, + }); +}); + test('state reflects changes after registration and open', () => { const provider = ChatProvider.getInstance(); @@ -248,10 +343,19 @@ test('reset clears all state', () => { provider.registerChat({ id: 'acme.chat', name: 'Acme' }, trigger, panel); provider.open(); provider.setDisplayMode('panel'); + provider.registerClientTools([ + { + name: 'dashboard_get_state', + description: 'Reads the active dashboard.', + inputSchema: { type: 'object' }, + execute: () => ({ content: 'outline' }), + }, + ]); provider.reset(); expect(provider.getChat()).toBeUndefined(); expect(provider.isOpen()).toBe(false); expect(provider.getDisplayMode()).toBe('floating'); + expect(provider.getClientTools()).toEqual([]); }); diff --git a/superset-frontend/src/core/chat/ChatProvider.ts b/superset-frontend/src/core/chat/ChatProvider.ts index b0d2f79af50e..9e2b55e64a42 100644 --- a/superset-frontend/src/core/chat/ChatProvider.ts +++ b/superset-frontend/src/core/chat/ChatProvider.ts @@ -29,6 +29,9 @@ import { createValueEventEmitter, createEventEmitter } from '../utils'; type Chat = chatApi.Chat; type DisplayMode = chatApi.DisplayMode; +type ClientTool = chatApi.ClientTool; +type ClientToolResult = chatApi.ClientToolResult; +type ClientToolSpec = chatApi.ClientToolSpec; /** * Singleton manager for the chat provider. @@ -57,6 +60,10 @@ class ChatProvider { private resizePanelEmitter = createEventEmitter<{ width: number }>(); + private clientToolGroups: ClientTool[][] = []; + + private clientToolsEmitter = createEventEmitter(); + private modeEmitter: ReturnType>; private constructor() { @@ -190,6 +197,68 @@ class ChatProvider { return this.resizePanelEmitter.subscribe; } + private resolveClientTools(): Map { + const resolved = new Map(); + this.clientToolGroups.forEach(group => { + group.forEach(tool => resolved.set(tool.name, tool)); + }); + return resolved; + } + + public registerClientTools(tools: readonly ClientTool[]): Disposable { + const group = [...tools]; + this.clientToolGroups.push(group); + this.clientToolsEmitter.fire(this.getClientTools()); + + return new Disposable(() => { + const index = this.clientToolGroups.indexOf(group); + if (index === -1) return; + this.clientToolGroups.splice(index, 1); + this.clientToolsEmitter.fire(this.getClientTools()); + }); + } + + public getClientTools(): ClientToolSpec[] { + return [...this.resolveClientTools().values()].map( + ({ name, description, inputSchema }) => ({ + name, + description, + inputSchema, + }), + ); + } + + public async executeClientTool( + name: string, + args: Record, + ): Promise { + const tool = this.resolveClientTools().get(name); + if (!tool) { + return { content: `Unknown client tool "${name}".`, isError: true }; + } + + try { + const result = await tool.execute(args); + if (result && typeof result.content === 'string') return result; + return { + content: `Client tool "${name}" returned no content.`, + isError: true, + }; + } catch (error) { + return { + content: + error instanceof Error + ? `Client tool "${name}" failed: ${error.message}` + : `Client tool "${name}" failed.`, + isError: true, + }; + } + } + + public get onDidChangeClientTools() { + return this.clientToolsEmitter.subscribe; + } + public reset(): void { this.chat = undefined; this.trigger = undefined; @@ -200,6 +269,8 @@ class ChatProvider { this.openEmitter = createEventEmitter(); this.closeEmitter = createEventEmitter(); this.resizePanelEmitter = createEventEmitter<{ width: number }>(); + this.clientToolGroups = []; + this.clientToolsEmitter = createEventEmitter(); this.modeEmitter = createValueEventEmitter('floating'); this.stateSubscribers.clear(); setItem(LocalStorageKeys.ChatState, { open: false, mode: 'floating' }); diff --git a/superset-frontend/src/core/chat/index.test.ts b/superset-frontend/src/core/chat/index.test.ts index 2fee1989135f..10359cbc6054 100644 --- a/superset-frontend/src/core/chat/index.test.ts +++ b/superset-frontend/src/core/chat/index.test.ts @@ -66,3 +66,24 @@ test('setDisplayMode updates the display mode', () => { chat.setDisplayMode('panel'); expect(chat.getDisplayMode()).toBe('panel'); }); + +test('registerClientTools exposes and executes a page-owned tool', async () => { + const registration = chat.registerClientTools([ + { + name: 'dashboard_get_state', + description: 'Reads the active dashboard.', + inputSchema: { type: 'object', properties: {} }, + execute: args => ({ content: JSON.stringify(args) }), + }, + ]); + + expect(chat.getClientTools()).toEqual([ + expect.objectContaining({ name: 'dashboard_get_state' }), + ]); + await expect( + chat.executeClientTool('dashboard_get_state', { detail: 'full' }), + ).resolves.toEqual({ content: '{"detail":"full"}' }); + + registration.dispose(); + expect(chat.getClientTools()).toEqual([]); +}); diff --git a/superset-frontend/src/core/chat/index.ts b/superset-frontend/src/core/chat/index.ts index b7536760926f..de885e7316fd 100644 --- a/superset-frontend/src/core/chat/index.ts +++ b/superset-frontend/src/core/chat/index.ts @@ -79,4 +79,8 @@ export const chat: typeof chatApi = { // The host fires this from its panel resizer; until that chrome exists the // event is exposed but never fires. onDidResizePanel: provider.onDidResizePanel, + registerClientTools: provider.registerClientTools.bind(provider), + getClientTools: provider.getClientTools.bind(provider), + executeClientTool: provider.executeClientTool.bind(provider), + onDidChangeClientTools: provider.onDidChangeClientTools, }; diff --git a/superset-frontend/src/core/dashboard/BuildingBlockView.test.tsx b/superset-frontend/src/core/dashboard/BuildingBlockView.test.tsx new file mode 100644 index 000000000000..eb01deb10bd8 --- /dev/null +++ b/superset-frontend/src/core/dashboard/BuildingBlockView.test.tsx @@ -0,0 +1,166 @@ +/** + * 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 } from 'spec/helpers/testing-library'; +import DashboardProvider from './DashboardProvider'; +import { registerBuiltInBuildingBlocks } from './registerBuiltInBuildingBlocks'; +import BuildingBlockView from './BuildingBlockView'; + +const provider = DashboardProvider.getInstance(); + +beforeAll(() => { + registerBuiltInBuildingBlocks(); +}); + +beforeEach(() => { + provider.reset(); +}); + +const withBlock = () => { + const rootId = provider.getRoot().id; + const id = provider.addBuildingBlock(rootId, 0, { + type: 'markdown', + props: { content: 'Quarterly notes' }, + }); + render(); + return { rootId, id }; +}; + +test('a block says which one it is', () => { + const { id } = withBlock(); + + // Named by the same call the Outline names its rows by, so a block is not + // "Quarterly notes" in one place and "Markdown" in the other. + expect(screen.getByTestId(`block-title-${id}`)).toHaveTextContent( + 'Quarterly notes', + ); +}); + +test('the delete control does not have to be found first', () => { + const { id } = withBlock(); + + // It used to appear only on hover, which is a control you have to already + // know is there. `toBeVisible` fails on the opacity that hid it. + expect(screen.getByTestId(`block-remove-${id}`)).toBeVisible(); +}); + +test('removing a block is offered as a bin, not as a cross', () => { + const { id } = withBlock(); + + // A cross on a card is the gesture for dismissing the card — closing it, + // putting it away, getting it off screen. This takes the block off the + // dashboard, and the bin is what says that everywhere else in the app. + expect( + screen.getByTestId(`block-remove-${id}`).querySelector('.anticon-delete'), + ).toBeInTheDocument(); +}); + +test('the root carries no header of its own', () => { + const rootId = provider.getRoot().id; + render(); + + // The root is the dashboard rather than something on it: a header there + // would label it "Canvas" and offer a delete the provider refuses. + expect( + screen.queryByTestId(`block-header-${rootId}`), + ).not.toBeInTheDocument(); + expect( + screen.queryByTestId(`block-remove-${rootId}`), + ).not.toBeInTheDocument(); +}); + +test("a block's name reads as its title, not as a caption on it", () => { + const { id } = withBlock(); + + // Set in the secondary colour at the small size, it read as an annotation + // hanging above the block rather than as the name of the thing below it — + // which is what it is, and the first thing anyone scanning the canvas uses + // to tell one block from the next. + const title = screen.getByTestId(`block-title-${id}`); + + expect(title).toHaveStyle({ color: 'rgba(0, 0, 0, 0.88)' }); + // Compared rather than pinned: `fontWeightStrong` is a theme token, and it + // does not resolve to the same number here as it does in the app. Asserting + // the literal would be asserting the test theme's value, which is not the + // one that ships. + expect(Number(getComputedStyle(title).fontWeight)).toBeGreaterThan(400); +}); + +/** The element a node draws itself as — the card, for a block that has one. */ +const frameOf = (id: string) => + document.querySelector(`[data-node-id="${id}"]`) as HTMLElement; + +test('a block hides what it is drawn over, name and all', () => { + const { rootId, id } = withBlock(); + + // A free canvas lets blocks overlap, and only the leaf's own box was ever + // opaque — so a block raised to the front still showed whatever sat behind + // it through the strip carrying its name, and two overlapping blocks + // rendered their names on top of each other. + expect(frameOf(id)).toHaveStyle({ backgroundColor: '#FFFFFF' }); + // The root is the canvas everything is arranged on, not a card on it. + render(); + expect(frameOf(rootId)).not.toHaveStyle({ backgroundColor: '#FFFFFF' }); +}); + +test('a block is one card, with its name inside the frame rather than above it', () => { + const { id } = withBlock(); + + // The frame was drawn by the leaf, which begins below the header — so a + // card's top edge ran between a block's name and its contents, and the name + // read as a caption floating over a separate box rather than as the head of + // the card it belongs to. Drawn once, around both, it is one card. + const frame = frameOf(id); + expect(frame.style.border).toMatch(/^1px solid /); + expect(frame.style.borderRadius).not.toBe(''); + // Nothing can spill past the corners the frame rounds. + expect(frame).toHaveStyle({ overflow: 'hidden' }); + + // And the band no longer paints a surface of its own over the one it is on: + // two backgrounds meeting at the header's edge is the seam this removes. + expect(screen.getByTestId(`block-header-${id}`).style.backgroundColor).toBe( + '', + ); +}); + +test('a leaf block no longer frames itself, so there is one border and not two', () => { + const { id } = withBlock(); + + const leaf = screen.getByTestId(`block-content-${id}`) + .firstElementChild as HTMLElement; + expect(leaf.style.border).toBe(''); + expect(leaf.style.borderRadius).toBe(''); + expect(leaf.style.backgroundColor).toBe(''); +}); + +test('the header takes its height out of the block, not out of the canvas', () => { + const { rootId, id } = withBlock(); + + // A leaf block resolves `height: 100%` against this box — a chart measures + // the result to size its canvas — so the band above it has to come out of + // the height rather than be added to it, or every block overflows its cell + // by exactly the header. + expect(screen.getByTestId(`block-content-${id}`).style.height).toMatch( + /^calc\(100% - \d+px\)$/, + ); + // The root has no header to subtract. + render(); + expect(screen.getByTestId(`block-content-${rootId}`)).toHaveStyle({ + height: '100%', + }); +}); diff --git a/superset-frontend/src/core/dashboard/BuildingBlockView.tsx b/superset-frontend/src/core/dashboard/BuildingBlockView.tsx index 56764220514a..d0afe24fc45e 100644 --- a/superset-frontend/src/core/dashboard/BuildingBlockView.tsx +++ b/superset-frontend/src/core/dashboard/BuildingBlockView.tsx @@ -18,11 +18,13 @@ */ import { forwardRef, type HTMLAttributes } from 'react'; import { t } from '@apache-superset/core/translation'; -import { useTheme } from '@apache-superset/core/theme'; -import { Flex, Typography } from '@superset-ui/core/components'; +import { css, styled, useTheme } from '@apache-superset/core/theme'; +import { ActionButton, Flex, Typography } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; import { ErrorBoundary } from 'src/components'; import { provider, useDashboardRevision } from './store'; import { resolveBuildingBlockView } from './resolveBuildingBlockView'; +import { blockLabel } from './blockLabel'; function UnsupportedBlockPlaceholder({ nodeId }: { nodeId: string }) { const theme = useTheme(); @@ -50,6 +52,49 @@ function UnsupportedBlockPlaceholder({ nodeId }: { nodeId: string }) { ); } +/** + * A block's name, and what can be done to the block. + * + * Carries no surface of its own and no rule under it. The card behind this is + * opaque and unbroken, so a second background here would only draw a seam + * across it a hand's width below the top edge — the block would read as a + * strip and a box rather than as one card with a name on it. + */ +const BlockHeader = styled.div` + ${({ theme }) => css` + display: flex; + align-items: center; + gap: ${theme.sizeUnit}px; + height: ${theme.controlHeightSM}px; + /* Aligned with the inset every block's own content sits at, so a block's + name reads as the head of the box beneath it rather than as something + floating loose to its left. The right side stays tight: the remove + button is a square target of its own and centres its icon, which is the + inset it needs. */ + padding-left: ${theme.padding}px; + padding-right: ${theme.sizeUnit}px; + `} +`; + +/** + * What the remove control is wrapped in, and why it is wrapped at all. + * + * The control itself is `ActionButton` — the shared component for an icon + * action carried on a surface that is already something else, and the one the + * dashboard list uses for its own Delete. It takes an `onClick` with no event, + * so the two gestures this sits inside are stopped here instead: a click on + * the bin must remove rather than select the block it is drawn on, and a + * pointer down on it must not start a react-grid-layout drag. + * + * `data-block-remove` is the other half of that second one — `CanvasBlock` + * names it in `draggableCancel`, and react-grid-layout matches the selector up + * the ancestors, so carrying it here covers the button inside. + */ +const RemoveSlot = styled.span` + display: flex; + flex: 0 0 auto; +`; + interface BuildingBlockViewProps extends HTMLAttributes { nodeId: string; } @@ -86,14 +131,182 @@ interface BuildingBlockViewProps extends HTMLAttributes { const BuildingBlockView = forwardRef( function BuildingBlockView({ nodeId, children, ...rest }, ref) { useDashboardRevision(); + const theme = useTheme(); const node = provider.getNode(nodeId); if (!node) return null; const resolved = resolveBuildingBlockView(node.type, nodeId); + const selected = provider.getSelection() === nodeId; + // The root is the dashboard itself rather than something on it: it has no + // name of its own to show, and removing it is refused by the provider, so + // a header there would be a label saying "Canvas" over a button that only + // ever raises an error. + const chrome = nodeId !== provider.getRoot().id; + const isRoot = !chrome; + // The same token `BlockHeader` is drawn at: the content box below is this + // element's height minus the band, so the two have to be one number. + const headerHeight = theme.controlHeightSM; return ( -
-
+
{ + event.stopPropagation(); + provider.setSelection(nodeId); + }} + onKeyDown={event => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + event.stopPropagation(); + provider.setSelection(nodeId); + } + }} + style={{ + ...rest.style, + // A block's contents are positioned against this element. + // react-grid-layout positions its children itself, so its own value + // is kept wherever it set one and `relative` only fills the gap + // when it did not. + position: rest.style?.position ?? 'relative', + // The card, drawn around the whole of a block rather than around + // part of it. + // + // This used to be each leaf block's own — every one of them opened + // with the same background, border and radius — and a leaf begins + // below the header, so the card's top edge ran between a block's + // name and its contents. The name sat outside the box it names, + // reading as a caption dropped over a separate card. Drawn here it + // encloses both, which is also the only place it can be drawn from: + // whether a node has a header at all is this component's to know, + // not the leaf's. + // + // Opaque for the same reason it is one card: on a free canvas + // blocks overlap, and anything a block does not paint is a window + // onto whatever is behind it. + backgroundColor: isRoot ? undefined : theme.colorBgContainer, + border: `1px solid ${theme.colorBorderSecondary}`, + borderRadius: theme.borderRadiusLG, + // Nothing reaches past the corners this rounds — a block's content + // is square and would otherwise fill them back in. + overflow: isRoot ? undefined : 'hidden', + // The dashboard's own gutter, and what makes it clickable. + // + // The root is drawn by a grid that fills its box edge to edge + // (`containerPadding={[0, 0]}` in CanvasBlock), which left the + // dashboard with no pixels of its own. The inset is what an author + // aims at to select the dashboard rather than something on it — and + // it is the same inset a block's content sits at, so the two read as + // one scale rather than two. It carries no surface of its own: this + // is what everything else is arranged *on*, not a card among them. + padding: isRoot ? theme.padding : undefined, + // Drawn over the block rather than around it: an outline takes no + // space, so nothing on screen shifts when a selection moves. + outline: selected ? `2px solid ${theme.colorPrimary}` : undefined, + outlineOffset: selected ? -2 : undefined, + }} + > + {/* What this block is, and how to be rid of it. + The name comes from `blockLabel`, the same call the Outline names + a row by, so a block is not "Sales by Territory" in one place and + "ECharts" in the other. A chart's name is authored in its ECharts + option and ChartBlock stops ECharts drawing it, so it appears here + once instead of twice. + + `data-block-remove` is what keeps a press on the button from + starting a react-grid-layout drag; see CanvasBlock's + `draggableCancel`. The propagation stops are the same idea for the + two gestures it sits inside: a click here removes rather than + selects, and a pointer down here grabs nothing. + + The button is nested inside a control, which is not ideal and is + the price of the wrapper itself being selectable — the alternative + was a block you can delete only from the panel. The keyboard path + is not this button: the Outline selects any block with proper tree + semantics and Properties carries the same Delete. */} + {chrome && ( + + + {blockLabel(node.type, node.props)} + + event.stopPropagation()} + onPointerDown={event => event.stopPropagation()} + onClick={event => event.stopPropagation()} + > + provider.removeBuildingBlock(nodeId)} + // A bin rather than a cross. A cross on a card is the gesture + // for dismissing the card — closing it, putting it away — and + // this does not put the block away, it takes it off the + // dashboard. The bin is what the rest of the app uses to say + // so, and it is the same act the panel offers as Delete. + // + // Quiet at rest and primary under the pointer, which is + // `ActionButton`'s own behaviour and the same answer the + // dashboard list gives for its Delete: a bin on every block, + // all of them lit red, would make a canvas read as a row of + // things about to be deleted. + icon={} + /> + + + )} + {/* The block's own box, which is the whole of this element's minus + the band above it. Subtracted in pixels off a percentage rather + than left to a flex column, because what a leaf block does with + the box is resolve `height: 100%` against it — a chart measures + the result to size its canvas — and that wants a height there is + no question about. */} +
{resolved ?? } diff --git a/superset-frontend/src/core/dashboard/DashboardProvider.test.ts b/superset-frontend/src/core/dashboard/DashboardProvider.test.ts index 887f1a058646..313add0e7182 100644 --- a/superset-frontend/src/core/dashboard/DashboardProvider.test.ts +++ b/superset-frontend/src/core/dashboard/DashboardProvider.test.ts @@ -140,6 +140,90 @@ test('moveBuildingBlock relocates a node to a new parent at the given index', () expect(provider.getNode(canvasId)?.children).toEqual([id]); }); +test('moveBuildingBlock keeps an explicit position when the parent is unchanged', () => { + const provider = DashboardProvider.getInstance(); + const rootId = provider.getRoot().id; + const first = provider.addBuildingBlock(rootId, 0, { type: 'text' }); + const second = provider.addBuildingBlock(rootId, 1, { type: 'text' }); + provider.updateLayout(first, { col: 3, row: 2, colSpan: 6 }); + + // Reordering within one parent is how a free canvas expresses "bring this + // to the front" — the node's own placement is not part of what changed. + provider.moveBuildingBlock(first, rootId, 1); + + expect(provider.getRoot().children).toEqual([second, first]); + expect(provider.getNode(first)?.layout).toMatchObject({ + col: 3, + row: 2, + colSpan: 6, + }); +}); + +test('bringToFront makes a node the last of its siblings, where it is drawn over them', () => { + const provider = DashboardProvider.getInstance(); + const rootId = provider.getRoot().id; + const first = provider.addBuildingBlock(rootId, 0, { type: 'text' }); + const second = provider.addBuildingBlock(rootId, 1, { type: 'text' }); + const third = provider.addBuildingBlock(rootId, 2, { type: 'text' }); + provider.updateLayout(first, { col: 3, row: 2 }); + + provider.bringToFront(first); + + expect(provider.getRoot().children).toEqual([second, third, first]); + // Raising a block is not moving it. + expect(provider.getNode(first)?.layout).toMatchObject({ col: 3, row: 2 }); +}); + +test('sendToBack makes a node the first of its siblings', () => { + const provider = DashboardProvider.getInstance(); + const rootId = provider.getRoot().id; + const first = provider.addBuildingBlock(rootId, 0, { type: 'text' }); + const second = provider.addBuildingBlock(rootId, 1, { type: 'text' }); + + provider.sendToBack(second); + + expect(provider.getRoot().children).toEqual([second, first]); +}); + +test('raising a node already in front changes nothing, and says nothing changed', () => { + const provider = DashboardProvider.getInstance(); + const rootId = provider.getRoot().id; + provider.addBuildingBlock(rootId, 0, { type: 'text' }); + const last = provider.addBuildingBlock(rootId, 1, { type: 'text' }); + const revisionBefore = provider.getRevision(); + + provider.bringToFront(last); + + // A drag that ends on the block already in front is the common case, and + // a revision tick for it re-renders every subscriber for nothing. + expect(provider.getRevision()).toBe(revisionBefore); +}); + +test('bringToFront is a no-op for the root, which has no siblings', () => { + const provider = DashboardProvider.getInstance(); + const revisionBefore = provider.getRevision(); + + expect(() => provider.bringToFront(provider.getRoot().id)).not.toThrow(); + expect(provider.getRevision()).toBe(revisionBefore); +}); + +test('getParentId returns the canvas holding a node', () => { + const provider = DashboardProvider.getInstance(); + const rootId = provider.getRoot().id; + const canvasId = provider.addBuildingBlock(rootId, 0, { type: 'canvas' }); + const childId = provider.addBuildingBlock(canvasId, 0, { type: 'text' }); + + expect(provider.getParentId(childId)).toBe(canvasId); + expect(provider.getParentId(canvasId)).toBe(rootId); +}); + +test('getParentId returns undefined for the root and for an unknown id', () => { + const provider = DashboardProvider.getInstance(); + + expect(provider.getParentId(provider.getRoot().id)).toBeUndefined(); + expect(provider.getParentId('missing')).toBeUndefined(); +}); + test('moveBuildingBlock throws when moving the root node', () => { const provider = DashboardProvider.getInstance(); const rootId = provider.getRoot().id; @@ -331,3 +415,60 @@ test('getRevision increments on every mutation and is stable otherwise', () => { expect(provider.getRevision()).toBe(before + 1); }); + +/** + * Selection is host-internal state, like the revision counter: a property of + * one person looking at one screen, not of the dashboard. + */ +test('selecting a node reports it back', () => { + const provider = DashboardProvider.getInstance(); + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'markdown', + }); + + provider.setSelection(id); + + expect(provider.getSelection()).toBe(id); +}); + +test('removing the selected node clears the selection', () => { + const provider = DashboardProvider.getInstance(); + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'markdown', + }); + provider.setSelection(id); + + provider.removeBuildingBlock(id); + + // A selection is a reference to a node, and a node that is gone cannot be + // the thing being edited — an inspector reading a dangling id would show a + // block that no longer exists. + expect(provider.getSelection()).toBeUndefined(); +}); + +test('removing a container clears a selection inside its subtree', () => { + const provider = DashboardProvider.getInstance(); + const sectionId = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'canvas', + }); + const childId = provider.addBuildingBlock(sectionId, 0, { type: 'markdown' }); + provider.setSelection(childId); + + provider.removeBuildingBlock(sectionId); + + // The node that vanished was a descendant of the one actually removed, + // which is why the check belongs in the commit rather than at the removal. + expect(provider.getSelection()).toBeUndefined(); +}); + +test('reset clears the selection along with the tree', () => { + const provider = DashboardProvider.getInstance(); + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'markdown', + }); + provider.setSelection(id); + + provider.reset(); + + expect(provider.getSelection()).toBeUndefined(); +}); diff --git a/superset-frontend/src/core/dashboard/DashboardProvider.ts b/superset-frontend/src/core/dashboard/DashboardProvider.ts index 4cc7a9afe0b0..e7475c485b90 100644 --- a/superset-frontend/src/core/dashboard/DashboardProvider.ts +++ b/superset-frontend/src/core/dashboard/DashboardProvider.ts @@ -31,6 +31,20 @@ type StoredNode = Omit; const ROOT_ID = 'root'; +/** + * The one node type that holds other nodes. + * + * Named here because two things need to agree on it and neither should learn + * it by string comparison of its own: this provider, deciding whether a new + * node gets a `children` array at all, and the palette, deciding whether a + * block an author places is a container or something to put in one. + */ +export const CONTAINER_TYPE = 'canvas'; + +/** Whether placing this type produces something other nodes can go inside. */ +export const isContainerType = (type: string): boolean => + type === CONTAINER_TYPE; + function createBlankNodes(): Record { return { [ROOT_ID]: { @@ -68,6 +82,22 @@ class DashboardProvider { private revision = 0; + /** + * Which node the author is working on. + * + * Host-internal, exactly like {@link getRevision} and for the same reason: + * it is a property of one person looking at one screen, not of the + * dashboard. Two people opening the same tree select different things, and + * nothing about a selection belongs in a document or in the public API an + * extension calls. + * + * It lives here rather than in page state because the canvas draws it and + * the editor panel reads it, and those sit in different layers — putting it + * in the one place both already subscribe to beats threading it through the + * render tree that `BuildingBlockView` deliberately keeps ignorant. + */ + private selection: string | undefined; + private layoutChangeEmitter = createEventEmitter(); private stateSubscribers = new Set<() => void>(); @@ -86,7 +116,31 @@ class DashboardProvider { public getRevision = (): number => this.revision; + public getSelection = (): string | undefined => this.selection; + + /** + * Selects a node, or clears the selection with `undefined`. + * + * Ticks the same revision every mutation does, so everything already + * subscribed re-reads without needing a second subscription of its own. + */ + public setSelection = (id: string | undefined): void => { + if (this.selection === id) { + return; + } + this.selection = id; + this.revision += 1; + this.stateSubscribers.forEach(fn => fn()); + }; + private commit(nodes: Record): void { + // A selection is a reference to a node, and a node that is gone cannot be + // the thing being edited. Clearing it here — rather than at each removal + // site — covers a subtree deletion too, where the node that vanished was + // a descendant of the one actually removed. + if (this.selection !== undefined && !nodes[this.selection]) { + this.selection = undefined; + } this.nodes = nodes; this.revision += 1; this.layoutChangeEmitter.fire(); @@ -102,6 +156,19 @@ class DashboardProvider { public getNode = (id: string): DashboardNode | undefined => this.toNode(id); + /** + * The canvas a node sits in, or `undefined` for the root and for a node + * that is not in the tree. + * + * {@link moveBuildingBlock} takes the destination parent as an argument, so + * every caller that moves a node already has to know which parent it is in + * — a caller reordering a node within its own container most of all. The + * walk itself is one line, and leaving it out meant each caller wrote that + * line again over a `nodes` map only this class is supposed to hold. + */ + public getParentId = (id: string): string | undefined => + this.findParentId(id, this.nodes); + /** True if `targetId` is `nodeId` itself or nested somewhere in its subtree. */ private isNodeOrDescendant(nodeId: string, targetId: string): boolean { if (nodeId === targetId) return true; @@ -180,7 +247,7 @@ class DashboardProvider { layout: spec.layout, props: spec.props, style: spec.style, - ...(spec.type === 'canvas' ? { children: [] } : {}), + ...(isContainerType(spec.type) ? { children: [] } : {}), }; const children = [...parent.children]; @@ -245,6 +312,8 @@ class DashboardProvider { ); } + const oldParentId = this.findParentId(id, this.nodes); + const nodes = { ...this.nodes }; Object.entries(nodes).forEach(([parentId, parent]) => { if (parent.children?.includes(id)) { @@ -268,24 +337,68 @@ class DashboardProvider { // drag-based reparenting (see `CanvasBlock`'s `handleDragStop`) already // resets exactly these two things on drop; this is that same reset, // applied here so the programmatic path gives the same guarantee. - const node = nodes[id]; - const destColumns = targetParent.layout?.columns ?? DEFAULT_COLUMNS; - nodes[id] = { - ...node, - layout: { - ...node.layout, - col: undefined, - row: undefined, - colSpan: - node.layout?.colSpan != null - ? Math.min(node.layout.colSpan, destColumns) - : undefined, - }, - }; + // + // None of which is true when the parent has not changed. A move within + // one container is a reorder — how a free canvas says "put this in + // front", since paint order there is child order — and the position it + // keeps is the one the author placed it at. Resetting it would teleport + // the block to auto-placement as the price of raising it. + if (oldParentId !== newParentId) { + const node = nodes[id]; + const destColumns = targetParent.layout?.columns ?? DEFAULT_COLUMNS; + nodes[id] = { + ...node, + layout: { + ...node.layout, + col: undefined, + row: undefined, + colSpan: + node.layout?.colSpan != null + ? Math.min(node.layout.colSpan, destColumns) + : undefined, + }, + }; + } this.commit(nodes); } + /** + * Which of its siblings a node is drawn over. + * + * Where children overlap — a `free` canvas — the container's child order is + * the paint order, because `react-grid-layout` gives an overlapping item no + * `z-index` of its own and the browser falls back to tree order. So "in + * front" is "last", and raising a block is reordering it. + * + * Named for what an author means rather than left to callers to express as + * an index, because the index is a trap: {@link moveBuildingBlock} detaches + * before it inserts, so the array a node lands in is one shorter than the + * one that was counted. Every caller getting that arithmetic right + * separately is a bug waiting for the second caller. + * + * Doing nothing when there is nothing to do is part of the contract, not an + * optimisation. A drag ends on the block already in front more often than + * not, and a revision tick there re-renders every subscriber to produce the + * array it already had. + */ + private restack(id: string, edge: 'front' | 'back'): void { + const parentId = this.findParentId(id, this.nodes); + if (parentId === undefined) return; + const children = this.nodes[parentId]?.children ?? []; + const settled = edge === 'front' ? children.at(-1) : children[0]; + if (settled === id) return; + this.moveBuildingBlock( + id, + parentId, + edge === 'front' ? children.length : 0, + ); + } + + public bringToFront = (id: string): void => this.restack(id, 'front'); + + public sendToBack = (id: string): void => this.restack(id, 'back'); + public updateLayout(id: string, layout: Partial): void { const node = this.nodes[id]; if (!node) { @@ -348,6 +461,7 @@ class DashboardProvider { /** Test/demo helper — discards all nodes back to a blank canvas. */ public reset(): void { this.nodes = createBlankNodes(); + this.selection = undefined; this.revision = 0; this.layoutChangeEmitter = createEventEmitter(); this.stateSubscribers.clear(); diff --git a/superset-frontend/src/core/dashboard/blockLabel.test.ts b/superset-frontend/src/core/dashboard/blockLabel.test.ts new file mode 100644 index 000000000000..2b7b74f8e38a --- /dev/null +++ b/superset-frontend/src/core/dashboard/blockLabel.test.ts @@ -0,0 +1,73 @@ +/** + * 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 { registerBuiltInBuildingBlocks } from './registerBuiltInBuildingBlocks'; +import { blockLabel } from './blockLabel'; + +beforeAll(() => { + registerBuiltInBuildingBlocks(); +}); + +test('a chart is named by the title its author wrote into the option', () => { + expect( + blockLabel('echarts', { + echartsOptions: { title: { text: 'Sales by Territory' } }, + }), + ).toBe('Sales by Territory'); +}); + +test('a chart carrying several titles is named by the first', () => { + // ECharts takes one title or a list of them; the first is the chart's and + // the rest annotate parts of it. + expect( + blockLabel('echarts', { + echartsOptions: { title: [{ text: 'Revenue' }, { text: 'Units' }] }, + }), + ).toBe('Revenue'); +}); + +test('a metric tile is named by the label it displays', () => { + expect(blockLabel('metric-tile', { label: 'Total Revenue' })).toBe( + 'Total Revenue', + ); +}); + +test('markdown is named by its opening words', () => { + expect( + blockLabel('markdown', { content: '# Acme Corp\n\nGenerated November' }), + ).toBe('# Acme Corp Generated November'); +}); + +test('a block with no name of its own is named by what it is', () => { + // "Table" says what a block is rather than which one it is — worth little, + // and still better than an empty header. + expect(blockLabel('ag-grid-table', {})).toBe('Table'); + expect(blockLabel('echarts', undefined)).toBe('ECharts'); +}); + +test('a name of nothing but spaces is no name', () => { + expect( + blockLabel('echarts', { echartsOptions: { title: { text: ' ' } } }), + ).toBe('ECharts'); + expect(blockLabel('markdown', { content: '\n\n' })).toBe('Markdown'); +}); + +test('a type nothing registered still says something', () => { + // An extension's block whose registration failed, or arrived late. + expect(blockLabel('acme-widget', undefined)).toBe('acme-widget'); +}); diff --git a/superset-frontend/src/core/dashboard/blockLabel.ts b/superset-frontend/src/core/dashboard/blockLabel.ts new file mode 100644 index 000000000000..1eea5ea1adf7 --- /dev/null +++ b/superset-frontend/src/core/dashboard/blockLabel.ts @@ -0,0 +1,83 @@ +/** + * 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. + */ + +/** + * @fileoverview What a block is called, in the one place every panel that + * names one can reach. + * + * A block is named in more than one part of the editor — its own header on + * the canvas, its row in the outline — and those have to agree. A block + * called "Sales by Territory" in one and "ECharts" in the other reads as two + * different blocks. + */ + +import { views } from 'src/core/views'; +import { DASHBOARD_BUILDING_BLOCKS_LOCATION } from './resolveBuildingBlockView'; + +type Props = Record | undefined; + +/** + * The ECharts option's own title, which is where a chart's name is authored. + * + * ECharts accepts either one title or an array of them; the first is the + * chart's, and any others annotate parts of it. + */ +const echartsTitle = (props: Props): unknown => { + const title = (props?.echartsOptions as { title?: unknown } | undefined) + ?.title; + const first = Array.isArray(title) ? title[0] : title; + return (first as { text?: unknown } | undefined)?.text; +}; + +/** + * Where a block type carries a name of its own. + * + * Only the types that have one: everything else is named by its registration, + * and an unlisted type — an extension's — falls through to that without + * needing to be known here. + */ +const NAMED_BY: Record unknown> = { + markdown: props => props?.content, + echarts: echartsTitle, + 'metric-tile': props => props?.label, +}; + +/** + * What to call the block of `type` holding `props`. + * + * A name the block's own content carries wins, because that is the name its + * author gave it and the one they will look for. Only when there is none does + * this fall back to the registered block name — "Markdown", "Table" — which + * says what a block is rather than which one it is, and is worth nothing at + * all when five of them sit in a column. + * + * Returned whole: how much of a long name fits is the caller's business, + * since a row in a panel and a header on a wide chart cut at different + * points. + */ +export function blockLabel(type: string, props: Props): string { + const own = NAMED_BY[type]?.(props); + if (typeof own === 'string' && own.trim() !== '') { + return own.trim().replace(/\s+/g, ' '); + } + const registered = views + .getViews(DASHBOARD_BUILDING_BLOCKS_LOCATION) + ?.find(view => view.id === type); + return registered?.name ?? type; +} diff --git a/superset-frontend/src/core/dashboard/blocks/AgGridTableBlock.tsx b/superset-frontend/src/core/dashboard/blocks/AgGridTableBlock.tsx index e3616e0da4b0..9b1b6f8d039c 100644 --- a/superset-frontend/src/core/dashboard/blocks/AgGridTableBlock.tsx +++ b/superset-frontend/src/core/dashboard/blocks/AgGridTableBlock.tsx @@ -105,9 +105,9 @@ export default function AgGridTableBlock({ nodeId }: { nodeId: string }) { // block — always a definite pixel box, same as `ChartBlock`. width: '100%', height: '100%', - backgroundColor: theme.colorBgContainer, - border: `1px solid ${theme.colorBorderSecondary}`, - borderRadius: theme.borderRadiusLG, + // Surface, border and corners belong to the card `BuildingBlockView` + // draws around this block and the name above it, so that the name is + // inside the frame rather than over it. overflow: 'hidden', }} > diff --git a/superset-frontend/src/core/dashboard/blocks/CanvasBlock.test.tsx b/superset-frontend/src/core/dashboard/blocks/CanvasBlock.test.tsx new file mode 100644 index 000000000000..2e44be78c988 --- /dev/null +++ b/superset-frontend/src/core/dashboard/blocks/CanvasBlock.test.tsx @@ -0,0 +1,319 @@ +/** + * 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 { + fireEvent, + render, + screen, + within, +} from 'spec/helpers/testing-library'; +import DashboardProvider from '../DashboardProvider'; +import CanvasBlock from './CanvasBlock'; + +/** + * Which renderer a container gets, and what a gesture in it commits. + * + * `react-grid-layout` is mocked down to the props that decide the two grid + * modes apart. Everything about how it draws is its own business and covered + * by its own tests; what matters here is that `free` reaches it with + * compaction off and overlap allowed, and that `grid` does not — because + * that single pair of props is the whole difference between "the space above + * a block closes" and "a block stays where it was put". + */ +jest.mock('react-grid-layout/legacy', () => ({ + __esModule: true, + default: ({ + children, + compactType, + allowOverlap, + draggableCancel, + resizeHandles, + }: { + children: React.ReactNode; + compactType: string | null; + allowOverlap?: boolean; + draggableCancel?: string; + resizeHandles?: string[]; + }) => ( +
+ {children} +
+ ), + WidthProvider: (component: unknown) => component, +})); +jest.mock('react-grid-layout/css/styles.css', () => ({}), { virtual: true }); + +const provider = DashboardProvider.getInstance(); + +beforeEach(() => { + provider.reset(); +}); + +const withMode = (mode?: 'grid' | 'free' | 'flex') => { + const rootId = provider.getRoot().id; + if (mode) { + provider.updateLayout(rootId, { mode }); + } + const first = provider.addBuildingBlock(rootId, 0, { type: 'markdown' }); + const second = provider.addBuildingBlock(rootId, 1, { type: 'markdown' }); + render(); + return { rootId, first, second }; +}; + +test('a grid compacts its children and does not let them overlap', () => { + withMode('grid'); + + const grid = screen.getByTestId('rgl'); + expect(grid).toHaveAttribute('data-compact-type', 'vertical'); + expect(grid).toHaveAttribute('data-allow-overlap', 'false'); +}); + +test('the corner a block removes itself from is not also a resize handle', () => { + withMode('grid'); + + // react-grid-layout appends its handles after a block's own content, so the + // north-east one landed on the remove control and took every click aimed at + // it -- `elementFromPoint` at that button's centre returned the handle. A + // handle nobody can grab is worse than no handle: the corner still looks + // resizable, and answers a drag that starts a pixel to either side. + expect(screen.getByTestId('rgl')).toHaveAttribute( + 'data-resize-handles', + 'se,sw,nw', + ); +}); + +test('a container that named no mode is drawn as a grid', () => { + withMode(); + + expect(screen.getByTestId('rgl')).toHaveAttribute( + 'data-compact-type', + 'vertical', + ); +}); + +test('a free canvas turns compaction off and allows overlap', () => { + withMode('free'); + + // `allowOverlap` is what makes a free canvas work. Passing `compactType` + // null on its own leaves react-grid-layout's collision resolution running + // with nothing to settle it, which is the runaway displacement recorded in + // CanvasBlock — a free canvas must never reach that path. + const grid = screen.getByTestId('rgl'); + expect(grid).toHaveAttribute('data-compact-type', 'null'); + expect(grid).toHaveAttribute('data-allow-overlap', 'true'); +}); + +test('a flex container is not a grid at all', () => { + withMode('flex'); + + // A flex line has no cells to give react-grid-layout coordinates in, so + // this is a different renderer rather than the same one configured + // differently. + expect(screen.getByTestId('flex-canvas')).toBeInTheDocument(); + expect(screen.queryByTestId('rgl')).not.toBeInTheDocument(); +}); + +test('a flex child hands its block a definite box', () => { + const rootId = provider.getRoot().id; + provider.updateLayout(rootId, { mode: 'flex' }); + const id = provider.addBuildingBlock(rootId, 0, { + type: 'markdown', + layout: { rowSpan: 4 }, + }); + render(); + + // Every leaf block fills the box its placement wrapper gives it — a chart + // measures that box to size its canvas, and markdown scrolls inside it. In + // a grid, react-grid-layout supplies the box by cloning the block with an + // explicit pixel width and height. A flex container positions its own + // children, so it has to hand the same box down itself; without it the + // block is content-height, the chart's measured height collapses, and + // markdown taller than its share paints over the row beneath it. + const block = within(screen.getByTestId(`flex-child-${id}`)).getByRole( + 'button', + { name: 'markdown' }, + ); + expect(block).toHaveStyle({ width: '100%', height: '100%' }); +}); + +test('a flex child is as tall as the same block in a grid', () => { + const rootId = provider.getRoot().id; + provider.updateLayout(rootId, { mode: 'flex' }); + const id = provider.addBuildingBlock(rootId, 0, { + type: 'markdown', + layout: { rowSpan: 4 }, + }); + render(); + + // react-grid-layout reserves the rows *and the gaps between them* + // (`rowUnit * rowSpan + (rowSpan - 1) * gap`), so 4 rows of 32 with a gap + // of 16 is 176px, not 128. Counting only the rows would make every block on + // the canvas shrink the moment the mode changed. + expect(screen.getByTestId(`flex-child-${id}`)).toHaveStyle({ + height: '176px', + }); +}); + +test('dragging one flex child onto another reorders them', () => { + const { rootId, first, second } = withMode('flex'); + const data = new Map(); + const dataTransfer = { + setData: (type: string, value: string) => data.set(type, value), + getData: (type: string) => data.get(type) ?? '', + effectAllowed: '', + }; + + fireEvent.dragStart(screen.getByTestId(`flex-child-${second}`), { + dataTransfer, + }); + fireEvent.drop(screen.getByTestId(`flex-child-${first}`), { dataTransfer }); + + // Position in a flex container is order, so the gesture that arranges one + // is a reorder — and it commits through the same moveBuildingBlock the AI + // tools call. + expect(provider.getNode(rootId)?.children).toEqual([second, first]); +}); + +test('a flex child dropped on itself changes nothing', () => { + const { rootId, first, second } = withMode('flex'); + const data = new Map(); + const dataTransfer = { + setData: (type: string, value: string) => data.set(type, value), + getData: (type: string) => data.get(type) ?? '', + effectAllowed: '', + }; + + fireEvent.dragStart(screen.getByTestId(`flex-child-${first}`), { + dataTransfer, + }); + fireEvent.drop(screen.getByTestId(`flex-child-${first}`), { dataTransfer }); + + expect(provider.getNode(rootId)?.children).toEqual([first, second]); +}); + +/** A drag payload jsdom's synthetic events do not carry on their own. */ +const paletteTransfer = (type: string) => { + const data = new Map([['application/x-dashboard-building-block', type]]); + return { + types: [...data.keys()], + getData: (key: string) => data.get(key) ?? '', + setData: (key: string, value: string) => data.set(key, value), + dropEffect: '', + effectAllowed: '', + }; +}; + +test('dropping a palette block on a container places it there', () => { + const { rootId } = withMode('grid'); + + fireEvent.drop(screen.getByTestId('canvas-container'), { + dataTransfer: paletteTransfer('markdown'), + }); + + const children = provider.getNode(rootId)?.children ?? []; + expect(children).toHaveLength(3); + expect(provider.getNode(children[2])?.type).toBe('markdown'); +}); + +test('a drop into a flex container lands there too', () => { + const { rootId } = withMode('flex'); + + fireEvent.drop(screen.getByTestId('flex-canvas'), { + dataTransfer: paletteTransfer('echarts'), + }); + + const children = provider.getNode(rootId)?.children ?? []; + expect(provider.getNode(children[children.length - 1])?.type).toBe('echarts'); +}); + +test('a drop carrying something else is not read as a block', () => { + const { rootId } = withMode('grid'); + const before = provider.getNode(rootId)?.children?.length; + + fireEvent.drop(screen.getByTestId('canvas-container'), { + dataTransfer: { + types: ['text/plain'], + getData: () => '', + dropEffect: '', + effectAllowed: '', + }, + }); + + // A private type rather than text/plain is what keeps a dragged file, or a + // selection of text from another window, from placing a block. + expect(provider.getNode(rootId)?.children?.length).toBe(before); +}); + +test('a placed block offers a way to remove it, and the root does not', () => { + const { rootId, first } = withMode('grid'); + + expect(screen.getByTestId(`block-remove-${first}`)).toBeInTheDocument(); + // Removing the root is refused by the provider, so offering the button + // would be offering an error. + expect( + screen.queryByTestId(`block-remove-${rootId}`), + ).not.toBeInTheDocument(); +}); + +test('the remove control removes that block and nothing else', () => { + const { rootId, first, second } = withMode('grid'); + + fireEvent.click(screen.getByTestId(`block-remove-${first}`)); + + expect(provider.getNode(rootId)?.children).toEqual([second]); +}); + +test('the grid is told not to start a drag from the remove control', () => { + const { first } = withMode('grid'); + + // react-grid-layout begins a drag on a press anywhere in the block it is + // positioning, and the button sits inside that block. `draggableCancel` is + // what it reads to exclude a region, so the selector and the attribute the + // control carries have to agree — aiming at the bin would otherwise drag the + // block it is attached to. + const cancel = screen + .getByTestId('rgl') + .getAttribute('data-draggable-cancel'); + expect(cancel).toContain('[data-block-remove]'); + // On the control or above it: react-draggable matches the selector against + // the pressed element and then walks its ancestors up to the grid item, so + // the attribute excludes the whole region it is set on. The bin is the + // shared `ActionButton`, which renders its own element and forwards no + // arbitrary attributes to it — the region is what carries this. + expect( + screen.getByTestId(`block-remove-${first}`).closest('[data-block-remove]'), + ).not.toBeNull(); +}); + +test('clicking the remove control removes rather than selects', () => { + const { first, second } = withMode('grid'); + + fireEvent.click(screen.getByTestId(`block-remove-${second}`)); + + // The wrapper selects on click and the button sits inside it. Without the + // stop, removing a block would also try to select the thing just removed. + expect(provider.getSelection()).toBeUndefined(); + expect(provider.getNode(second)).toBeUndefined(); + expect(provider.getNode(first)).toBeDefined(); +}); diff --git a/superset-frontend/src/core/dashboard/blocks/CanvasBlock.tsx b/superset-frontend/src/core/dashboard/blocks/CanvasBlock.tsx index 380beeed766f..adcc985ade7f 100644 --- a/superset-frontend/src/core/dashboard/blocks/CanvasBlock.tsx +++ b/superset-frontend/src/core/dashboard/blocks/CanvasBlock.tsx @@ -27,9 +27,11 @@ import 'react-grid-layout/css/styles.css'; import type { dashboard as dashboardApi } from '@apache-superset/core'; import { useTheme } from '@apache-superset/core/theme'; import { provider, useDashboardRevision } from '../store'; -import { resolveGridMetrics } from '../layoutStyle'; +import { resolveGridMetrics, resolveLayoutMode } from '../layoutStyle'; import { packChildLayout } from '../gridPacking'; +import { PALETTE_MIME, placeBlock } from '../placement'; import BuildingBlockView from '../BuildingBlockView'; +import FlexCanvas from './FlexCanvas'; type LayoutProps = dashboardApi.LayoutProps; @@ -197,14 +199,50 @@ export default function CanvasBlock({ nodeId }: { nodeId: string }) { } commitLayout(rglLayout); + + // A block dragged over its siblings is a block meant to be in front of + // them, and on a free canvas that has to be said in the document. + // `react-grid-layout` floats the dragged item with a `z-index: 3` it + // carries on a class while the pointer is down and loses on release — + // so the gesture shows the block on top the whole way, then drops it + // behind whatever it was dragged over. The lift was only ever CSS. + // + // Only where children overlap. Every other mode arranges them so they + // cannot, and reordering there would rewrite reading order, tab order + // and the Outline to change nothing anyone can see. + if (newItem && resolveLayoutMode(node?.layout) === 'free') { + provider.bringToFront(newItem.i); + } }, - [nodeId, commitLayout], + [nodeId, node?.layout, commitLayout], ); if (!node) return null; + const mode = resolveLayoutMode(node.layout); const { columns, gap, rowUnitPx } = resolveGridMetrics(node.layout, theme); const children = node.children ?? []; + + // A flex line has no cells, so it is not a grid with different settings — + // it is a different renderer. Everything below this point is grid geometry. + if (mode === 'flex') { + return ( + + ); + } + + /** + * `free` is `grid` with the compaction turned off. + * + * `allowOverlap` is what makes that work, and it is not the same thing as + * the `compactType={null}` recorded below as unusable: that path keeps + * react-grid-layout's collision resolution running with nothing to settle + * it, which is exactly the runaway displacement it was found to cause. + * `allowOverlap` removes the collision resolution instead, which is what a + * free canvas means — a block stays where it was put, over a sibling if + * that is where the author put it. + */ + const free = mode === 'free'; const packed = packChildLayout(children, columns, provider.getNode); const layout: Layout = children.map(id => ({ i: id, @@ -216,6 +254,26 @@ export default function CanvasBlock({ nodeId }: { nodeId: string }) { return (
{ + if (event.dataTransfer.types.includes(PALETTE_MIME)) { + event.preventDefault(); + event.dataTransfer.dropEffect = 'copy'; + } + }} + onDrop={event => { + const type = event.dataTransfer.getData(PALETTE_MIME); + if (type !== '') { + event.preventDefault(); + event.stopPropagation(); + placeBlock(nodeId, type); + } + }} style={{ width: '100%', height: '100%', overflow: 'auto' }} > ({ + __esModule: true, + use: jest.fn(), + init: jest.fn(() => ({ + setOption: mockSetOption, + resize: jest.fn(), + dispose: jest.fn(), + })), +})); + +jest.mock('../chartData', () => ({ + __esModule: true, + fetchQueryData: jest.fn(async () => ({ rows: [{ x: 'a', y: 1 }] })), +})); + +/** + * The stock test double never calls back, so nothing this component draws is + * ever measured. ECharts has no self-sizing — it draws what it is told to + * resize to — so a size has to arrive for the canvas to exist at all. + */ +beforeAll(() => { + window.ResizeObserver = class { + constructor(private callback: ResizeObserverCallback) {} + + observe() { + this.callback( + [{ contentRect: { width: 400, height: 300 } } as ResizeObserverEntry], + this as unknown as ResizeObserver, + ); + } + + unobserve() {} + + disconnect() {} + }; +}); + +const provider = DashboardProvider.getInstance(); + +beforeEach(() => { + provider.reset(); + mockSetOption.mockClear(); +}); + +test('a chart does not draw the name its header already carries', async () => { + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'echarts', + props: { + dataBinding: { datasource: 1, columns: ['x'], metrics: [] }, + echartsOptions: { + title: { text: 'Sales by Territory' }, + series: [{ type: 'bar' }], + }, + }, + }); + render(); + + await waitFor(() => expect(mockSetOption).toHaveBeenCalled()); + + // `blockLabel` reads the title out of this same option to name the block, + // so leaving it here would print the chart's name twice, at two sizes, in + // two places. The rest of the option has to survive untouched. + const [option] = mockSetOption.mock.calls[0]; + expect(option).not.toHaveProperty('title'); + expect(option).toHaveProperty('series'); +}); diff --git a/superset-frontend/src/core/dashboard/blocks/ChartBlock.tsx b/superset-frontend/src/core/dashboard/blocks/ChartBlock.tsx index 3225e0a1f204..b54007936da2 100644 --- a/superset-frontend/src/core/dashboard/blocks/ChartBlock.tsx +++ b/superset-frontend/src/core/dashboard/blocks/ChartBlock.tsx @@ -223,19 +223,20 @@ export default function ChartBlock({ nodeId }: { nodeId: string }) { // eslint-disable-next-line react-hooks/exhaustive-deps }, [bindingKey]); - const option = useMemo( - () => - rows - ? resolveBindings( - (node?.props?.echartsOptions as Record) ?? {}, - { - rows, - theme, - }, - ) - : undefined, - [node?.props?.echartsOptions, rows, theme], - ); + const option = useMemo(() => { + if (!rows) return undefined; + const resolved = resolveBindings( + (node?.props?.echartsOptions as Record) ?? {}, + { rows, theme }, + ); + // The chart's name is drawn by the block's header, which reads it from + // this same option (see `blockLabel`). Leaving it here too would print it + // twice, at two sizes, in two places — and the header's copy is the one + // that sits where every other block's name sits. + const withoutTitle = { ...resolved }; + delete withoutTitle.title; + return withoutTitle; + }, [node?.props?.echartsOptions, rows, theme]); if (!node) return null; @@ -249,9 +250,9 @@ export default function ChartBlock({ nodeId }: { nodeId: string }) { // so this is never zero or ambiguous. width: '100%', height: '100%', - backgroundColor: theme.colorBgContainer, - border: `1px solid ${theme.colorBorderSecondary}`, - borderRadius: theme.borderRadiusLG, + // Surface, border and corners belong to the card `BuildingBlockView` + // draws around this block and the name above it, so that the name is + // inside the frame rather than over it. overflow: 'hidden', }} > diff --git a/superset-frontend/src/core/dashboard/blocks/FlexCanvas.tsx b/superset-frontend/src/core/dashboard/blocks/FlexCanvas.tsx new file mode 100644 index 000000000000..ad3fad0e6dbb --- /dev/null +++ b/superset-frontend/src/core/dashboard/blocks/FlexCanvas.tsx @@ -0,0 +1,183 @@ +/** + * 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 { useCallback, useState } from 'react'; +import type { DragEvent } from 'react'; +import type { dashboard as dashboardApi } from '@apache-superset/core'; +import { useTheme } from '@apache-superset/core/theme'; +import { provider } from '../store'; +import { + DEFAULT_COLUMNS, + resolveBlockHeightPx, + resolveFlexBasis, + resolveFlexMetrics, +} from '../layoutStyle'; +import { PALETTE_MIME, placeBlock } from '../placement'; +import BuildingBlockView from '../BuildingBlockView'; + +type LayoutProps = dashboardApi.LayoutProps; + +/** Carried on a reorder drag so a drop elsewhere in the app ignores it. */ +const FLEX_MIME = 'application/x-dashboard-flex-child'; + +/** + * A `flex` container: children flow along a line and wrap, sharing it in + * proportion to their `colSpan`. + * + * `react-grid-layout` is deliberately absent here. Every other mode is a + * grid, and RGL's whole model is coordinates in one — a flex line has no + * cells to give it, and asking it to lay out a wrapping proportional flow + * would mean computing the flow ourselves and then telling RGL the answer. + * CSS already does that, correctly, at every width. + * + * Which means position is order, and order is `children`. So the gesture + * that arranges a flex container is a reorder rather than a reposition, and + * it commits through `moveBuildingBlock` — the same call the AI tools use, so + * dragging a block and asking for it to be moved end at the same place. + * Without this a flex container would be a mode an author can see and not + * author in, which is worse than not offering it. + */ +export default function FlexCanvas({ + nodeId, + layout, + childIds, +}: { + nodeId: string; + layout: LayoutProps | undefined; + childIds: readonly string[]; +}) { + const theme = useTheme(); + const metrics = resolveFlexMetrics(layout, theme); + const columns = layout?.columns ?? DEFAULT_COLUMNS; + /** Which child the pointer is currently over, so the drop target is visible. */ + const [over, setOver] = useState(undefined); + + const drop = useCallback( + (event: DragEvent, targetId: string) => { + event.preventDefault(); + setOver(undefined); + const draggedId = event.dataTransfer.getData(FLEX_MIME); + if (draggedId === '' || draggedId === targetId) { + return; + } + const index = childIds.indexOf(targetId); + if (index === -1) { + return; + } + try { + provider.moveBuildingBlock(draggedId, nodeId, index); + } catch { + // Dropped into itself or one of its own descendants. The provider + // refuses it; leaving the tree as it was is the whole handling. + } + }, + [childIds, nodeId], + ); + + return ( +
{ + if (event.dataTransfer.types.includes(PALETTE_MIME)) { + event.preventDefault(); + event.dataTransfer.dropEffect = 'copy'; + } + }} + onDrop={event => { + const type = event.dataTransfer.getData(PALETTE_MIME); + if (type !== '') { + event.preventDefault(); + event.stopPropagation(); + placeBlock(nodeId, type); + } + }} + style={{ + display: 'flex', + width: '100%', + height: '100%', + overflow: 'auto', + alignContent: 'flex-start', + flexDirection: metrics.flexDirection, + flexWrap: metrics.flexWrap, + justifyContent: metrics.justifyContent, + alignItems: metrics.alignItems, + gap: metrics.gap, + }} + > + {childIds.map(childId => { + const child = provider.getNode(childId); + const basis = resolveFlexBasis(child?.layout, columns, childIds.length); + return ( +
{ + event.dataTransfer.setData(FLEX_MIME, childId); + event.dataTransfer.effectAllowed = 'move'; + }} + onDragOver={event => { + event.preventDefault(); + setOver(childId); + }} + onDragLeave={() => + setOver(current => (current === childId ? undefined : current)) + } + onDrop={event => drop(event, childId)} + style={{ + // The basis is the share; growing past it would let a wide + // sibling's leftover space silently re-widen a narrow one, so + // what the author set is what is drawn. + flex: `0 0 ${metrics.flexDirection === 'row' ? basis : 'auto'}`, + // Subtracting the gap keeps two halves on one line: a basis of + // 50% twice plus a gap between them is wider than the line. + maxWidth: + metrics.flexDirection === 'row' + ? `calc(${basis} - ${metrics.gap}px)` + : undefined, + height: resolveBlockHeightPx(child?.layout?.rowSpan, metrics), + outline: + over === childId + ? `2px solid ${theme.colorPrimary}` + : undefined, + cursor: 'grab', + }} + > + {/* The box, handed down. Every leaf block fills the one its + placement wrapper gives it — a chart measures it to size its + canvas, markdown scrolls inside it — and in a grid that box + arrives as the explicit pixel width and height + react-grid-layout injects when it clones the block. A flex + container positions its children itself, so it owes them the + same thing: without it the block is content-height, a chart's + measured height collapses to its loading indicator, and + markdown taller than its share paints over the row below. */} + +
+ ); + })} +
+ ); +} diff --git a/superset-frontend/src/core/dashboard/blocks/MarkdownBlock.tsx b/superset-frontend/src/core/dashboard/blocks/MarkdownBlock.tsx index bb5317cf87b0..0a6c39105181 100644 --- a/superset-frontend/src/core/dashboard/blocks/MarkdownBlock.tsx +++ b/superset-frontend/src/core/dashboard/blocks/MarkdownBlock.tsx @@ -37,9 +37,9 @@ export default function MarkdownBlock({ nodeId }: { nodeId: string }) { style={{ width: '100%', height: '100%', - backgroundColor: theme.colorBgContainer, - border: `1px solid ${theme.colorBorderSecondary}`, - borderRadius: theme.borderRadiusLG, + // Surface, border and corners belong to the card `BuildingBlockView` + // draws around this block and the name above it, so that the name is + // inside the frame rather than over it. padding: theme.padding, overflow: 'auto', }} diff --git a/superset-frontend/src/core/dashboard/blocks/MetricTileBlock.tsx b/superset-frontend/src/core/dashboard/blocks/MetricTileBlock.tsx index 2bb278dc2253..e5a45c6ac95f 100644 --- a/superset-frontend/src/core/dashboard/blocks/MetricTileBlock.tsx +++ b/superset-frontend/src/core/dashboard/blocks/MetricTileBlock.tsx @@ -153,9 +153,9 @@ export default function MetricTileBlock({ nodeId }: { nodeId: string }) { // block — always a definite pixel box, same as `ChartBlock`. width: '100%', height: '100%', - backgroundColor: theme.colorBgContainer, - border: `1px solid ${theme.colorBorderSecondary}`, - borderRadius: theme.borderRadiusLG, + // Surface, border and corners belong to the card `BuildingBlockView` + // draws around this block and the name above it, so that the name is + // inside the frame rather than over it. padding: theme.padding, overflow: 'hidden', }} diff --git a/superset-frontend/src/core/dashboard/layoutStyle.test.ts b/superset-frontend/src/core/dashboard/layoutStyle.test.ts new file mode 100644 index 000000000000..3f1de02847b0 --- /dev/null +++ b/superset-frontend/src/core/dashboard/layoutStyle.test.ts @@ -0,0 +1,105 @@ +/** + * 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 { supersetTheme } from '@apache-superset/core/theme'; +import { + resolveFlexBasis, + resolveFlexMetrics, + resolveGridMetrics, + resolveLayoutMode, +} from './layoutStyle'; + +const theme = supersetTheme as unknown as Parameters< + typeof resolveGridMetrics +>[1]; + +test('a container that names no mode is a grid', () => { + // The whole of the back-compatibility story: every node authored before + // the field existed, and every AI tool call that still omits it, arranges + // exactly as it did. + expect(resolveLayoutMode(undefined)).toBe('grid'); + expect(resolveLayoutMode({ columns: 12 })).toBe('grid'); +}); + +test('a container that names a mode gets it', () => { + expect(resolveLayoutMode({ mode: 'free' })).toBe('free'); + expect(resolveLayoutMode({ mode: 'flex' })).toBe('flex'); +}); + +test('flex maps the schema names to CSS rather than forwarding them', () => { + const metrics = resolveFlexMetrics( + { + mode: 'flex', + direction: 'column', + justify: 'space-between', + align: 'center', + }, + theme, + ); + + // `start` is the schema's word and `flex-start` is CSS's. A stored layout + // never holds a raw CSS keyword the renderer merely passes through. + expect(metrics.flexDirection).toBe('column'); + expect(metrics.justifyContent).toBe('space-between'); + expect(metrics.alignItems).toBe('center'); +}); + +test('flex falls back rather than letting an unknown value reach the style', () => { + const metrics = resolveFlexMetrics( + { justify: 'sideways' as never, align: 'diagonal' as never }, + theme, + ); + + expect(metrics.justifyContent).toBe('flex-start'); + expect(metrics.alignItems).toBe('stretch'); +}); + +test('flex wraps unless told not to', () => { + expect(resolveFlexMetrics({}, theme).flexWrap).toBe('wrap'); + expect(resolveFlexMetrics({ wrap: false }, theme).flexWrap).toBe('nowrap'); +}); + +test('gap and row height survive a change of mode', () => { + // Switching how a container arranges its children should not change how + // far apart or how tall they are. + const layout = { gap: 24, rowUnit: 40 }; + + expect(resolveFlexMetrics(layout, theme).gap).toBe( + resolveGridMetrics(layout, theme).gap, + ); + expect(resolveFlexMetrics(layout, theme).rowUnitPx).toBe( + resolveGridMetrics(layout, theme).rowUnitPx, + ); +}); + +test('a flex child that was sized takes that share of the line', () => { + expect(resolveFlexBasis({ colSpan: 12 }, 24, 2)).toBe('50%'); + expect(resolveFlexBasis({ colSpan: 6 }, 24, 4)).toBe('25%'); +}); + +test('a flex child that was never sized takes an equal share, not its content', () => { + // The things a dashboard arranges have no intrinsic width — a chart fills + // whatever box it is handed — so content sizing draws a row of four + // sections as four slivers. + expect(resolveFlexBasis(undefined, 24, 4)).toBe('25%'); + expect(resolveFlexBasis({ rowSpan: 3 }, 24, 3)).toBe(`${100 / 3}%`); +}); + +test('a flex child cannot claim more of the line than there is', () => { + expect(resolveFlexBasis({ colSpan: 99 }, 24, 2)).toBe('100%'); +}); diff --git a/superset-frontend/src/core/dashboard/layoutStyle.ts b/superset-frontend/src/core/dashboard/layoutStyle.ts index f151c26db34f..603340311ec6 100644 --- a/superset-frontend/src/core/dashboard/layoutStyle.ts +++ b/superset-frontend/src/core/dashboard/layoutStyle.ts @@ -20,6 +20,7 @@ import type { dashboard as dashboardApi } from '@apache-superset/core'; import type { useTheme } from '@apache-superset/core/theme'; type LayoutProps = dashboardApi.LayoutProps; +type LayoutMode = dashboardApi.LayoutMode; type Theme = ReturnType; /** Column count a container falls back to when its layout omits `columns`. */ @@ -27,6 +28,17 @@ export const DEFAULT_COLUMNS = 24; const DEFAULT_GAP = 16; +/** + * The mode a container arranges its children in. + * + * Absent means `grid`, and that is the whole of the back-compatibility story: + * every node authored before the field existed, and every AI tool call that + * still omits it, keeps arranging exactly as it did. + */ +export function resolveLayoutMode(layout: LayoutProps | undefined): LayoutMode { + return layout?.mode ?? 'grid'; +} + /** A container's resolved grid geometry, in the plain numbers `CanvasBlock` feeds to `react-grid-layout` (`cols`/`rowHeight`/`margin`). */ export interface GridMetrics { columns: number; @@ -50,3 +62,97 @@ export function resolveGridMetrics( rowUnitPx: layout?.rowUnit ?? theme.sizeUnit * 8, }; } + +/** + * The pixel height a container reserves for a child spanning `rowSpan` rows. + * + * The gaps *between* those rows belong to the block, not to the space around + * it: this is `react-grid-layout`'s own `calcGridItemWHPx`, which already + * sizes every grid-mode block. A container that positions its own children + * has to apply the same formula rather than multiplying rows by row height, + * or the identical block is drawn shorter than the grid drew it and changing + * a container's mode silently resizes everything in it. + * + * A child that never declared a span occupies one row, matching the same + * default `packChildLayout` gives it. + */ +export function resolveBlockHeightPx( + rowSpan: number | undefined, + metrics: Pick, +): number { + const rows = rowSpan ?? 1; + return Math.round( + rows * metrics.rowUnitPx + Math.max(0, rows - 1) * metrics.gap, + ); +} + +/** The CSS a `flex` container lays its own children out with. */ +export interface FlexMetrics { + flexDirection: 'row' | 'column'; + flexWrap: 'wrap' | 'nowrap'; + justifyContent: string; + alignItems: string; + gap: number; + rowUnitPx: number; +} + +const JUSTIFY: Record = { + start: 'flex-start', + center: 'center', + end: 'flex-end', + 'space-between': 'space-between', + 'space-around': 'space-around', +}; + +const ALIGN: Record = { + start: 'flex-start', + center: 'center', + end: 'flex-end', + stretch: 'stretch', +}; + +/** + * Resolves a `flex` container's geometry. + * + * The names in the schema are mapped here rather than forwarded, so the + * stored layout never holds a raw CSS keyword the renderer merely passes + * through — `start` is the schema's word and `flex-start` is CSS's, and an + * unrecognised value falls back rather than reaching the style attribute. + * + * `gap` and `rowUnit` are shared with the grid modes on purpose: switching a + * container's mode should change how its children are arranged, not how far + * apart or how tall they are. + */ +export function resolveFlexMetrics( + layout: LayoutProps | undefined, + theme: Theme, +): FlexMetrics { + return { + flexDirection: layout?.direction === 'column' ? 'column' : 'row', + flexWrap: layout?.wrap === false ? 'nowrap' : 'wrap', + justifyContent: JUSTIFY[layout?.justify ?? 'start'] ?? 'flex-start', + alignItems: ALIGN[layout?.align ?? 'stretch'] ?? 'stretch', + gap: layout?.gap ?? DEFAULT_GAP, + rowUnitPx: layout?.rowUnit ?? theme.sizeUnit * 8, + }; +} + +/** + * A `flex` child's share of the line, as a CSS `flex-basis` percentage. + * + * A child that was never sized takes an equal share rather than being sized + * by its content: the things a dashboard arranges have no intrinsic width — + * a chart fills whatever box it is handed — so content sizing resolves to no + * width at all, and a row of four sections draws as four slivers. + */ +export function resolveFlexBasis( + layout: LayoutProps | undefined, + columns: number, + siblings: number, +): string { + const span = layout?.colSpan; + if (span == null) { + return `${100 / Math.max(1, siblings)}%`; + } + return `${(Math.min(span, columns) / columns) * 100}%`; +} diff --git a/superset-frontend/src/core/dashboard/placement.ts b/superset-frontend/src/core/dashboard/placement.ts new file mode 100644 index 000000000000..80830d06b969 --- /dev/null +++ b/superset-frontend/src/core/dashboard/placement.ts @@ -0,0 +1,71 @@ +/** + * 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. + */ + +/** + * @fileoverview Placing a new block, in the one place both ways of asking + * for it can reach. + * + * A block arrives on a dashboard two ways — clicked in the palette, or + * dragged from it onto a container — and they must produce the same node. Two + * copies of "what a freshly placed block looks like" is how a block dropped + * into a section ends up subtly different from the same block clicked into + * it, and the difference is invisible until someone hits it. + */ + +import { isContainerType } from './DashboardProvider'; +import { DEFAULT_COLUMNS } from './layoutStyle'; +import { provider } from './store'; + +/** + * What a palette drag carries. + * + * A private type rather than `text/plain` so a drop of anything else — a + * file, a selection of text, a drag from another application — is not read + * as a request to place a block. + */ +export const PALETTE_MIME = 'application/x-dashboard-building-block'; + +/** + * Places a new block of `type` at the end of `parentId`'s children and + * selects it, returning its id. + * + * A container arrives with the grid every other container defaults to, so a + * nested canvas is usable the moment it lands rather than needing its columns + * set before anything can go inside it. Selecting what was just placed is + * what brings its properties forward: placing something is the moment you + * want to configure it. + */ +export function placeBlock(parentId: string, type: string): string { + const index = provider.getNode(parentId)?.children?.length ?? 0; + const id = provider.addBuildingBlock(parentId, index, { + type, + ...(isContainerType(type) + ? { + layout: { + columns: DEFAULT_COLUMNS, + gap: 16, + colSpan: DEFAULT_COLUMNS, + rowSpan: 4, + }, + } + : {}), + }); + provider.setSelection(id); + return id; +} diff --git a/superset-frontend/src/core/navigation/index.test.ts b/superset-frontend/src/core/navigation/index.test.ts index c717d3779ef1..0e6f6eaff8c2 100644 --- a/superset-frontend/src/core/navigation/index.test.ts +++ b/superset-frontend/src/core/navigation/index.test.ts @@ -99,6 +99,12 @@ test('chart and dashboard list pages get their own page types', async () => { expect(navigation.getPage()).toBe('dashboard_list'); }); +test('Dashboard v2 is distinct from the classic dashboard route', async () => { + const { notifyLocationChanged, navigation } = await importNavigation(); + notifyLocationChanged('/dashboard/v2/new/'); + expect(navigation.getPage()).toBe('dashboard_v2'); +}); + test('dataset list and single-dataset pages get distinct page types', async () => { const { notifyLocationChanged, navigation } = await importNavigation(); notifyLocationChanged('/tablemodelview/list/'); diff --git a/superset-frontend/src/core/navigation/index.ts b/superset-frontend/src/core/navigation/index.ts index 2e2a61d3fb4d..0fbe276a5b09 100644 --- a/superset-frontend/src/core/navigation/index.ts +++ b/superset-frontend/src/core/navigation/index.ts @@ -41,6 +41,7 @@ const PAGE_ROUTES: { path: string; page: Page }[] = [ // greedily capture `/dashboard/list/` (idOrSlug='list'), so the more specific // list route has to win first — mirroring the `routes.tsx` Switch precedence. { path: RoutePaths.DASHBOARD_LIST, page: 'dashboard_list' }, + { path: RoutePaths.DASHBOARD_V2_NEW, page: 'dashboard_v2' }, { path: RoutePaths.DASHBOARD, page: 'dashboard' }, { path: RoutePaths.QUERY_HISTORY, page: 'query_history' }, { path: RoutePaths.SAVED_QUERIES, page: 'saved_queries' }, diff --git a/superset-frontend/src/pages/DashboardBuilderV2/CanvasControls.tsx b/superset-frontend/src/pages/DashboardBuilderV2/CanvasControls.tsx new file mode 100644 index 000000000000..c18759652115 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/CanvasControls.tsx @@ -0,0 +1,102 @@ +/** + * 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 { ReactElement } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { useTheme } from '@apache-superset/core/theme'; +import { Button } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { provider } from 'src/core/dashboard/store'; +import Inert from './InertControl'; + +/** + * What acts on the canvas as a whole, in the canvas's own corner. + * + * These two are not chrome about the dashboard — not what it is called, who + * owns it, or whether it is published. They act on the blocks in front of you, + * and both are reached for while looking at them, which is why they sit here + * rather than on the bar above. + * + * **Arrange** is a route, not a control. How a container lays out its + * children is a property of that container and is asked with the rest of them + * — the columns, the gap, the row height it works alongside. That is the + * right home for it and also further from hand than something permanently on + * screen, so this is the way back to it. A second copy of the switcher would + * be a second thing to keep agreeing with the first; selecting the root is + * all this does, and the editor panel brings Properties forward on a + * selection it did not make itself. It stays live on a page where almost + * nothing is, because selecting a node this page already holds in memory + * needs no dashboard row. + * + * **Refresh** is the opposite: named, and honest that it cannot work. There + * is no row behind this page and no query to re-run, so it says so rather + * than doing nothing quietly. + */ +export default function CanvasControls(): ReactElement { + const theme = useTheme(); + + return ( +
+ + + + +
+ ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/DashboardHeader.test.tsx b/superset-frontend/src/pages/DashboardBuilderV2/DashboardHeader.test.tsx new file mode 100644 index 000000000000..2374cad31d86 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/DashboardHeader.test.tsx @@ -0,0 +1,153 @@ +/** + * 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 userEvent from '@testing-library/user-event'; +import { render, screen } from 'spec/helpers/testing-library'; +import DashboardProvider from 'src/core/dashboard/DashboardProvider'; +import DashboardHeader from './DashboardHeader'; + +const provider = DashboardProvider.getInstance(); + +const ADMIN = { userId: 1, firstName: 'Admin', lastName: 'User' }; + +/** The header reads the session for who is authoring, so it needs the store. */ +const renderHeader = () => + render(, { + useRedux: true, + initialState: { user: ADMIN }, + }); + +beforeEach(() => { + provider.reset(); +}); + +test('the header carries the dashboard-level affordances', () => { + renderHeader(); + + expect(screen.getByTestId('header-templates')).toBeInTheDocument(); + expect(screen.getByTestId('header-history')).toBeInTheDocument(); + expect(screen.getByTestId('header-favorite')).toBeInTheDocument(); + expect(screen.getByTestId('header-published')).toHaveTextContent('Draft'); + expect(screen.getByTestId('header-undo')).toBeInTheDocument(); + expect(screen.getByTestId('header-redo')).toBeInTheDocument(); + expect(screen.getByTestId('header-save')).toBeInTheDocument(); +}); + +test('everything the builder cannot actually do is disabled, not silently dead', () => { + renderHeader(); + + // The builder keeps its tree in memory with no dashboard row behind it: + // nothing here can be saved, favourited, published or refreshed, and there + // is no history to step through. A control that looks live and does + // nothing teaches something false about all of them. + [ + 'header-templates', + 'header-history', + 'header-favorite', + 'header-undo', + 'header-redo', + 'header-save', + ].forEach(test => expect(screen.getByTestId(test)).toBeDisabled()); +}); + +test('the record of what was written sits beside writing it', () => { + renderHeader(); + + const order = [ + ...screen.getByTestId('dashboard-header').querySelectorAll('[data-test]'), + ].map(el => el.getAttribute('data-test')); + + // Saving commits a version; History is the versions already committed. + // They are one concern read in one place, so History leaves the far left — + // where it sat beside Templates as a thing asked before the work — and + // comes to rest immediately before the button that produces what it lists. + expect(order.indexOf('header-history')).toBe( + order.indexOf('header-save') - 1, + ); +}); + +test('how the dashboard is arranged is not asked in the header', () => { + renderHeader(); + + // Arranging the canvas is authoring, not chrome. It belongs with the rest + // of the root's properties, where the columns and the gap it works with + // already live — see Inspector's Arrangement section. + expect(screen.queryByTestId('layout-mode-switcher')).not.toBeInTheDocument(); +}); + +test('what acts on the canvas is not offered from the bar above it', () => { + renderHeader(); + + // Arranging and refreshing both act on the canvas as a whole, and are both + // reached for while looking at it. They sit in its corner — see + // CanvasControls. What stays here is what the dashboard is, not what is + // being done to the blocks on it. + expect(screen.queryByTestId('canvas-arrange')).not.toBeInTheDocument(); + expect(screen.queryByTestId('header-arrange')).not.toBeInTheDocument(); + expect(screen.queryByTestId('header-refresh')).not.toBeInTheDocument(); +}); + +test('the header says who is making the dashboard', () => { + renderHeader(); + + // The one piece of dashboard metadata this page can state truthfully: a + // dashboard being created is being created by whoever is looking at it. + expect(screen.getByTestId('header-metadata')).toHaveTextContent('Admin User'); +}); + +test('the header does not claim a dashboard with no row behind it was saved', () => { + renderHeader(); + + // Every other unavailable affordance here says so. A humanized "a day ago" + // beside them would be the only thing on the bar inventing a fact. + expect(screen.getByTestId('header-metadata')).toHaveTextContent( + 'Not saved yet', + ); +}); + +test('the dashboard is nameable, and the name is stored on the dashboard', async () => { + renderHeader(); + + await userEvent.type(screen.getByTestId('header-title'), 'Vaccine rollout'); + await userEvent.tab(); + + // On the root node rather than in this component's state: a name is + // something the dashboard has, so the assistant can read and rename it too. + expect(provider.getRoot().props?.title).toBe('Vaccine rollout'); +}); + +test('the title shows a rename made anywhere else', () => { + provider.updateProps(provider.getRoot().id, { title: 'From the assistant' }); + + renderHeader(); + + expect(screen.getByTestId('header-title')).toHaveValue('From the assistant'); +}); + +test('emptying the title is not a rename', async () => { + provider.updateProps(provider.getRoot().id, { title: 'Quarterly review' }); + renderHeader(); + + await userEvent.clear(screen.getByTestId('header-title')); + await userEvent.tab(); + + // A stray select-all-and-delete must not silently leave the dashboard + // nameless; the field goes back to what the dashboard is still called. + expect(provider.getRoot().props?.title).toBe('Quarterly review'); + expect(screen.getByTestId('header-title')).toHaveValue('Quarterly review'); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/DashboardHeader.tsx b/superset-frontend/src/pages/DashboardBuilderV2/DashboardHeader.tsx new file mode 100644 index 000000000000..96021ed09317 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/DashboardHeader.tsx @@ -0,0 +1,278 @@ +/** + * 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 { useEffect, useState } from 'react'; +import type { ReactElement } from 'react'; +import { useSelector } from 'react-redux'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { Divider, Input, PublishedLabel } from '@superset-ui/core/components'; +import MetadataBar, { + MetadataType, +} from '@superset-ui/core/components/MetadataBar'; +import { Icons } from '@superset-ui/core/components/Icons'; +import type { BootstrapUser } from 'src/types/bootstrapTypes'; +import { provider, useDashboardRevision } from 'src/core/dashboard/store'; +import Inert from './InertControl'; + +/** + * Who is making this dashboard, and when it was last written down. + * + * The two facts a dashboard carries about itself rather than about its + * contents, drawn with the same `MetadataBar` the saved dashboard header + * uses — so a dashboard being built and one being read state them in the + * same shape, with the same icons, in the same place. + * + * Only one of them can be true here. A dashboard being created is being + * created by whoever is looking at it, so the creator is read from the + * session rather than invented. There is no row behind this page and nothing + * has ever been written, so there is no modified time to humanize — and + * "a day ago" beside a Save button that is disabled for having nothing to + * save would be the only thing on this bar stating a fact that is not one. + */ +/** + * The signed-in person's name. + * + * Assembled here rather than through `getUserName`, which reads the + * `first_name`/`last_name` an API hands back for an owner. The session user + * is the same person in a different shape — `firstName`/`lastName` off the + * bootstrap — and passing one to the other returns an empty string rather + * than failing, which is how this first went out reading "Not available" + * over a perfectly well-known name. + */ +const nameOf = (user: BootstrapUser): string => + [user?.firstName, user?.lastName].filter(Boolean).join(' ') || + user?.username || + ''; + +const Metadata = (): ReactElement => { + const user = useSelector<{ user?: BootstrapUser }, BootstrapUser>( + state => state.user, + ); + const author = nameOf(user) || t('Not available'); + const unsaved = t('Not saved yet'); + + return ( + + + + ); +}; + +/** + * The name, drawn as a name rather than as a field. + * + * A bordered box on a bar of small controls read as one more control, and the + * one thing on the bar that says what you are looking at was the hardest thing + * on it to find. Borderless at the heading weight, it reads as the title it + * is; the surface arriving under the pointer and on focus is what still says + * it can be typed into, which is the same trade the editable titles elsewhere + * in the app make. + */ +const TitleInput = styled(Input)` + ${({ theme }) => css` + max-width: ${theme.sizeUnit * 60}px; + height: ${theme.controlHeightSM}px; + padding-inline: ${theme.sizeUnit}px; + font-size: ${theme.fontSizeLG}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + background-color: transparent; + transition: background-color ${theme.motionDurationMid}; + + &:hover, + &:focus { + background-color: ${theme.colorFillQuaternary}; + } + + &::placeholder { + font-weight: ${theme.fontWeightNormal}; + color: ${theme.colorTextTertiary}; + } + `} +`; + +/** + * The dashboard's name, edited where it is read. + * + * It is stored on the root node rather than in this component, because a name + * is something the dashboard has and not something this screen remembers: put + * in page state it would be invisible to the assistant, unreachable by the + * client tools, and gone on the next navigation. The root canvas is the only + * node a dashboard-level fact can belong to, so that is where it lives. + * + * A title is also a `markdown` block an author can place at the top of the + * canvas, and that stays true — this is a different thing with a different + * job. That one is content, laid out and arranged like any other block; this + * one is what the dashboard is called. + * + * The draft commits on blur rather than on every keystroke: a name being + * typed is not a name, and one commit per character would be one revision + * tick per character for everything subscribed to the store. + */ +const Title = ({ nodeId, title }: { nodeId: string; title: string }) => { + const [draft, setDraft] = useState(title); + // What was accepted replaces the draft, because the draft was a view of it: + // a rename the assistant makes while this is on screen has to show. + useEffect(() => setDraft(title), [title]); + + return ( + setDraft(event.target.value)} + onBlur={() => { + const next = draft.trim(); + // An empty name is not a rename. Restoring the draft rather than + // writing the blank is what keeps a stray select-all-and-delete from + // silently leaving the dashboard nameless. + if (next === '') { + setDraft(title); + } else if (next !== title) { + provider.updateProps(nodeId, { title: next }); + } + }} + /> + ); +}; + +/** + * The bar itself. + * + * Inset horizontally the way the rest of the app insets a page header, so the + * left edge of the bar and the left edge of the work below it are one line + * rather than two a few pixels apart. The rule beneath is `colorSplit` — what + * this app draws a separator with — rather than the heavier border it shares + * with the boxes that hold things. + */ +const Bar = styled.header` + ${({ theme }) => css` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + flex: 0 0 auto; + padding: ${theme.sizeUnit * 2}px ${theme.sizeUnit * 4}px; + border-bottom: 1px solid ${theme.colorSplit}; + background-color: ${theme.colorBgContainer}; + `} +`; + +/** What an author does to the whole dashboard, at the end they read last. */ +const Actions = styled.span` + ${({ theme }) => css` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + margin-left: auto; + `} +`; + +/** + * The dashboard's header: what this dashboard is, and what can be done to it. + * + * Two kinds of thing share the bar. On the left is the dashboard as the + * product would know it — where to start from, where it has been, what it is + * called, whether it is published, and whose it is. On the right is what an + * author does to the whole of it: step back through what they did, or write + * it down. + * + * How the canvas is arranged is not among them. It reads like chrome and is + * not: it is a property of the root node, sitting in the same `layout` the + * columns and the gap sit in, and asking for it here put one third of that + * one decision on the other side of the screen from the rest. It is asked in + * the root's own properties now, where a canvas is selected and arranged in + * one place. + */ +export default function DashboardHeader(): ReactElement { + useDashboardRevision(); + const root = provider.getRoot(); + + return ( + + {/* Where this dashboard came from: a starting point to build on, asked + before the work rather than during it, which is why it leads the + bar. History used to sit beside it on that reasoning and has gone to + the other end — it is read against saving, not against starting. */} + + {t('Templates')} + + + <Inert label={t('Favorite')} test="header-favorite" buttonStyle="link"> + <Icons.StarOutlined iconSize="m" /> + </Inert> + {/* Nothing here can publish, so the chip states the only status this + page can honestly claim. */} + <span data-test="header-published"> + <PublishedLabel isPublished={false} /> + </span> + {/* Beside the status rather than opposite it: whether a dashboard is a + draft, whose it is, and when it was last written are one answer to + one question — what state is this in — and they are read together. */} + <Metadata /> + + <Actions> + {/* Icons, not words, because these two are reached by muscle memory + far more often than they are read. The name stays on them for + anyone not reading with their eyes. */} + <Inert label={t('Undo')} test="header-undo"> + <Icons.UndoOutlined iconSize="s" /> + </Inert> + <Inert label={t('Redo')} test="header-redo"> + <Icons.RedoOutlined iconSize="s" /> + </Inert> + {/* Stepping back through the work and writing it down are two + different acts on the bar's one crowded end, and at an even gap + the four of them read as one run of controls. The rule is what + says where one pair stops and the other starts. */} + <Divider type="vertical" /> + {/* Saving commits a version; History is the versions already + committed. One concern, read in one place — so the record sits + immediately before the button that produces what it lists, rather + than at the far side of the bar from it. */} + <Inert label={t('History')} test="header-history" reads> + {t('History')} + </Inert> + <Inert label={t('Save')} test="header-save" reads> + {t('Save')} + </Inert> + </Actions> + </Bar> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/DashboardProperties.tsx b/superset-frontend/src/pages/DashboardBuilderV2/DashboardProperties.tsx new file mode 100644 index 000000000000..803051557f6f --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/DashboardProperties.tsx @@ -0,0 +1,436 @@ +/** + * 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 { useCallback, useEffect, useMemo, useState } from 'react'; +import type { ReactElement } from 'react'; +import rison from 'rison'; +import { SupersetClient } from '@superset-ui/core'; +import { t, tn } from '@apache-superset/core/translation'; +import { css, styled, useTheme } from '@apache-superset/core/theme'; +import { Collapse, Form } from '@superset-ui/core/components'; +import { useJsonValidation } from '@superset-ui/core/components/AsyncAceEditor'; +import type { TagType } from 'src/components'; +import type Subject from 'src/types/Subject'; +import type { SubjectPickerValue } from 'src/features/subjects/SubjectPicker'; +import { useModalValidation } from 'src/components/Modal'; +import { + AccessSection, + AdvancedSection, + BasicInfoSection, + CertificationSection, + RefreshSection, + StylingSection, +} from 'src/dashboard/components/PropertiesModal/sections'; +import { provider, useDashboardRevision } from 'src/core/dashboard/store'; + +/** + * The dashboard's own fields, as the root node stores them. + * + * Named once because three things have to agree on them: what is read out of + * the root to fill the form, what is written back, and what counts as a + * change worth committing. + */ +const TEXT_FIELDS = [ + 'title', + 'slug', + 'description', + 'certifiedBy', + 'certificationDetails', +] as const; + +type TextField = (typeof TEXT_FIELDS)[number]; +type TextValues = Record<TextField, string>; + +interface FetchedTheme { + id: number; + theme_name: string; + json_data?: string; +} + +const asString = (value: unknown): string => + typeof value === 'string' ? value : ''; + +/** + * A section's name, at the weight the rest of this panel names things at. + * + * The sections below are drawn for a modal, and the wrapper they come with + * dresses each one as a heading with a subtitle, a banded background and a + * tick saying it validates. In a modal that is the whole screen and the + * reader has nothing else to look at; in a rail beside the canvas it shouts + * over the fields it introduces, and sat a heading twice the size of the + * `Arrangement` heading directly beneath it. + * + * So the sections are kept and their wrapper is not: this matches `Section` + * in the Inspector, which is what a group of fields is called everywhere else + * in this panel — the same size as the labels it introduces, carrying the + * difference in weight rather than in size, so a heading is not set in less + * than the fields beneath it. The ticks go with the wrapper — they report on a + * save that this page cannot do, and each section still says what is wrong + * with it where the wrong thing is. + */ +const sectionLabel = ( + theme: ReturnType<typeof useTheme>, + title: string, +): ReactElement => ( + <span + style={{ + fontSize: theme.fontSize, + fontWeight: theme.fontWeightStrong, + color: theme.colorText, + }} + > + {title} + </span> +); + +/** + * The panel, and the one thing `size="small"` cannot reach on its own. + * + * A global rule sets `padding: 4px 8px` on every `input[type="text"]` in the + * app. antd sizes a small input by zeroing its own block padding, so that + * rule wins on specificity and a field marked `ant-input-sm` still renders at + * the middle height — eight pixels taller than every other input in this + * rail. The sections below write `type="text"` explicitly, which is what puts + * them in the global rule's way. + * + * Scoped to this panel rather than fixed at the global rule, which is load + * bearing for the rest of the app and not this change's to move. + */ +const Panel = styled.div` + ${({ theme }) => css` + padding-top: ${theme.sizeUnit * 3}px; + font-size: ${theme.fontSizeSM}px; + + /* Doubled deliberately. The global rule is a class plus an attribute + selector, the same specificity this would otherwise have, and it is + injected later — so matching it is losing to it. */ + && input[type='text'] { + padding-block: 0; + padding-inline: ${theme.sizeUnit * 2}px; + } + `} +`; + +/** How many blocks are on the dashboard, at any depth. */ +const countBlocks = (id: string): number => + (provider.getNode(id)?.children ?? []).reduce( + (total, childId) => total + 1 + countBlocks(childId), + 0, + ); + +/** + * Everything the dashboard is, as opposed to everything on it. + * + * The six sections are the ones `PropertiesModal` already draws, reused whole + * rather than reimplemented: the modal and this panel are two ways into one + * set of fields, and a second implementation is how the two quietly stop + * agreeing about what a dashboard has. Their wrapper is not reused — see + * {@link sectionLabel} for why a modal's headings do not belong in a rail. + * + * Everything is stored on the root node's props, beside the `title` the + * header already keeps there — the only place a dashboard-level fact is + * visible to the assistant and reachable by the client tools. Nothing is + * persisted, because nothing on this page is; what that means for the reader + * is said plainly at the top of the panel rather than left to be discovered + * at the disabled Save button. + * + * Text commits on blur, through one handler on the container rather than one + * per field: every input and both editors bubble a blur, and one commit per + * field left beats one revision tick per keystroke. Discrete controls — the + * pickers, the colour scheme, the refresh interval, the switch — commit in + * their own handler, because there is no typing to wait out and a dropdown + * that closes on an uncommitted value reads as broken. + */ +export default function DashboardProperties(): ReactElement { + useDashboardRevision(); + const theme = useTheme(); + const root = provider.getRoot(); + const props = useMemo(() => root.props ?? {}, [root.props]); + + const [form] = Form.useForm(); + + /** What the root currently says, in the shape the form takes. */ + const accepted = useMemo( + () => + Object.fromEntries( + TEXT_FIELDS.map(key => [key, asString(props[key])]), + ) as TextValues, + [props], + ); + + // The form holds a draft of the text fields, and the draft is a view of + // what was accepted — so a rename made in the header, or by the assistant, + // replaces it rather than being typed over. + useEffect(() => form.setFieldsValue(accepted), [accepted, form]); + + // Both editors report every keystroke. Held here and committed with the + // rest of the text, so an unfinished CSS rule is not a revision. + const [customCss, setCustomCss] = useState(() => asString(props.customCss)); + const [jsonMetadata, setJsonMetadata] = useState(() => + asString(props.jsonMetadata), + ); + useEffect(() => setCustomCss(asString(props.customCss)), [props.customCss]); + useEffect( + () => setJsonMetadata(asString(props.jsonMetadata)), + [props.jsonMetadata], + ); + + const jsonAnnotations = useJsonValidation(jsonMetadata, { + errorPrefix: 'Invalid JSON metadata', + }); + + const { validationStatus, validateSection } = useModalValidation({ + sections: [ + { + key: 'basic', + name: t('General information'), + validator: () => + form.getFieldValue('title')?.trim() + ? [] + : [t('Dashboard name is required')], + }, + { + key: 'advanced', + name: t('Advanced settings'), + validator: () => + jsonAnnotations.length > 0 ? [t('Invalid JSON metadata')] : [], + }, + ], + }); + + const write = useCallback( + (next: Record<string, unknown>) => provider.updateProps(root.id, next), + [root.id], + ); + + /** + * Commits every text field that changed, on the way out of any of them. + * + * Two are refused rather than written. An emptied name is not a rename — + * the same rule the header title keeps, and the reason a stray + * select-all-and-delete cannot leave the dashboard nameless. Unparseable + * JSON metadata is not metadata; the section already shows where it broke, + * and writing a string no reader can parse would leave the dashboard in a + * state only this field could get it out of. + */ + const commit = useCallback((): void => { + validateSection('basic'); + validateSection('advanced'); + + const draft = form.getFieldsValue() as Partial<TextValues>; + const changed: Record<string, unknown> = {}; + + TEXT_FIELDS.forEach(key => { + const value = asString(draft[key]); + if (key === 'title' && value.trim() === '') { + form.setFieldsValue({ title: accepted.title }); + return; + } + if (value !== accepted[key]) { + changed[key] = value; + } + }); + + if (customCss !== asString(props.customCss)) { + changed.customCss = customCss; + } + if ( + jsonMetadata !== asString(props.jsonMetadata) && + jsonAnnotations.length === 0 + ) { + changed.jsonMetadata = jsonMetadata; + } + + if (Object.keys(changed).length > 0) { + write(changed); + } + }, [ + accepted, + customCss, + form, + jsonAnnotations.length, + jsonMetadata, + props.customCss, + props.jsonMetadata, + validateSection, + write, + ]); + + // Offered to StylingSection, which lists them. Fetched here because the + // panel is where they are needed and nothing else on this page knows the + // dashboard has a theme at all. A failure leaves the list empty rather than + // taking the panel down with it — every other field still edits. + const [themes, setThemes] = useState<FetchedTheme[]>([]); + useEffect(() => { + const query = rison.encode({ + columns: ['id', 'theme_name', 'is_system', 'json_data'], + filters: [{ col: 'is_system', opr: 'eq', value: false }], + }); + let live = true; + SupersetClient.get({ endpoint: `/api/v1/theme/?q=${query}` }) + .then(({ json }) => { + if (live) setThemes(json.result ?? []); + }) + .catch(() => {}); + return () => { + live = false; + }; + }, []); + + const blocks = countBlocks(root.id); + + return ( + // One handler for every field that is typed into: each bubbles its blur + // here, and what changed is worked out once rather than remembered per + // field. + <Panel data-test="dashboard-properties" onBlur={commit}> + <h3 + data-test="dashboard-properties-name" + style={{ + margin: 0, + fontSize: theme.fontSize, + fontWeight: theme.fontWeightStrong, + color: theme.colorText, + }} + > + {accepted.title || t('Untitled dashboard')} + </h3> + <p + data-test="dashboard-properties-counts" + style={{ + margin: `${theme.sizeUnit}px 0 0`, + color: theme.colorTextSecondary, + }} + > + {/* Filters are a literal nothing rather than a number that moves: + this builder has no concept of one yet, and a count that could + only ever read zero is still the honest answer to what is here. */} + {`${tn('%s block', '%s blocks', blocks, blocks)}, ${tn( + '%s filter', + '%s filters', + 0, + 0, + )}`} + </p> + <p + data-test="dashboard-properties-caption" + style={{ + margin: `${theme.sizeUnit * 2}px 0 ${theme.sizeUnit * 3}px`, + color: theme.colorTextTertiary, + }} + > + {t( + 'These belong to the dashboard rather than to its contents. Nothing here is saved yet — the builder holds them in memory.', + )} + </p> + + {/* `size="small"` reaches every control the reused sections draw: they + are written for a modal, where a control has the room to be full + height, and this rail spends its width on the fields themselves. */} + <Form form={form} layout="vertical" size="small" initialValues={accepted}> + <Collapse + ghost + size="small" + expandIconPosition="start" + defaultActiveKey={['basic']} + items={[ + { + key: 'basic', + label: sectionLabel(theme, t('General information')), + children: ( + <BasicInfoSection + form={form} + validationStatus={validationStatus} + /> + ), + }, + { + key: 'access', + label: sectionLabel(theme, t('Access & ownership')), + children: ( + <AccessSection + isLoading={false} + tags={(props.tags as TagType[]) ?? []} + editors={(props.editors as Subject[]) ?? []} + viewers={(props.viewers as Subject[]) ?? []} + onChangeEditors={(editors: SubjectPickerValue[]) => + write({ editors }) + } + onChangeViewers={(viewers: SubjectPickerValue[]) => + write({ viewers }) + } + onChangeTags={tags => write({ tags })} + onClearTags={() => write({ tags: [] })} + /> + ), + }, + { + key: 'styling', + label: sectionLabel(theme, t('Styling')), + children: ( + <StylingSection + themes={themes} + selectedThemeId={(props.themeId as number) ?? null} + colorScheme={asString(props.colorScheme)} + customCss={customCss} + hasCustomLabelsColor={false} + showChartTimestamps={props.showChartTimestamps === true} + onThemeChange={value => write({ themeId: value || null })} + onColorSchemeChange={colorScheme => write({ colorScheme })} + onCustomCssChange={setCustomCss} + onShowChartTimestampsChange={showChartTimestamps => + write({ showChartTimestamps }) + } + /> + ), + }, + { + key: 'refresh', + label: sectionLabel(theme, t('Refresh settings')), + children: ( + <RefreshSection + refreshFrequency={(props.refreshFrequency as number) ?? 0} + onRefreshFrequencyChange={refreshFrequency => + write({ refreshFrequency }) + } + /> + ), + }, + { + key: 'certification', + label: sectionLabel(theme, t('Certification')), + children: <CertificationSection isLoading={false} />, + }, + { + key: 'advanced', + label: sectionLabel(theme, t('Advanced settings')), + children: ( + <AdvancedSection + jsonMetadata={jsonMetadata} + jsonAnnotations={jsonAnnotations} + validationStatus={validationStatus} + onJsonMetadataChange={setJsonMetadata} + /> + ), + }, + ]} + /> + </Form> + </Panel> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.test.tsx b/superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.test.tsx new file mode 100644 index 000000000000..9f1a471d440e --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.test.tsx @@ -0,0 +1,262 @@ +/** + * 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 userEvent from '@testing-library/user-event'; +import { act, fireEvent, render, screen } from 'spec/helpers/testing-library'; +import DashboardProvider from 'src/core/dashboard/DashboardProvider'; +import 'src/core/dashboard'; +import EditorPanel from './EditorPanel'; + +const provider = DashboardProvider.getInstance(); + +beforeEach(() => { + provider.reset(); +}); + +const mount = () => { + const onAdd = jest.fn(); + render(<EditorPanel onAdd={onAdd} />); + return onAdd; +}; + +test('the panel offers building blocks, properties and an outline', () => { + mount(); + + expect( + screen.getByRole('tab', { name: 'Building blocks' }), + ).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'Properties' })).toBeInTheDocument(); + expect(screen.getByRole('tab', { name: 'Outline' })).toBeInTheDocument(); +}); + +test('building blocks is what you start on, and it lists what is registered', () => { + mount(); + + // The list is `views.getViews('dashboard.buildingBlocks')` — the same + // registry BuildingBlockView resolves a renderer through. Nothing here + // names a block, so registering one makes it placeable with no edit. + expect(screen.getByTestId('palette')).toBeVisible(); + expect(screen.getByTestId('palette-markdown')).toBeVisible(); + expect(screen.getByTestId('palette-echarts')).toBeVisible(); +}); + +test('a canvas is shelved as structure and everything else as content', () => { + mount(); + + // The one distinction this fork records: whether placing the type produces + // something other blocks can go inside. + expect(screen.getByTestId('palette-shelf-structure')).toContainElement( + screen.getByTestId('palette-canvas'), + ); + expect(screen.getByTestId('palette-shelf-content')).toContainElement( + screen.getByTestId('palette-markdown'), + ); +}); + +test('clicking a block asks the page to place it', async () => { + const onAdd = mount(); + + await userEvent.click(screen.getByTestId('palette-markdown')); + + expect(onAdd).toHaveBeenCalledWith('markdown'); +}); + +test('searching narrows the palette to what was asked for', async () => { + mount(); + + await userEvent.type(screen.getByTestId('palette-search'), 'markdown'); + + expect(screen.getByTestId('palette-markdown')).toBeVisible(); + expect(screen.queryByTestId('palette-echarts')).not.toBeInTheDocument(); +}); + +test('with nothing selected, properties says so rather than showing a stale block', async () => { + mount(); + + await userEvent.click(screen.getByRole('tab', { name: 'Properties' })); + + expect(screen.getByTestId('inspector-empty')).toBeVisible(); +}); + +test('selecting something brings its properties forward', () => { + mount(); + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'markdown', + }); + + act(() => provider.setSelection(id)); + + // A selection is the moment you want to configure the thing selected, so + // the panel follows rather than making the author find the tab. + expect(screen.getByTestId('inspector-identity')).toHaveTextContent(id); +}); + +test('the outline lists the dashboard and selects what you click', async () => { + mount(); + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'markdown', + props: { content: 'Quarterly review' }, + }); + + await userEvent.click(screen.getByRole('tab', { name: 'Outline' })); + + // Markdown is labelled by its content: five rows all reading "Markdown" + // identify nothing. + expect(screen.getByTestId(`outline-row-${id}`)).toHaveTextContent( + 'Quarterly review', + ); + + await userEvent.click(screen.getByTestId(`outline-row-${id}`)); + expect(provider.getSelection()).toBe(id); +}); + +test('choosing a row in the outline leaves you in the outline', async () => { + mount(); + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'markdown', + }); + await userEvent.click(screen.getByRole('tab', { name: 'Outline' })); + + await userEvent.click(screen.getByTestId(`outline-row-${id}`)); + + // Reading a structure means going through it. A tab that ejected to + // Properties on the first row would hide the very row it just marked as + // selected. + expect(screen.getByTestId(`outline-row-${id}`)).toBeVisible(); + expect(screen.getByRole('tab', { name: 'Outline' })).toHaveAttribute( + 'aria-selected', + 'true', + ); +}); + +test('the list of tabs says what it is a list of', () => { + mount(); + + expect( + screen.getByRole('tablist', { name: 'Editor panel views' }), + ).toBeInTheDocument(); +}); + +/** + * The panel's width belongs to whoever is authoring. A property form is the + * widest thing here, and only the author knows how much canvas they are + * willing to spend on it. + */ +const widthOf = () => + Number.parseInt(screen.getByTestId('editor-panel').style.width, 10); + +test('the panel opens wide enough to edit a block in', () => { + mount(); + + expect(widthOf()).toBe(500); +}); + +test('the handle resizes from the keyboard, so a drag is not the only way', () => { + mount(); + const handle = screen.getByTestId('panel-resize'); + handle.focus(); + + fireEvent.keyDown(handle, { key: 'ArrowRight' }); + expect(widthOf()).toBe(516); + + fireEvent.keyDown(handle, { key: 'End' }); + expect(widthOf()).toBe(800); + + fireEvent.keyDown(handle, { key: 'Home' }); + expect(widthOf()).toBe(280); +}); + +test('the handle reports the width it actually has', () => { + mount(); + + // What a screen reader announces has to be the width on screen, or the + // control is lying about the only thing it does. + expect(screen.getByTestId('panel-resize')).toHaveAttribute( + 'aria-valuenow', + '500', + ); +}); + +test('the search field is set in from the panel edge and down from the tabs', () => { + mount(); + + // Flush against both, it reads as chrome around the list rather than the + // way into it. + expect(screen.getByTestId('palette')).toHaveStyle('padding-top: 12px'); +}); + +test('a palette row can actually be dragged, as its grip promises', () => { + mount(); + const row = screen.getByTestId('palette-markdown'); + const setData = jest.fn(); + + // The grip beside the label promised a drag the row did not carry. + expect(row).toHaveAttribute('draggable', 'true'); + row.dispatchEvent( + Object.assign(new Event('dragstart', { bubbles: true }), { + dataTransfer: { setData, effectAllowed: '' }, + }), + ); + + expect(setData).toHaveBeenCalledWith( + 'application/x-dashboard-building-block', + 'markdown', + ); +}); + +test('the panel can be got out of the way, and brought back', async () => { + mount(); + + // The canvas is the work; this rail is how you act on it, and an author + // reading a dashboard at full width wants it gone without losing where + // they were in it. + await userEvent.click(screen.getByTestId('panel-collapse')); + + expect(screen.queryByRole('tab', { name: 'Building blocks' })).toBeNull(); + expect(screen.getByTestId('panel-expand')).toBeInTheDocument(); + + await userEvent.click(screen.getByTestId('panel-expand')); + + expect( + screen.getByRole('tab', { name: 'Building blocks' }), + ).toBeInTheDocument(); +}); + +test('a closed panel keeps the width it was opened at', async () => { + mount(); + const grip = screen.getByTestId('panel-resize'); + grip.focus(); + fireEvent.keyDown(grip, { key: 'End' }); + + await userEvent.click(screen.getByTestId('panel-collapse')); + await userEvent.click(screen.getByTestId('panel-expand')); + + // Closing is not resizing. A panel that reopened at the default would + // silently discard a width the author had already chosen. + expect(screen.getByTestId('editor-panel')).toHaveStyle('width: 800px'); +}); + +test('a closed panel offers no edge to drag', async () => { + mount(); + + await userEvent.click(screen.getByTestId('panel-collapse')); + + // There is nothing to size: the strip is exactly as wide as the one + // control on it, and dragging it wider would be a third state. + expect(screen.queryByTestId('panel-resize')).toBeNull(); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.tsx b/superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.tsx new file mode 100644 index 000000000000..aafe7c08f71f --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/EditorPanel.tsx @@ -0,0 +1,337 @@ +/** + * 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 { useCallback, useEffect, useRef, useState } from 'react'; +import type { KeyboardEvent, PointerEvent, ReactElement } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { Button, Tabs } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { provider, useDashboardRevision } from 'src/core/dashboard/store'; +import Inspector from './Inspector'; +import Outline from './Outline'; +import Palette from './Palette'; + +type PanelTab = 'blocks' | 'properties' | 'outline'; + +/** + * How wide the panel opens, and how far it may be dragged. + * + * The default is set by the Properties tab, which holds a block's whole set + * of fields and is the widest thing here; the palette and the outline are + * narrow whatever they are given. The ceiling leaves a usable canvas on a + * small screen. + */ +const DEFAULT_WIDTH = 500; +const MIN_WIDTH = 280; +const MAX_WIDTH = 800; +/** How far one arrow press moves the edge. */ +const KEYBOARD_STEP = 16; + +const clampWidth = (width: number): number => + Math.min(MAX_WIDTH, Math.max(MIN_WIDTH, width)); + +/** + * The rail, open or shut. + * + * Separated from the canvas with `colorSplit`, the same hairline the header + * rules itself off with, so the three edges of the authoring shell are one + * line and not three weights of one. + */ +const Rail = styled.aside` + ${({ theme }) => css` + flex-shrink: 0; + display: flex; + flex-direction: column; + position: relative; + /* The panel is a fixed-height column and the scrolling happens inside the + tab body, so the tab bar stays put however long a form gets. */ + overflow: hidden; + padding: ${theme.sizeUnit * 2}px; + border-right: 1px solid ${theme.colorSplit}; + background-color: ${theme.colorBgContainer}; + `} +`; + +const ClosedRail = styled.aside` + ${({ theme }) => css` + flex-shrink: 0; + display: flex; + justify-content: center; + padding: ${theme.sizeUnit}px; + border-right: 1px solid ${theme.colorSplit}; + background-color: ${theme.colorBgContainer}; + `} +`; + +/** + * The edge, as something to take hold of. + * + * The hit area is wide enough to aim at and the line inside it is not: a band + * of colour the width of the target announced itself as a bar being added to + * the layout rather than as the edge answering. What lights is a rule down the + * middle, which is the edge the pointer is already on. + * + * Coloured on focus as well as on hover, because focus is the state with no + * cursor to read — and the width of the authoring surface must be reachable + * without a pointer at all. + */ +const Grip = styled.div<{ $active: boolean }>` + ${({ theme, $active }) => css` + position: absolute; + top: 0; + right: 0; + bottom: 0; + /* Wide enough to be worth aiming at, sitting over the panel's own border + so the edge is the target rather than a strip beside it. */ + width: ${theme.sizeUnit * 2}px; + z-index: 1; + cursor: col-resize; + touch-action: none; + + /* Sat at the edge itself rather than a few pixels inside it: what lights + has to be the line the panel already ends on, or it reads as a second + rule appearing beside the first. */ + &::after { + content: ''; + position: absolute; + top: 0; + bottom: 0; + right: 0; + width: 2px; + background-color: ${$active ? theme.colorPrimary : 'transparent'}; + transition: background-color ${theme.motionDurationMid}; + } + + &:focus-visible { + outline: none; + } + `} +`; + +/** + * The authoring panel: one rail, three ways of working on a dashboard. + * + * Placing a block, editing one and finding one are the same activity at + * different moments, and an author is only ever doing one of them. Giving + * each its own permanent rail would spend the canvas on a choice made moment + * to moment, so they share a rail and the canvas keeps the room. + */ +export default function EditorPanel({ + onAdd, +}: { + onAdd: (type: string) => void; +}): ReactElement { + useDashboardRevision(); + const [tab, setTab] = useState<PanelTab>('blocks'); + const [width, setWidth] = useState(DEFAULT_WIDTH); + /** Whether the rail is out of the way. The width it had is kept either way. */ + const [closed, setClosed] = useState(false); + /** Whether the grip is showing itself — under the pointer, or focused. */ + const [gripped, setGripped] = useState(false); + const panel = useRef<HTMLElement | null>(null); + /** Where a drag started, so a slow drag and a fast one land the same place. */ + const from = useRef<{ x: number; width: number } | null>(null); + + /** + * Selecting something shows it, and that is a response to the selection + * changing rather than to it existing: an author who goes back to the + * palette with a block still selected stays there, because nothing changed. + * + * A selection made in the Outline is the exception. Reading a structure + * means going through it, and a tab that ejected on the first row would + * hide the very row it had just marked as selected — so the Outline sets + * the selection without moving anyone, and every other route brings + * Properties forward. + */ + const selection = provider.getSelection(); + const [shown, setShown] = useState(selection); + if (selection !== shown) { + setShown(selection); + if (selection !== undefined && tab === 'blocks') { + setTab('properties'); + } + } + + /** + * A name for the list of tabs. antd forwards unknown props to its own root + * element rather than to the `role="tablist"` it renders inside, so the + * only place this name can be put is on that element. + */ + useEffect(() => { + panel.current + ?.querySelector('[role="tablist"]') + ?.setAttribute('aria-label', t('Editor panel views')); + }, []); + + /** + * Resizing, by pointer and by key. + * + * The pointer is captured on the handle, so a drag faster than the browser + * can paint does not slip off a small target and strand the panel mid-width. + * Each move is measured from where the drag began rather than from the last + * position, so a drag that leaves the window and comes back resumes instead + * of drifting. + * + * The keys are not a convenience: a grip only a mouse can move makes the + * width of the authoring surface unreachable to anyone driving this from + * the keyboard, and the width is the whole of what the control does. + */ + const startDrag = useCallback( + (event: PointerEvent<HTMLDivElement>): void => { + from.current = { x: event.clientX, width }; + event.currentTarget.setPointerCapture?.(event.pointerId); + }, + [width], + ); + + const drag = (event: PointerEvent<HTMLDivElement>): void => { + if (from.current !== null) { + setWidth(clampWidth(from.current.width + event.clientX - from.current.x)); + } + }; + + const endDrag = (event: PointerEvent<HTMLDivElement>): void => { + from.current = null; + event.currentTarget.releasePointerCapture?.(event.pointerId); + }; + + const nudge = (event: KeyboardEvent<HTMLDivElement>): void => { + const moves: Record<string, (current: number) => number> = { + ArrowRight: current => current + KEYBOARD_STEP, + ArrowLeft: current => current - KEYBOARD_STEP, + Home: () => MIN_WIDTH, + End: () => MAX_WIDTH, + }; + const move = moves[event.key]; + if (move !== undefined) { + // Arrow and Home/End would otherwise scroll the panel out from under + // the author while they are sizing it. + event.preventDefault(); + setWidth(current => clampWidth(move(current))); + } + }; + + /** + * Out of the way, and back. + * + * The canvas is the work and this rail is how an author acts on it — but + * reading a dashboard, or showing one to someone, wants the whole width. + * Closing keeps `width` untouched rather than zeroing it, so reopening + * restores the width the author chose instead of silently discarding it. + * + * Closed, the panel is a strip carrying one control rather than nothing at + * all: a rail that vanished with no way back is a rail an author loses. + * The strip has no edge to drag, because it has no size to choose. + */ + if (closed) { + return ( + <ClosedRail data-test="editor-panel" aria-label={t('Editor panel')}> + <Button + buttonSize="xsmall" + buttonStyle="link" + data-test="panel-expand" + aria-label={t('Show the editor panel')} + aria-expanded={false} + tooltip={t('Show the editor panel')} + placement="right" + onClick={() => setClosed(false)} + > + <Icons.MenuUnfoldOutlined iconSize="m" /> + </Button> + </ClosedRail> + ); + } + + return ( + <Rail + ref={panel} + data-test="editor-panel" + aria-label={t('Editor panel')} + // The one thing that cannot be a class: it is a value the author sets by + // dragging, and a class per pixel is a stylesheet per drag. + style={{ width }} + > + <Tabs + activeKey={tab} + onChange={key => setTab(key as PanelTab)} + size="small" + style={{ flex: 1, minHeight: 0 }} + // Riding the tab bar rather than sitting above it: closing the panel + // is done to the panel, and a row of its own for one icon would cost + // the height of a row on every screen that never uses it. + tabBarExtraContent={{ + right: ( + <Button + buttonSize="xsmall" + buttonStyle="link" + data-test="panel-collapse" + aria-label={t('Hide the editor panel')} + aria-expanded + tooltip={t('Hide the editor panel')} + placement="bottom" + onClick={() => setClosed(true)} + > + <Icons.MenuFoldOutlined iconSize="m" /> + </Button> + ), + }} + items={[ + { + key: 'blocks', + label: t('Building blocks'), + children: <Palette onAdd={onAdd} />, + }, + { + key: 'properties', + label: t('Properties'), + children: <Inspector />, + }, + { + key: 'outline', + label: t('Outline'), + children: <Outline />, + }, + ]} + /> + {/* The rule's suggested `hr` is the static kind of separator: it takes + neither focus nor a value, and both are what make this one a + splitter an author can reach without a pointer. */} + <Grip + // eslint-disable-next-line jsx-a11y/prefer-tag-over-role + role="separator" + tabIndex={0} + data-test="panel-resize" + aria-orientation="vertical" + aria-label={t('Resize the editor panel')} + aria-valuenow={width} + aria-valuemin={MIN_WIDTH} + aria-valuemax={MAX_WIDTH} + $active={gripped} + onPointerDown={startDrag} + onPointerMove={drag} + onPointerUp={endDrag} + onKeyDown={nudge} + onPointerEnter={() => setGripped(true)} + onPointerLeave={() => setGripped(from.current !== null)} + onFocus={() => setGripped(true)} + onBlur={() => setGripped(false)} + /> + </Rail> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/InertControl.tsx b/superset-frontend/src/pages/DashboardBuilderV2/InertControl.tsx new file mode 100644 index 000000000000..c94aec238ad4 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/InertControl.tsx @@ -0,0 +1,89 @@ +/** + * 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 { ReactElement, ReactNode } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { Button, type ButtonProps } from '@superset-ui/core/components'; + +const NOT_AVAILABLE = t('Not available yet'); + +/** + * This prototype's controls, at two of the shared Button's own sizes. + * + * Nothing here is drawn at full size: the bars and rails are chrome around the + * work rather than the work itself, and every pixel they take is one the + * canvas does not get. But the two kinds of control on them are not read the + * same way. A word is read, and one squeezed to the smallest step there is is + * read slowly; an icon is recognised by its shape, and loses nothing there. + * + * Said in `buttonSize`, which is the prop the shared `Button` actually reads. + * `size` is antd's, and the wrapper writes its own height over whatever antd + * does with it — so every control here asked for `size="small"`, got the full + * 32px default, and was then pushed back down by a hand-written height, + * padding and font size at each site. Those helpers were a copy of this scale + * maintained beside it, free to drift from it and answering to no theme + * override; `buttonSize` is the scale itself. + */ + +/** + * An affordance that is present, named and honest about not working. + * + * Most of this prototype's chrome is one. The builder keeps its tree in + * memory and has no dashboard row behind it: nothing can be saved, + * favourited, published or refreshed, and there is no history to step + * through. Drawing them disabled says which parts of the product this page is + * still missing; drawing them live and inert would teach something false + * about all of them. + * + * `Button` renders a disabled control inside a span so its tooltip survives — + * a bare disabled button swallows the pointer events a tooltip listens for, + * and the explanation would never reach the one control that needs it. That + * is the whole reason this is a component rather than a prop spread at each + * site, and it is why a second home for it did not get a second copy. + */ +export default function Inert({ + label, + test, + buttonStyle, + /** Whether this one is read as a word rather than recognised as a shape. */ + reads, + style, + children, +}: { + label: string; + test: string; + buttonStyle?: ButtonProps['buttonStyle']; + reads?: boolean; + style?: ButtonProps['style']; + children: ReactNode; +}): ReactElement { + return ( + <Button + buttonSize={reads ? 'small' : 'xsmall'} + buttonStyle={buttonStyle} + disabled + aria-label={label} + data-test={test} + tooltip={`${label} — ${NOT_AVAILABLE}`} + placement="bottom" + style={style} + > + {children} + </Button> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/Inspector.test.tsx b/superset-frontend/src/pages/DashboardBuilderV2/Inspector.test.tsx new file mode 100644 index 000000000000..574c9d8e86b0 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/Inspector.test.tsx @@ -0,0 +1,458 @@ +/** + * 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 userEvent from '@testing-library/user-event'; +import { + fireEvent, + render, + screen, + waitFor, +} from 'spec/helpers/testing-library'; +import DashboardProvider from 'src/core/dashboard/DashboardProvider'; +import 'src/core/dashboard'; +import Inspector from './Inspector'; + +const provider = DashboardProvider.getInstance(); + +beforeEach(() => { + provider.reset(); +}); + +/** + * Brings the JSON half forward. The panel opens on the form, so every test + * that reads the raw record has to say so — which is also the assertion that + * the form is what comes first. + */ +const openJson = async () => { + await userEvent.click(screen.getByRole('tab', { name: 'JSON' })); + return screen.findByTestId('inspector-props'); +}; + +const select = (type: string, props?: Record<string, unknown>) => { + const id = provider.addBuildingBlock(provider.getRoot().id, 0, { + type, + ...(props ? { props } : {}), + }); + provider.setSelection(id); + render(<Inspector />); + return id; +}; + +test('a markdown block placed a moment ago can still be given content', async () => { + // The block arrives with no props at all. Waiting for a `content` key to + // exist before offering the field is what left a fresh block with no way + // to be given one. + const id = select('markdown'); + + await userEvent.type( + screen.getByTestId('inspector-content'), + 'Quarterly review', + ); + await userEvent.tab(); + + expect(provider.getNode(id)?.props?.content).toBe('Quarterly review'); +}); + +test('content a block already has is what the field shows', () => { + select('markdown', { content: 'Welcome' }); + + expect(screen.getByTestId('inspector-content')).toHaveValue('Welcome'); +}); + +test('a block with no prose field is still authorable through its properties', async () => { + select('echarts'); + + // A chart's dataBinding and echartsOptions have never had a hand-editing + // path. They are just keys, and the general editor reaches every one. + expect(screen.queryByTestId('inspector-content')).not.toBeInTheDocument(); + expect(await openJson()).toBeInTheDocument(); +}); + +test('applying properties writes them to the block', async () => { + const id = select('echarts'); + await openJson(); + + fireEvent.change(screen.getByTestId('inspector-props'), { + target: { value: '{"dataBinding":{"datasetId":3,"metrics":["count"]}}' }, + }); + await userEvent.click(screen.getByTestId('inspector-props-apply')); + + expect(provider.getNode(id)?.props?.dataBinding).toEqual({ + datasetId: 3, + metrics: ['count'], + }); +}); + +test('a key deleted from the properties stops reaching the block', async () => { + const id = select('echarts', { keep: 1, drop: 2 }); + await openJson(); + + fireEvent.change(screen.getByTestId('inspector-props'), { + target: { value: '{"keep":1}' }, + }); + await userEvent.click(screen.getByTestId('inspector-props-apply')); + + // `updateProps` merges, so omitting a key would silently do nothing and + // the block would go on rendering from the value it appeared to lose. + // Sending `undefined` is as close to a removal as a merge can express: the + // block reads nothing there, and the key does not survive serialization + // back into the editor. + expect(provider.getNode(id)?.props?.drop).toBeUndefined(); + expect(provider.getNode(id)?.props?.keep).toBe(1); + expect(screen.getByTestId('inspector-props')).toHaveValue( + JSON.stringify({ keep: 1 }, null, 2), + ); +}); + +test('malformed properties cannot be applied, and stay on screen to be fixed', async () => { + const id = select('echarts', { kept: true }); + await openJson(); + + fireEvent.change(screen.getByTestId('inspector-props'), { + target: { value: '{ "broken": ' }, + }); + + expect(screen.getByTestId('inspector-props-apply')).toBeDisabled(); + expect(screen.getByTestId('inspector-props-error')).toBeInTheDocument(); + // The draft is the author's; it is not reverted out from under them. + expect(screen.getByTestId('inspector-props')).toHaveValue('{ "broken": '); + expect(provider.getNode(id)?.props?.kept).toBe(true); +}); + +test('properties that are not an object are refused', async () => { + select('echarts'); + await openJson(); + + fireEvent.change(screen.getByTestId('inspector-props'), { + target: { value: '[1, 2, 3]' }, + }); + + expect(screen.getByTestId('inspector-props-apply')).toBeDisabled(); +}); + +/** The form is what the panel opens on, so this only has to find it. */ +const openForm = async () => screen.findByTestId('inspector-props-form'); + +test('properties can be edited as a form or as JSON, whichever suits', async () => { + select('echarts', { title: 'Revenue' }); + + // Two views of one set of values, not two places a value can live. The + // form is where the values are filled in and is what the panel opens on; + // JSON is where the shape is changed, since it alone can add or drop a key. + expect(screen.getByRole('tab', { name: 'Form' })).toHaveAttribute( + 'aria-selected', + 'true', + ); + await openForm(); + + expect(await openJson()).toBeInTheDocument(); +}); + +test('the form is built from the properties the block is actually holding', async () => { + select('echarts', { title: 'Revenue', limit: 10 }); + + const form = await openForm(); + + // No block type is named anywhere in this panel, so a contributed block + // gets a form on the same terms a built-in one does. + expect(form).toHaveTextContent('Title'); + expect(form).toHaveTextContent('Limit'); + expect(screen.getByDisplayValue('Revenue')).toBeInTheDocument(); +}); + +test('a value typed into the form reaches the block', async () => { + const id = select('echarts', { title: 'Revenue' }); + await openForm(); + + await userEvent.clear(screen.getByDisplayValue('Revenue')); + await userEvent.type(screen.getByRole('textbox'), 'Quarterly revenue'); + + // Awaited because JsonForms debounces what it reports by 10ms — which is + // also why this writes on change rather than on blur: a commit on blur + // fires before that debounce lands and would save the value as it stood a + // keystroke earlier. + await waitFor(() => + expect(provider.getNode(id)?.props?.title).toBe('Quarterly revenue'), + ); +}); + +test('each half of the properties editor is set down from the tabs above it', async () => { + select('echarts', { title: 'Revenue' }); + + // Flush against the tab bar, whichever label comes first reads as a caption + // on the tabs rather than as the head of the field under it — the same set + // down the panel and the palette already take from theirs. + expect((await openForm()).parentElement).toHaveStyle('padding-top: 12px'); + await openJson(); + expect(screen.getByTestId('inspector-props-json')).toHaveStyle( + 'padding-top: 12px', + ); +}); + +test('the properties on screen can be taken away as JSON', async () => { + const writeText = jest.fn(); + const original = global.navigator.clipboard; + // @ts-expect-error jsdom ships no clipboard to spy on + global.navigator.clipboard = { write: writeText, writeText }; + select('echarts', { title: 'Revenue' }); + await openJson(); + + // What is copied is what is on screen, not what the block holds — an edit + // typed but not applied yet is the state most worth being able to take + // somewhere else. + fireEvent.change(screen.getByTestId('inspector-props'), { + target: { value: '{"title":"Quarterly"}' }, + }); + await userEvent.click(screen.getByTestId('inspector-props-copy')); + + expect(writeText).toHaveBeenCalledWith('{"title":"Quarterly"}'); + // @ts-expect-error restoring what jsdom did not have + global.navigator.clipboard = original; +}); + +test("the dashboard-wide properties are the dashboard's alone", async () => { + select('echarts', { title: 'Revenue' }); + + // What a dashboard is called, who may see it and how often it refreshes are + // properties of the dashboard, not of anything placed on it — a block asked + // for a URL slug would be asking for something it has no such thing as. + expect(screen.queryByTestId('dashboard-properties')).not.toBeInTheDocument(); + [ + 'General information', + 'Access & ownership', + 'Styling', + 'Refresh settings', + 'Certification', + 'Advanced settings', + ].forEach(section => + expect(screen.queryByText(section)).not.toBeInTheDocument(), + ); + // And what a block does have stays where it is. + expect(await openForm()).toBeInTheDocument(); +}); + +test('a block with no properties yet says where they are added', async () => { + select('echarts'); + + // A form generated from values cannot offer a field for a key nothing has + // written. Rendering nothing at all would read as a broken tab. + const form = await openForm(); + + expect(form).toHaveTextContent('JSON'); +}); + +test('reverting restores what the block still has', async () => { + select('echarts', { kept: true }); + await openJson(); + + fireEvent.change(screen.getByTestId('inspector-props'), { + target: { value: '{}' }, + }); + await userEvent.click(screen.getByTestId('inspector-props-revert')); + + expect(screen.getByTestId('inspector-props')).toHaveValue( + JSON.stringify({ kept: true }, null, 2), + ); +}); + +test('the panel is set down from the tabs above it', () => { + select('markdown'); + + // Flush against the tab bar, the first line reads as a caption belonging + // to the tabs rather than to the block it names. + expect(screen.getByTestId('inspector')).toHaveStyle('padding-top: 12px'); +}); + +test('the empty state is set down too', () => { + render(<Inspector />); + + expect(screen.getByTestId('inspector-empty')).toHaveStyle( + 'padding-top: 12px', + ); +}); + +/** + * Places two blocks in a free canvas and selects the one drawn underneath. + * + * A free canvas paints its children in the order it holds them, so the first + * child is the one nothing can be put in front of by any other means. + */ +const selectInFreeCanvas = (which: 'first' | 'second') => { + const rootId = provider.getRoot().id; + provider.updateLayout(rootId, { mode: 'free' }); + const first = provider.addBuildingBlock(rootId, 0, { type: 'markdown' }); + const second = provider.addBuildingBlock(rootId, 1, { type: 'markdown' }); + provider.setSelection(which === 'first' ? first : second); + render(<Inspector />); + return { rootId, first, second }; +}; + +test('a block under another in a free canvas can be brought to the front', async () => { + const { first, second } = selectInFreeCanvas('first'); + + await userEvent.click(screen.getByTestId('inspector-bring-to-front')); + + expect(provider.getRoot().children).toEqual([second, first]); +}); + +test('bringing a block to the front leaves it where the author put it', async () => { + const { first } = selectInFreeCanvas('first'); + provider.updateLayout(first, { col: 5, row: 4 }); + + await userEvent.click(screen.getByTestId('inspector-bring-to-front')); + + expect(provider.getNode(first)?.layout).toMatchObject({ col: 5, row: 4 }); +}); + +test('a block over another in a free canvas can be sent to the back', async () => { + const { first, second } = selectInFreeCanvas('second'); + + await userEvent.click(screen.getByTestId('inspector-send-to-back')); + + expect(provider.getRoot().children).toEqual([second, first]); +}); + +const selectRoot = () => { + const rootId = provider.getRoot().id; + provider.setSelection(rootId); + render(<Inspector />); + return rootId; +}; + +test('the root is where the dashboard is arranged, now that the header does not ask', () => { + selectRoot(); + + expect(screen.getByTestId('layout-mode-switcher')).toBeInTheDocument(); +}); + +test('selecting the dashboard offers the properties the dashboard has', () => { + selectRoot(); + + // The six the saved dashboard's own properties modal asks for, reused + // whole — this panel and that modal are two ways into one set of fields. + expect(screen.getByTestId('dashboard-properties')).toBeInTheDocument(); + [ + 'General information', + 'Access & ownership', + 'Styling', + 'Refresh settings', + 'Certification', + 'Advanced settings', + ].forEach(section => expect(screen.getByText(section)).toBeInTheDocument()); +}); + +test('the dashboard is not a block, so it is not placed and cannot be deleted', () => { + selectRoot(); + + // `removeBuildingBlock` refuses the root outright, so a Delete there is a + // control that only ever raises; and the root is placed by nothing, so it + // has no column or row of its own to start at. + expect(screen.queryByTestId('inspector-delete')).not.toBeInTheDocument(); + expect( + screen.queryByTestId('inspector-section-placement'), + ).not.toBeInTheDocument(); + expect(screen.queryByTestId('inspector-identity')).not.toBeInTheDocument(); +}); + +test('the panel counts what is on the dashboard', () => { + const rootId = provider.getRoot().id; + const section = provider.addBuildingBlock(rootId, 0, { type: 'canvas' }); + provider.addBuildingBlock(section, 0, { type: 'markdown' }); + provider.addBuildingBlock(rootId, 1, { type: 'markdown' }); + selectRoot(); + + // Every block, at any depth — a section and what is inside it are both + // things on the dashboard. + expect(screen.getByTestId('dashboard-properties-counts')).toHaveTextContent( + '3 blocks, 0 filters', + ); +}); + +test('stacking is not offered in a grid canvas, where nothing overlaps', () => { + const rootId = provider.getRoot().id; + provider.addBuildingBlock(rootId, 0, { type: 'markdown' }); + const second = provider.addBuildingBlock(rootId, 1, { type: 'markdown' }); + provider.setSelection(second); + render(<Inspector />); + + expect( + screen.queryByTestId('inspector-bring-to-front'), + ).not.toBeInTheDocument(); + expect( + screen.queryByTestId('inspector-send-to-back'), + ).not.toBeInTheDocument(); +}); + +/** Places a child inside a canvas laid out in `mode`, and selects one of them. */ +const selectChildOfCanvasIn = (mode: 'grid' | 'flex') => { + const rootId = provider.getRoot().id; + provider.updateLayout(rootId, { mode }); + const childId = provider.addBuildingBlock(rootId, 0, { type: 'markdown' }); + provider.setSelection(childId); + render(<Inspector />); + return childId; +}; + +test('a flex child is not asked where it starts, because a flex line has no cells', () => { + selectChildOfCanvasIn('flex'); + + // `col`/`row` are grid coordinates. A flex container lays its children out + // in `children` order and reads neither, so offering them is offering a + // field that silently does nothing. + expect(screen.queryByTestId('inspector-col')).not.toBeInTheDocument(); + expect(screen.queryByTestId('inspector-row')).not.toBeInTheDocument(); + + // Its share of the line and its height are read in every mode. + expect(screen.getByTestId('inspector-colSpan')).toBeInTheDocument(); + expect(screen.getByTestId('inspector-rowSpan')).toBeInTheDocument(); +}); + +test('a grid child is still asked where it starts', () => { + selectChildOfCanvasIn('grid'); + + expect(screen.getByTestId('inspector-col')).toBeInTheDocument(); + expect(screen.getByTestId('inspector-row')).toBeInTheDocument(); +}); + +test('a flex container is asked the things only a flex line has', () => { + const rootId = provider.getRoot().id; + provider.updateLayout(rootId, { mode: 'flex' }); + provider.setSelection(rootId); + render(<Inspector />); + + // Documented on LayoutProps as "flex only. Ignored in every other mode" — + // and until now unreachable from the panel at all, so a flex canvas could + // be chosen and then not actually arranged. + ['direction', 'wrap', 'justify', 'align'].forEach(key => + expect(screen.getByTestId(`inspector-${key}`)).toBeInTheDocument(), + ); +}); + +test('a grid container is not asked about flow, which it does not read', () => { + const rootId = provider.getRoot().id; + provider.setSelection(rootId); + render(<Inspector />); + + ['direction', 'wrap', 'justify', 'align'].forEach(key => + expect(screen.queryByTestId(`inspector-${key}`)).not.toBeInTheDocument(), + ); + // What every mode does read stays put. + ['columns', 'gap', 'rowUnit'].forEach(key => + expect(screen.getByTestId(`inspector-${key}`)).toBeInTheDocument(), + ); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/Inspector.tsx b/superset-frontend/src/pages/DashboardBuilderV2/Inspector.tsx new file mode 100644 index 000000000000..04eb6c8e0fec --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/Inspector.tsx @@ -0,0 +1,816 @@ +/** + * 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 { useEffect, useState } from 'react'; +import type { ReactElement, ReactNode } from 'react'; +import type { dashboard as dashboardApi } from '@apache-superset/core'; +import { t } from '@apache-superset/core/translation'; +import { css, styled, useTheme } from '@apache-superset/core/theme'; +import { + Button, + EmptyState, + Form, + Input, + InputNumber, + Radio, + Switch, + Tabs, +} from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import copyTextToClipboard from 'src/utils/copy'; +import { provider, useDashboardRevision } from 'src/core/dashboard/store'; +import { blockLabel } from 'src/core/dashboard/blockLabel'; +import { resolveLayoutMode } from 'src/core/dashboard/layoutStyle'; +import DashboardProperties from './DashboardProperties'; +import LayoutModeSwitcher from './LayoutModeSwitcher'; +import PropsForm from './PropsForm'; + +type LayoutProps = dashboardApi.LayoutProps; + +/** Which layout fields belong to a container, and which to its child. */ +const CONTAINER_FIELDS: readonly { + readonly key: keyof LayoutProps; + readonly label: string; +}[] = [ + { key: 'columns', label: t('Columns') }, + { key: 'gap', label: t('Gap') }, + { key: 'rowUnit', label: t('Row height') }, +]; + +const CHILD_FIELDS: readonly { + readonly key: keyof LayoutProps; + readonly label: string; +}[] = [ + { key: 'colSpan', label: t('Width (columns)') }, + { key: 'rowSpan', label: t('Height (rows)') }, + { key: 'col', label: t('Start column') }, + { key: 'row', label: t('Start row') }, +]; + +/** + * Which fields a mode actually reads. + * + * A field the renderer ignores is worse than a missing one: it accepts a + * value, writes it to the node, and changes nothing on screen — so an author + * concludes the layout is broken rather than that the question did not apply. + * + * `columns`, `gap` and `rowUnit` are read by every mode, flex included, and + * stay put: a flex line divides into `columns` parts and gives each child the + * share its `colSpan` names (see `resolveFlexBasis`), spaces them by `gap`, + * and sizes them off `rowUnit` (see `resolveFlexMetrics`). + * + * What differs is at the two ends. `col`/`row` are coordinates in a grid, and + * a flex line has no cells to hold them — position there is `children` order, + * which is why dragging in a flex canvas reorders rather than repositions. + * And `direction`/`wrap`/`justify`/`align` are what a flex line has instead, + * documented on `LayoutProps` as "flex only, ignored in every other mode" — + * and until now not offered anywhere, so a flex canvas could be chosen and + * then not actually arranged. + */ +const GRID_ONLY_PLACEMENT: ReadonlySet<keyof LayoutProps> = new Set([ + 'col', + 'row', +]); + +const FLEX_DIRECTIONS = [ + { value: 'row', label: t('Row') }, + { value: 'column', label: t('Column') }, +]; + +const FLEX_JUSTIFY = [ + { value: 'start', label: t('Start') }, + { value: 'center', label: t('Center') }, + { value: 'end', label: t('End') }, + { value: 'space-between', label: t('Space between') }, + { value: 'space-around', label: t('Space around') }, +]; + +const FLEX_ALIGN = [ + { value: 'stretch', label: t('Stretch') }, + { value: 'start', label: t('Start') }, + { value: 'center', label: t('Center') }, + { value: 'end', label: t('End') }, +]; + +/** + * One of a small fixed set, chosen where the choices can all be seen. + * + * `Radio.Group` rather than a dropdown, which is what `LayoutModeSwitcher` + * already uses for the layout mode a few lines above these — and the shared + * `Select` does not declare `value` or `size` among the antd props it + * exposes, so driving one from the store would mean widening a type in a + * package the rest of the app depends on. + */ +const ChoiceField = ({ + label, + test, + value, + options, + onChange, +}: { + label: string; + test: string; + value: string; + options: readonly { readonly value: string; readonly label: string }[]; + onChange: (next: string) => void; +}): ReactElement => { + const theme = useTheme(); + return ( + <Form.Item label={label} style={{ marginBottom: theme.sizeUnit * 2 }}> + <Radio.Group + size="small" + value={value} + data-test={test} + onChange={event => onChange(event.target.value as string)} + > + {options.map(option => ( + <Radio.Button key={option.value} value={option.value}> + {option.label} + </Radio.Button> + ))} + </Radio.Group> + </Form.Item> + ); +}; + +/** The four a flex line is arranged by, and no other mode reads. */ +const FlexFields = ({ + nodeId, + layout, +}: { + nodeId: string; + layout: LayoutProps | undefined; +}): ReactElement => { + const theme = useTheme(); + const set = (next: Partial<LayoutProps>) => + provider.updateLayout(nodeId, next); + + return ( + <> + <ChoiceField + label={t('Direction')} + test="inspector-direction" + value={layout?.direction ?? 'row'} + options={FLEX_DIRECTIONS} + onChange={next => set({ direction: next as LayoutProps['direction'] })} + /> + <ChoiceField + label={t('Justify')} + test="inspector-justify" + value={layout?.justify ?? 'start'} + options={FLEX_JUSTIFY} + onChange={next => set({ justify: next as LayoutProps['justify'] })} + /> + <ChoiceField + label={t('Align')} + test="inspector-align" + value={layout?.align ?? 'stretch'} + options={FLEX_ALIGN} + onChange={next => set({ align: next as LayoutProps['align'] })} + /> + <Form.Item label={t('Wrap')} style={{ marginBottom: theme.sizeUnit * 2 }}> + <Switch + size="small" + data-test="inspector-wrap" + checked={layout?.wrap !== false} + onChange={wrap => set({ wrap })} + /> + </Form.Item> + </> + ); +}; + +/** + * A group of fields, and where one stops. + * + * The panel is a single column that can run several screens deep, and the + * headings alone were doing all the work of dividing it — set at the same + * weight as the field labels beneath them, they read as one more label rather + * than as the top of a group. The rule above each section is what actually + * separates them; the heading is bolder so a scan finds it first. + */ +const Group = styled.section` + ${({ theme }) => css` + margin-top: ${theme.sizeUnit * 4}px; + padding-top: ${theme.sizeUnit * 4}px; + border-top: 1px solid ${theme.colorSplit}; + `} +`; + +/** + * At the size the fields under it are labelled, and heavier. + * + * Smaller and greyer than the labels it introduces, a section heading reads as + * a caption belonging to the field above rather than as the top of the group + * below — the hierarchy inverted, with "Content" the section set in less than + * "Content" the field. Weight carries the difference instead, with the rule + * above doing the separating. + */ +const GroupTitle = styled.h4` + ${({ theme }) => css` + margin: 0 0 ${theme.sizeUnit * 2}px; + font-size: ${theme.fontSize}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + `} +`; + +/** Where the panel ends, and the one control that ends the block with it. */ +const Footer = styled.div` + ${({ theme }) => css` + margin-top: ${theme.sizeUnit * 4}px; + padding-top: ${theme.sizeUnit * 4}px; + border-top: 1px solid ${theme.colorSplit}; + `} +`; + +/** What is selected, named the way the canvas and the Outline name it. */ +const IdentityName = styled.h3` + ${({ theme }) => css` + margin: 0; + font-size: ${theme.fontSize}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + overflow-wrap: anywhere; + `} +`; + +const IdentityMeta = styled.p` + ${({ theme }) => css` + margin: ${theme.sizeUnit}px 0 0; + font-size: ${theme.fontSizeSM}px; + color: ${theme.colorTextTertiary}; + word-break: break-all; + `} +`; + +const Section = ({ + title, + test, + children, +}: { + title: string; + test: string; + children: ReactNode; +}): ReactElement => ( + <Group data-test={test}> + <GroupTitle>{title}</GroupTitle> + {children} + </Group> +); + +/** + * A number that may be absent, and stays absent when cleared. + * + * Every one of these fields has a meaning for "not set" that differs from any + * number: a child with no `col` is auto-placed, and a container with no + * `columns` takes the default. Writing a zero when a field is emptied would + * turn "let the grid decide" into "pin it at nothing". + */ +const NumberField = ({ + label, + value, + test, + onChange, +}: { + label: string; + value: number | undefined; + test: string; + onChange: (next: number | undefined) => void; +}): ReactElement => { + const theme = useTheme(); + return ( + <Form.Item label={label} style={{ marginBottom: theme.sizeUnit * 2 }}> + <InputNumber + size="small" + style={{ width: '100%' }} + value={value ?? null} + placeholder={t('Auto')} + data-test={test} + onChange={next => onChange(typeof next === 'number' ? next : undefined)} + /> + </Form.Item> + ); +}; + +/** + * Block types whose renderer reads a plain-text `content` prop. + * + * A convenience over the general props editor below, not a special case in + * the render path: prose is miserable to write inside a JSON string, with + * every newline escaped and every quote doubled. Anything not named here is + * still fully authorable — through the editor that knows no types at all. + */ +const PLAIN_TEXT_CONTENT = new Set(['markdown']); + +/** The `content` a block renders, edited where it is displayed. */ +const ContentField = ({ + nodeId, + content, +}: { + nodeId: string; + content: string; +}): ReactElement => { + const theme = useTheme(); + const [draft, setDraft] = useState(content); + // What was accepted replaces the draft, because the draft was a view of it: + // an edit made by the assistant while this panel is open has to show. + useEffect(() => setDraft(content), [content, nodeId]); + + return ( + // "Text", not "Content": the section this sits in is already called + // Content, and the two stacked read as the same word said twice. What the + // box holds is prose, which is what the label should say. + <Form.Item label={t('Text')} style={{ marginBottom: theme.sizeUnit * 2 }}> + <Input.TextArea + size="small" + rows={4} + value={draft} + data-test="inspector-content" + onChange={event => setDraft(event.target.value)} + onBlur={() => { + if (draft !== content) { + provider.updateProps(nodeId, { content: draft }); + } + }} + /> + </Form.Item> + ); +}; + +/** + * Which of its siblings a block is drawn over. + * + * A free canvas is the only place this can be asked. `react-grid-layout` + * gives an overlapping child no `z-index` of its own, so the browser falls + * back to tree order and the container's `children` order becomes the paint + * order — the last child wins, and a block earlier in the array cannot be + * put in front of a later one by moving it, resizing it, or selecting it. + * That order was never something an author could see, let alone choose; this + * is what turns it into something they can say. + * + * The two ends are the whole control on purpose. "Forward one" and "back + * one" are the same call with an index arithmetic that only means anything + * to someone already picturing the array, and the Outline is where a longer + * stack is read and reordered. + * + * Every other mode arranges its children so they do not overlap, so there is + * nothing to be in front of and the question does not arise. + */ +const StackingControls = ({ nodeId }: { nodeId: string }): ReactElement => { + const theme = useTheme(); + return ( + <div style={{ display: 'flex', gap: theme.sizeUnit }}> + {/* `secondary`, which is what this app calls a button that is not the + headline action — the style Cancel takes beside Save. Left unsaid the + shared Button draws every one of them `primary`, which is two filled + headline buttons for what is a pair of nudges. Neither of these + commits anything, and neither leads. */} + <Button + buttonSize="xsmall" + buttonStyle="secondary" + data-test="inspector-bring-to-front" + onClick={() => provider.bringToFront(nodeId)} + > + {t('Bring to front')} + </Button> + <Button + buttonSize="xsmall" + buttonStyle="secondary" + data-test="inspector-send-to-back" + onClick={() => provider.sendToBack(nodeId)} + > + {t('Send to back')} + </Button> + </div> + ); +}; + +const format = (props: Record<string, unknown> | undefined): string => + JSON.stringify(props ?? {}, null, 2); + +/** Long enough to be read, short enough not to outlast the glance at it. */ +const COPIED_FOR_MS = 1500; + +/** + * Everything a block renders from, offered whole and as text. + * + * This is the general answer to "how do I give this block its content", and + * it is general on purpose: a chart's `dataBinding` and `echartsOptions`, a + * table's `columnDefs`, and whatever an extension's block reads next year + * are all just keys here. A form per block type would need this panel to + * learn every type — the exact knowledge `BuildingBlockView` is built not to + * have, and what `PropsForm` generates a form without needing. + * + * This half is where the *shape* is decided, which is why it survives having + * a form beside it: a key that does not exist yet has no field, and can only + * be added by writing it. + * + * The draft is held until it parses and the author asks for it, so malformed + * JSON never reaches a block. What is applied is the whole record: keys the + * author deleted are sent as `undefined`, which is as close to a removal as + * a merge can express — the block reads `undefined` either way, and the key + * does not survive the next serialization back into this editor. Without + * that, deleting a line here would silently do nothing and the block would + * go on rendering from the value it appeared to lose. + */ +const PropsJsonEditor = ({ + nodeId, + props, +}: { + nodeId: string; + props: Record<string, unknown> | undefined; +}): ReactElement => { + const theme = useTheme(); + const accepted = format(props); + const [draft, setDraft] = useState(accepted); + useEffect(() => setDraft(accepted), [accepted, nodeId]); + + // Reverts on its own so the control goes back to offering the copy rather + // than reporting one indefinitely, and on any edit, because a tick beside + // text that has since changed is a tick about the wrong text. + const [copied, setCopied] = useState(false); + useEffect(() => { + if (!copied) return undefined; + const timer = setTimeout(() => setCopied(false), COPIED_FOR_MS); + return () => clearTimeout(timer); + }, [copied]); + useEffect(() => setCopied(false), [draft]); + + let parsed: Record<string, unknown> | undefined; + let error: string | undefined; + try { + const value = JSON.parse(draft); + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + error = t('Properties must be a JSON object.'); + } else { + parsed = value as Record<string, unknown>; + } + } catch (caught) { + error = caught instanceof Error ? caught.message : String(caught); + } + + const dirty = draft !== accepted; + + return ( + <> + {/* Named by the tab it is on, so the label says what these are rather + than repeating how they are being written. */} + <Form.Item + label={t('Properties')} + style={{ marginBottom: theme.sizeUnit * 2 }} + > + <Input.TextArea + size="small" + rows={8} + value={draft} + data-test="inspector-props" + onChange={event => setDraft(event.target.value)} + /> + </Form.Item> + {error !== undefined && ( + <p + data-test="inspector-props-error" + style={{ + margin: `0 0 ${theme.sizeUnit}px`, + fontSize: theme.fontSizeSM, + color: theme.colorErrorText, + }} + > + {error} + </p> + )} + <div style={{ display: 'flex', gap: theme.sizeUnit }}> + <Button + buttonSize="xsmall" + buttonStyle="primary" + data-test="inspector-props-apply" + disabled={parsed === undefined || !dirty} + onClick={() => { + if (parsed === undefined) { + return; + } + const removed = Object.keys(props ?? {}).filter( + key => !(key in parsed!), + ); + provider.updateProps(nodeId, { + ...parsed, + ...Object.fromEntries(removed.map(key => [key, undefined])), + }); + }} + > + {t('Apply')} + </Button> + {/* `secondary` beside the primary Apply — the pairing this app uses + wherever one button commits and the one next to it does not. Two + `primary` buttons side by side say both are the thing to press. */} + <Button + buttonSize="xsmall" + buttonStyle="secondary" + data-test="inspector-props-revert" + disabled={!dirty} + onClick={() => setDraft(accepted)} + > + {t('Revert')} + </Button> + {/* Set apart from the two beside it, because it is not one of them: + those commit what is in the box and this only takes a copy of it. + The draft rather than what the block holds, so what is copied is + what is on screen — including an edit not applied yet. + Confirmed in place: a panel this narrow has nowhere to put a + message, and a copy that says nothing leaves you pressing it + again to be sure. */} + <Button + buttonSize="xsmall" + buttonStyle="link" + data-test="inspector-props-copy" + aria-label={t('Copy properties as JSON')} + tooltip={copied ? t('Copied') : t('Copy properties as JSON')} + placement="bottom" + style={{ marginLeft: 'auto' }} + onClick={() => { + copyTextToClipboard(() => Promise.resolve(draft)); + setCopied(true); + }} + > + {copied ? ( + <Icons.CheckOutlined iconSize="s" /> + ) : ( + <Icons.CopyOutlined iconSize="s" /> + )} + </Button> + </div> + </> + ); +}; + +/** + * The two ways into one set of properties. + * + * They are not alternatives so much as halves. The JSON side is the whole + * record as text: it is the only one that can add a key or drop one, and the + * only one that can express a value no field knows how to hold. The form side + * is generated from the values that are already there (see + * `inferPropsSchema`), so it cannot invent a key — but it is where a value is + * actually filled in, with a control that suits its type instead of quoting + * and escaping inside a string. + * + * The form comes first and is what the panel opens on: it is the half that + * asks a question rather than handing over a record to edit, and most of what + * an author does here is change a value that already exists. The one case it + * cannot serve — a block placed a moment ago, with no properties and so no + * fields — says so and names the tab that can, rather than leaving a blank + * pane that reads as broken. + * + * Only the JSON half is wrapped in an antd `Form`, and the asymmetry is load + * bearing rather than an oversight. The generated controls render their own + * `Form.Item name={...}`, and an antd `Form` above them binds those items to + * its store — which means antd supplies the `value` and the `onChange`, + * overriding the ones JsonForms passed. The field still accepts typing; the + * edit just goes into a form store nothing reads instead of into the block. + * `SemanticLayerModal` renders JsonForms under a plain `<form>` element for + * the same reason. + */ +const PropsEditor = ({ + nodeId, + props, +}: { + nodeId: string; + props: Record<string, unknown> | undefined; +}): ReactElement => { + const theme = useTheme(); + // Set down from the tab bar, the same step the panel and the palette take + // from theirs. Flush against it, whichever label comes first reads as a + // caption belonging to the tabs rather than as the head of the field under + // it — and on the JSON side that label is the one word saying what the box + // beneath it holds. + const inset = { paddingTop: theme.sizeUnit * 3 }; + + return ( + <Tabs + size="small" + defaultActiveKey="form" + data-test="inspector-props-tabs" + items={[ + { + key: 'form', + label: t('Form'), + children: ( + <div style={inset}> + <PropsForm nodeId={nodeId} props={props} /> + </div> + ), + }, + { + key: 'json', + label: t('JSON'), + children: ( + <Form + layout="vertical" + component="div" + style={inset} + data-test="inspector-props-json" + > + <PropsJsonEditor nodeId={nodeId} props={props} /> + </Form> + ), + }, + ]} + /> + ); +}; + +/** + * Property editing over the selected node. + * + * Every field writes through `updateLayout`/`updateProps` — the same two + * calls the AI client tools make — so a change made here and one asked for in + * chat are the same edit arriving by different routes, and neither has a path + * of its own to keep correct. + * + * The Inspector holds no state the store does not: what it shows is read on + * each render, so an assistant edit updates it like anything else. + */ +export default function Inspector(): ReactElement { + useDashboardRevision(); + const theme = useTheme(); + const selection = provider.getSelection(); + const node = + selection === undefined ? undefined : provider.getNode(selection); + + // Set down from the tab bar above. Whatever comes first here — the + // identity of what is selected, or the line saying nothing is — reads as a + // caption hanging off the tabs when it starts flush against them. + const inset = { paddingTop: theme.sizeUnit * 3 }; + + if (!node) { + return ( + <div data-test="inspector-empty" style={inset}> + <EmptyState + size="small" + image="empty.svg" + title={t('Nothing selected')} + description={t( + 'Pick a block on the canvas, or a row in the Outline, to edit it here.', + )} + /> + </div> + ); + } + + const isContainer = node.children !== undefined; + // The root is the dashboard rather than a block on it, so what it is asked + // for is different in kind: what it is called, who it belongs to, how it + // looks — not where it sits or what it renders. Arranging is the one thing + // the two have in common, and it comes below as it does for any container. + const isRoot = node.id === provider.getRoot().id; + const parentId = provider.getParentId(node.id); + const parent = + parentId === undefined ? undefined : provider.getNode(parentId); + // Only where children can overlap is there anything to be in front of. + const stacks = + parent !== undefined && resolveLayoutMode(parent.layout) === 'free'; + const content = node.props?.content; + // Offered for a block whose renderer reads prose, whether or not it has + // any yet — a markdown block placed a moment ago has no props at all, and + // waiting for a `content` key to exist before showing the field is what + // left it with no way to be given one. + const takesText = + typeof content === 'string' || PLAIN_TEXT_CONTENT.has(node.type); + + return ( + <div data-test="inspector" style={{ ...inset, fontSize: theme.fontSizeSM }}> + {isRoot ? ( + <DashboardProperties /> + ) : ( + // The same shape the dashboard's own panel opens with: what this is, + // then the smaller print about it. The name is `blockLabel`'s — the + // one the canvas header and the Outline row already use — so a block + // is called one thing in all three places, and the type and id sit + // under it as what they are, a fact about the block rather than its + // name. + <div data-test="inspector-identity"> + <IdentityName>{blockLabel(node.type, node.props)}</IdentityName> + <IdentityMeta> + {node.type} · {node.id} + </IdentityMeta> + </div> + )} + + {/* Outside the `Form` below, and each half of it wrapping its own — + the generated form must not have an antd `Form` above it. See + `PropsEditor`. */} + {!isRoot && ( + <Section title={t('Content')} test="inspector-section-content"> + {takesText && ( + <Form layout="vertical" component="div"> + <ContentField + nodeId={node.id} + content={typeof content === 'string' ? content : ''} + /> + </Form> + )} + <PropsEditor nodeId={node.id} props={node.props} /> + </Section> + )} + + {/* Labels above their fields: beside them halves the width left for the + control, in the panel that most needs the room. */} + <Form layout="vertical" component="div"> + {isContainer && ( + <Section + title={t('Arrangement')} + test="inspector-section-arrangement" + > + {/* One run of fields: the mode is asked as a field now, so it + spaces itself against the rest rather than needing a gap put + between it and them. */} + <LayoutModeSwitcher nodeId={node.id} /> + {CONTAINER_FIELDS.map(field => ( + <NumberField + key={field.key} + label={field.label} + test={`inspector-${field.key}`} + value={node.layout?.[field.key] as number | undefined} + onChange={next => + provider.updateLayout(node.id, { [field.key]: next }) + } + /> + ))} + {resolveLayoutMode(node.layout) === 'flex' && ( + <FlexFields nodeId={node.id} layout={node.layout} /> + )} + </Section> + )} + + {/* The root is placed by nothing — it is what everything else is + placed in — so it has no column, row or span of its own to set. */} + {!isRoot && ( + <Section title={t('Placement')} test="inspector-section-placement"> + {CHILD_FIELDS.filter( + field => + !( + parent !== undefined && + resolveLayoutMode(parent.layout) === 'flex' && + GRID_ONLY_PLACEMENT.has(field.key) + ), + ).map(field => ( + <NumberField + key={field.key} + label={field.label} + test={`inspector-${field.key}`} + value={node.layout?.[field.key] as number | undefined} + onChange={next => + provider.updateLayout(node.id, { [field.key]: next }) + } + /> + ))} + {stacks && <StackingControls nodeId={node.id} />} + </Section> + )} + </Form> + + {/* `removeBuildingBlock` refuses the root, so offering it here would be + a button that only ever raises. + + Ruled off from the fields above it rather than following them at a + gap: everything else in this column changes the block, and this is + the one control that ends it. The rule is the same one that divides + the sections, so the panel reads as ending here rather than as + having one more field. */} + {!isRoot && ( + <Footer> + <Button + buttonSize="xsmall" + // `buttonStyle`, not antd's own `danger`: the shared Button reads + // the former and derives the latter from it, so a bare `danger` + // is dropped and the control falls back to `primary` — which drew + // the one destructive thing in this panel as its filled headline + // action. + buttonStyle="danger" + icon={<Icons.DeleteOutlined iconSize="s" />} + data-test="inspector-delete" + onClick={() => provider.removeBuildingBlock(node.id)} + > + {t('Delete block')} + </Button> + </Footer> + )} + </div> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/LayoutModeSwitcher.test.tsx b/superset-frontend/src/pages/DashboardBuilderV2/LayoutModeSwitcher.test.tsx new file mode 100644 index 000000000000..abafeeb65c6a --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/LayoutModeSwitcher.test.tsx @@ -0,0 +1,96 @@ +/** + * 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 userEvent from '@testing-library/user-event'; +import { render, screen } from 'spec/helpers/testing-library'; +import DashboardProvider from 'src/core/dashboard/DashboardProvider'; +import LayoutModeSwitcher from './LayoutModeSwitcher'; + +const provider = DashboardProvider.getInstance(); + +beforeEach(() => { + provider.reset(); +}); + +const mount = () => { + const rootId = provider.getRoot().id; + provider.addBuildingBlock(rootId, 0, { type: 'markdown' }); + render(<LayoutModeSwitcher nodeId={rootId} />); + return rootId; +}; + +test('a container that never named a mode reads as a grid', () => { + mount(); + + // Not a default the control invents for display: it is the mode the + // container actually arranges in, so the button and the canvas agree. + expect(screen.getByTestId('layout-mode-grid')).toBeChecked(); +}); + +test('choosing a mode writes it to the container', async () => { + const rootId = mount(); + + await userEvent.click(screen.getByTestId('layout-mode-flex')); + + expect(provider.getNode(rootId)?.layout?.mode).toBe('flex'); +}); + +test('the control follows a mode set anywhere else', () => { + const rootId = provider.getRoot().id; + provider.addBuildingBlock(rootId, 0, { type: 'markdown' }); + provider.updateLayout(rootId, { mode: 'free' }); + + render(<LayoutModeSwitcher nodeId={rootId} />); + + // `updateLayout` is what an AI tool call goes through. Asking the + // assistant for a free canvas and pressing Free are the same edit, so the + // control has to reflect whichever happened last rather than its own idea. + expect(screen.getByTestId('layout-mode-free')).toBeChecked(); +}); + +test('changing the mode leaves every block where it was', async () => { + const rootId = provider.getRoot().id; + const blockId = provider.addBuildingBlock(rootId, 0, { + type: 'markdown', + layout: { col: 3, row: 2, colSpan: 6, rowSpan: 4 }, + }); + render(<LayoutModeSwitcher nodeId={rootId} />); + + await userEvent.click(screen.getByTestId('layout-mode-flex')); + await userEvent.click(screen.getByTestId('layout-mode-grid')); + + // Grid and Free read the same four coordinates and Flex ignores them, so + // a round trip through Flex must not be where a position quietly dies. + expect(provider.getNode(blockId)?.layout).toMatchObject({ + col: 3, + row: 2, + colSpan: 6, + rowSpan: 4, + }); +}); + +test('a node that holds no children offers no arrangement', () => { + const rootId = provider.getRoot().id; + const leafId = provider.addBuildingBlock(rootId, 0, { type: 'markdown' }); + + render(<LayoutModeSwitcher nodeId={leafId} />); + + // A leaf arranges nothing. Offering it a layout mode would be offering a + // setting with nothing to apply to. + expect(screen.queryByTestId('layout-mode-switcher')).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/LayoutModeSwitcher.tsx b/superset-frontend/src/pages/DashboardBuilderV2/LayoutModeSwitcher.tsx new file mode 100644 index 000000000000..ae61f668827d --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/LayoutModeSwitcher.tsx @@ -0,0 +1,141 @@ +/** + * 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 { ReactElement } from 'react'; +import type { dashboard as dashboardApi } from '@apache-superset/core'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { Form, Radio, Tooltip } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { provider, useDashboardRevision } from 'src/core/dashboard/store'; +import { resolveLayoutMode } from 'src/core/dashboard/layoutStyle'; + +type LayoutMode = dashboardApi.LayoutMode; + +/** + * What each mode is for, in the author's terms rather than the schema's. + * + * Every one of them says what happens to a block, because that is the only + * part an author can see: whether the space a block leaves behind closes, and + * whether a block may sit over another. The names on the buttons are short + * enough to read at a glance; the sentence is where the difference lives. + */ +const MODES: readonly { + readonly key: LayoutMode; + readonly label: string; + readonly hint: string; + readonly icon: ReactElement; +}[] = [ + { + key: 'grid', + label: t('Grid'), + hint: t('Blocks snap to columns and close up the space above them.'), + icon: <Icons.TableOutlined iconSize="s" />, + }, + { + key: 'flex', + label: t('Flex'), + hint: t('Blocks flow along a line and wrap, sharing it by width.'), + icon: <Icons.LayoutOutlined iconSize="s" />, + }, + { + key: 'free', + label: t('Free'), + hint: t('Blocks stay exactly where you put them, and may overlap.'), + icon: <Icons.AppstoreOutlined iconSize="s" />, + }, +]; + +/** Spaced like the number fields it sits above, so the section reads evenly. */ +const ModeField = styled(Form.Item)` + ${({ theme }) => css` + margin-bottom: ${theme.sizeUnit * 2}px; + `} +`; + +const ModeLabel = styled.span` + ${({ theme }) => css` + display: inline-flex; + align-items: center; + gap: ${theme.sizeUnit}px; + `} +`; + +/** + * How one container arranges its children. + * + * The control edits the container's own `layout.mode`, which is the same + * field an AI tool call writes through `dashboard.updateLayout` — so asking + * the assistant for a free canvas and pressing Free here are the same edit, + * and the button reflects whichever of the two happened last. + * + * A mode change moves nothing. Grid and Free read the same four coordinates + * per child, so switching between them only changes whether the container + * compacts them; switching to Flex leaves those coordinates untouched in the + * store, so coming back finds every block where it was left. + */ +export default function LayoutModeSwitcher({ + nodeId, +}: { + nodeId: string; +}): ReactElement | null { + useDashboardRevision(); + const node = provider.getNode(nodeId); + if (!node?.children) { + return null; + } + const mode = resolveLayoutMode(node.layout); + + return ( + // Asked the way every other field in this section is asked — label above, + // control beneath. Beside its control it was the one question in the + // Arrangement section reading left to right, which made a setting that + // belongs with the columns and the gap look like chrome sitting over them. + <ModeField label={t('Layout')} data-test="layout-mode-switcher"> + <Radio.Group + size="small" + value={mode} + // The `Form.Item` label has no control id to point at without a + // `name`, so the group carries its own name rather than going + // unannounced. + aria-label={t('Layout')} + onChange={event => + provider.updateLayout(nodeId, { + mode: event.target.value as LayoutMode, + }) + } + > + {MODES.map(option => ( + <Tooltip key={option.key} title={option.hint} placement="bottom"> + <Radio.Button + value={option.key} + data-test={`layout-mode-${option.key}`} + > + {/* The icon and the name are one label, spaced by the layout + rather than by a text node holding a space. */} + <ModeLabel> + {option.icon} + {option.label} + </ModeLabel> + </Radio.Button> + </Tooltip> + ))} + </Radio.Group> + </ModeField> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/Outline.test.tsx b/superset-frontend/src/pages/DashboardBuilderV2/Outline.test.tsx new file mode 100644 index 000000000000..78581a888cbe --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/Outline.test.tsx @@ -0,0 +1,122 @@ +/** + * 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 userEvent from '@testing-library/user-event'; +import { fireEvent, render, screen } from 'spec/helpers/testing-library'; +import DashboardProvider from 'src/core/dashboard/DashboardProvider'; +import BuildingBlockView from 'src/core/dashboard/BuildingBlockView'; +import 'src/core/dashboard'; +import Outline from './Outline'; + +const provider = DashboardProvider.getInstance(); + +/** + * Which elements were scrolled to, in order. jsdom has no layout, so the call + * is the only observable part of scrolling — what it was called on is what + * says the right block was reached for. + */ +const scrolled: Element[] = []; + +beforeEach(() => { + provider.reset(); + scrolled.length = 0; + Element.prototype.scrollIntoView = jest.fn(function record(this: Element) { + scrolled.push(this); + }); +}); + +/** The outline next to the canvas it reaches into: the pairing under test. */ +const mount = () => + render( + <> + <Outline /> + <BuildingBlockView nodeId={provider.getRoot().id} /> + </>, + ); + +const addMarkdown = (content: string): string => + provider.addBuildingBlock(provider.getRoot().id, 0, { + type: 'markdown', + props: { content }, + }); + +test('an empty dashboard says so instead of showing an empty tree', () => { + mount(); + + expect(screen.getByTestId('outline-empty')).toBeInTheDocument(); +}); + +test('a row is listed for every block, labelled by its content', () => { + const id = addMarkdown('Revenue by region'); + mount(); + + // Scoped to the row rather than looked for on the page: the block's own + // header on the canvas carries the same name, by design, so a bare text + // query would match twice and prove neither. + expect(screen.getByRole('tree')).toBeInTheDocument(); + expect(screen.getByTestId(`outline-row-${id}`)).toHaveTextContent( + 'Revenue by region', + ); +}); + +test('choosing a row selects that block', async () => { + const id = addMarkdown('Revenue by region'); + mount(); + + await userEvent.click(screen.getByTestId(`outline-row-${id}`)); + + expect(provider.getSelection()).toBe(id); +}); + +test('choosing a row brings its block into view on the canvas', async () => { + const below = addMarkdown('Down the page'); + addMarkdown('Up the top'); + const { container } = mount(); + + await userEvent.click(screen.getByTestId(`outline-row-${below}`)); + + // The point of the outline is reaching blocks the canvas is worst at + // offering — including one scrolled out of sight. Selecting it and leaving + // it off screen marks a block the author cannot see. + expect(scrolled).toEqual([ + container.querySelector(`[data-node-id="${below}"]`), + ]); +}); + +test('the keyboard reaches a block the same way the pointer does', () => { + const id = addMarkdown('Revenue by region'); + const { container } = mount(); + + fireEvent.keyDown(screen.getByTestId(`outline-row-${id}`), { key: 'Enter' }); + + expect(provider.getSelection()).toBe(id); + expect(scrolled).toEqual([container.querySelector(`[data-node-id="${id}"]`)]); +}); + +test('a row whose block is not on screen still selects', async () => { + // The outline can outlive the canvas it describes — rendered on its own + // here, but equally a block behind a collapsed container. Selection is the + // part that must not depend on finding an element to scroll. + const id = addMarkdown('Revenue by region'); + render(<Outline />); + + await userEvent.click(screen.getByTestId(`outline-row-${id}`)); + + expect(provider.getSelection()).toBe(id); + expect(scrolled).toEqual([]); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/Outline.tsx b/superset-frontend/src/pages/DashboardBuilderV2/Outline.tsx new file mode 100644 index 000000000000..2cd27aa0b953 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/Outline.tsx @@ -0,0 +1,299 @@ +/** + * 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 { ReactElement } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { EmptyState } from '@superset-ui/core/components'; +import { views } from 'src/core/views'; +import { DASHBOARD_BUILDING_BLOCKS_LOCATION } from 'src/core/dashboard/resolveBuildingBlockView'; +import { provider, useDashboardRevision } from 'src/core/dashboard/store'; + +/** How long a label may run before it is cut. */ +const LABEL_LIMIT = 40; + +/** + * What a node is called in the outline. + * + * A registered block's own name first, because that is what the author chose + * it by in the palette. Markdown gets its opening words instead — a list of + * five rows all reading "Markdown" identifies nothing, and the content is the + * only thing that tells them apart. + */ +const labelOf = (type: string, props: Record<string, unknown> | undefined) => { + const content = props?.content; + if (typeof content === 'string' && content.trim() !== '') { + const text = content.trim().replace(/\s+/g, ' '); + return text.length > LABEL_LIMIT ? `${text.slice(0, LABEL_LIMIT)}…` : text; + } + const registered = views + .getViews(DASHBOARD_BUILDING_BLOCKS_LOCATION) + ?.find(view => view.id === type); + return registered?.name ?? type; +}; + +/** + * Selects a node and shows it where it lives. + * + * Marking a block as selected is only half of reaching it: the rows this + * panel exists for are the ones for blocks the canvas is currently not + * offering, and an outline that selected something off screen would leave an + * author looking at a canvas that appears not to have answered. The block's + * own element carries `data-node-id` (see `BuildingBlockView`), so the canvas + * needs no wiring back to here. + * + * `nearest` rather than `center`: this fires on every row, and reading down a + * list of blocks that are already in view should not move the canvas under + * them. Nothing happens at all when the element is absent — a node can be in + * the tree without being rendered — and selection has already been set by + * then either way. + */ +const select = (nodeId: string): void => { + provider.setSelection(nodeId); + document + .querySelector(`[data-node-id="${nodeId}"]`) + ?.scrollIntoView({ block: 'nearest', behavior: 'smooth' }); +}; + +/** + * A node, as a tile to read and to reach through. + * + * The same tile the palette is built from, because the two panels are the same + * kind of thing seen twice: a tree of blocks, one of blocks you could place + * and one of blocks you did. A block that is a bordered tile with a name in + * the Building blocks tab and a bare line of text in the Outline reads as two + * different kinds of object. + * + * What it does not borrow is the grip: these do not drag. What it adds is + * selection, which the palette has no equivalent of — the accent border and + * fill, kept through hover so a pointer passing over the selected tile does + * not read as unselecting it. + */ +const OutlineTile = styled.div<{ $selected: boolean }>` + ${({ theme, $selected }) => css` + position: relative; + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + padding: ${theme.sizeUnit * 2}px; + border: 1px solid ${$selected ? theme.colorPrimary : theme.colorBorder}; + border-radius: ${theme.borderRadiusSM}px; + background-color: ${ + $selected ? theme.colorPrimaryBg : theme.colorFillQuaternary + }; + font-size: ${theme.fontSizeSM}px; + color: ${$selected ? theme.colorPrimaryText : theme.colorText}; + cursor: pointer; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + transition: + border-color ${theme.motionDurationMid}, + background-color ${theme.motionDurationMid}; + + &:hover { + border-color: ${ + $selected ? theme.colorPrimary : theme.colorPrimaryBorderHover + }; + background-color: ${ + $selected ? theme.colorPrimaryBgHover : theme.colorFillTertiary + }; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimaryBorder}; + outline-offset: -2px; + } + `} +`; + +/** The tree itself: the reset, and the space between what is at its top. */ +const List = styled.ul` + ${({ theme }) => css` + list-style: none; + margin: 0; + padding: 0; + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit}px; + `} +`; + +/** + * A node's children, and the guide that says they are its. + * + * The same treatment the palette gives a shelf, for the same reason and at the + * same measurements: indentation alone leaves the eye to infer the grouping + * from an edge that is not drawn, and here the nesting can run deeper than the + * palette's single level, so there is that much more to infer. + * + * Drawn from here rather than from the tile, because unlike the palette a tile + * in this tree may itself hold a branch — and the guide has to clear that + * whole subtree to reach the sibling below it. So the vertical is drawn on the + * list item, which is the tile *and* everything under it; only the last item + * draws it on its own tile instead, stopping at the stub. A guide that carries + * on past the final tile reads as a branch with something still to come; one + * drawn on every tile would break wherever a node had children. + */ +const Branch = styled(List)` + ${({ theme }) => css` + margin-top: ${theme.sizeUnit}px; + margin-left: ${theme.sizeUnit * 2}px; + padding-left: ${theme.sizeUnit * 3}px; + + & > li { + position: relative; + } + + /* The vertical, past everything this item holds, to the one below it. */ + & > li:not(:last-child)::before, + /* The last item's, stopping where its own stub meets it. */ + & > li:last-child > [role='treeitem']::before, + /* Every item's stub back to the guide. */ + & > li > [role='treeitem']::after { + content: ''; + position: absolute; + left: -${theme.sizeUnit * 3}px; + background-color: ${theme.colorBorder}; + } + + & > li:not(:last-child)::before { + top: -${theme.sizeUnit}px; + bottom: -${theme.sizeUnit}px; + width: 1px; + } + + & > li:last-child > [role='treeitem']::before { + top: -${theme.sizeUnit}px; + bottom: 50%; + width: 1px; + } + + & > li > [role='treeitem']::after { + top: 50%; + width: ${theme.sizeUnit * 3}px; + height: 1px; + } + `} +`; + +/** + * Set down from the tab bar and in from the panel edge, the same step the + * palette and the inspector take from theirs. Flush against the tabs, the + * first row read as a caption hanging off them rather than as the top of a + * list — and the three tabs of one panel should start on one line. + */ +const Panel = styled.div` + ${({ theme }) => css` + padding: ${theme.sizeUnit * 3}px ${theme.sizeUnit}px 0; + `} +`; + +const Row = ({ + nodeId, + depth, +}: { + nodeId: string; + depth: number; +}): ReactElement | null => { + const node = provider.getNode(nodeId); + if (!node) { + return null; + } + const selected = provider.getSelection() === nodeId; + const children = node.children ?? []; + + return ( + <li role="none"> + <OutlineTile + role="treeitem" + aria-level={depth + 1} + aria-selected={selected} + tabIndex={selected ? 0 : -1} + data-test={`outline-row-${nodeId}`} + $selected={selected} + onClick={() => select(nodeId)} + onKeyDown={event => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + select(nodeId); + } + }} + > + {labelOf(node.type, node.props)} + </OutlineTile> + {children.length > 0 && ( + <Branch + // The tags the rule suggests are document sections, not tree + // structure. `group` inside `tree` is the pattern WAI-ARIA + // specifies for a treeitem's children, and a screen reader's tree + // navigation reads it — no semantic element means this. + // eslint-disable-next-line jsx-a11y/prefer-tag-over-role + role="group" + > + {children.map(childId => ( + <Row key={childId} nodeId={childId} depth={depth + 1} /> + ))} + </Branch> + )} + </li> + ); +}; + +/** + * The dashboard's structure, as something to read and to reach into. + * + * The canvas shows what a dashboard looks like; this shows what it is made + * of. That matters most for exactly the blocks the canvas is worst at + * offering — one nested inside a container, one scrolled out of view, one + * sized so small there is nothing to click. + * + * Choosing a row selects it and leaves the author here. Reading a structure + * means going through it, and a panel that ejected to Properties on the first + * row would hide the very row it had just marked as selected. + */ +export default function Outline(): ReactElement { + useDashboardRevision(); + const root = provider.getRoot(); + const children = root.children ?? []; + + if (children.length === 0) { + return ( + <Panel data-test="outline-empty"> + <EmptyState + size="small" + image="empty-dashboard.svg" + title={t('Nothing on the dashboard yet')} + description={t( + 'Blocks you place show up here, in the order they sit on the canvas.', + )} + /> + </Panel> + ); + } + + return ( + <Panel> + <List role="tree" aria-label={t('Dashboard outline')} data-test="outline"> + {children.map(childId => ( + <Row key={childId} nodeId={childId} depth={0} /> + ))} + </List> + </Panel> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/Palette.tsx b/superset-frontend/src/pages/DashboardBuilderV2/Palette.tsx new file mode 100644 index 000000000000..79e69dfa3322 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/Palette.tsx @@ -0,0 +1,424 @@ +/** + * 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 { useMemo, useState } from 'react'; +import type { ReactElement, ReactNode } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled } from '@apache-superset/core/theme'; +import { EmptyState, Input } from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { views } from 'src/core/views'; +import { DASHBOARD_BUILDING_BLOCKS_LOCATION } from 'src/core/dashboard/resolveBuildingBlockView'; +import { isContainerType } from 'src/core/dashboard/DashboardProvider'; +import { PALETTE_MIME } from 'src/core/dashboard/placement'; + +/** + * Which shelf a block sits on. + * + * Derived from the one distinction this fork actually records: whether + * placing the type produces something other blocks can go inside. That is a + * checkable property of the node the provider builds, not a category anybody + * maintains, so a block registered by an extension tomorrow is shelved + * correctly without this file learning its name. + * + * There is deliberately no Extensions shelf. A registered `View` carries an + * id, a name and a description and nothing that says who contributed it, so + * built-in and extension-contributed blocks are genuinely indistinguishable + * here. Splitting them on a dotted-id naming convention would be a guess + * dressed as a fact; the shelf can be added the day provenance is. + */ +const SHELVES: readonly { + readonly key: 'structure' | 'content'; + readonly name: string; +}[] = [ + { key: 'structure', name: t('Structure') }, + { key: 'content', name: t('Content') }, +]; + +export interface PaletteEntry { + readonly type: string; + readonly label: string; + readonly description?: string; + readonly shelf: 'structure' | 'content'; +} + +/** Everything registered as a building block, in the order it was registered. */ +export const paletteEntries = (): readonly PaletteEntry[] => + (views.getViews(DASHBOARD_BUILDING_BLOCKS_LOCATION) ?? []).map(view => ({ + type: view.id, + label: view.name, + description: view.description, + shelf: isContainerType(view.id) ? 'structure' : 'content', + })); + +const matches = (entry: PaletteEntry, query: string): boolean => { + if (query === '') { + return true; + } + const needle = query.toLowerCase(); + return ( + entry.label.toLowerCase().includes(needle) || + (entry.description ?? '').toLowerCase().includes(needle) + ); +}; + +/** + * The panel's own scroll column. + * + * The search field stays put and the shelves move under it: a list long enough + * to scroll is exactly when the field that filters it must not scroll away. + */ +const Column = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + /* The field is not the first item of the list it filters, and at a tighter + gap it read as one — the shelf below it sat as close to it as its own + tiles sit to each other. The space is what separates searching the + palette from reading it. */ + gap: ${theme.sizeUnit * 5}px; + min-height: 0; + /* Set down from the tab bar and in from the panel edge: a search field + flush against both reads as part of the chrome around the list rather + than the way into it. */ + padding: ${theme.sizeUnit * 3}px ${theme.sizeUnit}px 0; + `} +`; + +const Shelves = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit}px; + overflow-y: auto; + min-height: 0; + `} +`; + +/** + * A shelf's name, heavier than what is on the shelf. + * + * Set in the secondary colour it came out lighter than the rows beneath it, + * which reads as the shelf belonging to the list rather than the list to the + * shelf. Same size as its rows and heavier, the same trade the Inspector's + * section headings make. + * + * The hover is a wash rather than the fill a row takes, because a shelf that + * lights the way a row lights is a row: this one opens and closes a group, and + * should not look like something to place. + */ +const ShelfButton = styled.button` + ${({ theme }) => css` + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + width: 100%; + padding: ${theme.sizeUnit}px; + border: 0; + border-radius: ${theme.borderRadiusSM}px; + background: none; + color: ${theme.colorText}; + font-size: ${theme.fontSizeSM}px; + font-weight: ${theme.fontWeightStrong}; + text-align: left; + cursor: pointer; + transition: background-color ${theme.motionDurationMid}; + + /* The toggle is the shelf's state made visible — plus for shut, minus for + open — and it is quieter than the name it sits beside, which is what is + actually being read. */ + .palette-toggle { + display: flex; + flex: 0 0 auto; + color: ${theme.colorTextTertiary}; + } + + &:hover { + background-color: ${theme.colorFillQuaternary}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimaryBorder}; + outline-offset: -2px; + } + `} +`; + +/** + * What ties a shelf to the blocks on it. + * + * The tiles are indented under their shelf, and indentation alone leaves the + * eye to infer the grouping from an edge that is not drawn. The guide down the + * left is that edge, and each tile reaches back to it with a stub — so a tile + * belongs to the shelf above it visibly rather than by inference. + * + * The guide is drawn by the tiles rather than here (see `BlockTile`), because + * where it has to stop is the middle of the last tile and this element cannot + * know where that is. Drawn in `colorBorder`, which is what this app draws one + * thing off from another with — the same one the tiles are drawn with, so the + * guide and what it holds are one weight of line. + */ +const Branch = styled.div` + ${({ theme }) => css` + display: flex; + flex-direction: column; + gap: ${theme.sizeUnit}px; + margin-left: ${theme.sizeUnit * 2}px; + padding-left: ${theme.sizeUnit * 3}px; + `} +`; + +/** + * A block, as a tile to pick up. + * + * A bordered tile rather than a bare row: what these are is a set of things + * that get dragged onto a canvas and become boxes there, and a tile with an + * edge is a thing you can take hold of in a way a line of text is not. The + * grip states the same thing in the same place on every one of them. + * + * The stub reaching left is what joins the tile to its shelf's guide — see + * `Branch`. It is drawn from the tile rather than by the shelf because only + * the tile knows where its own middle is. + * + * `grab` becoming `grabbing`, and the border taking the accent under the + * pointer, are the two halves of saying this can be dragged. The focus ring is + * the same answer for a keyboard, which the row had no visible reply to at all. + * Every colour here is a token: the tile has to hold up in both themes, and a + * literal only ever suits the one it was picked in. + */ +const BlockTile = styled.button` + ${({ theme }) => css` + position: relative; + display: flex; + align-items: center; + gap: ${theme.sizeUnit * 2}px; + width: 100%; + padding: ${theme.sizeUnit * 2}px; + border: 1px solid ${theme.colorBorder}; + border-radius: ${theme.borderRadiusSM}px; + background-color: ${theme.colorFillQuaternary}; + color: ${theme.colorText}; + font-size: ${theme.fontSizeSM}px; + text-align: left; + cursor: grab; + transition: + border-color ${theme.motionDurationMid}, + background-color ${theme.motionDurationMid}; + + /* The shelf's guide, and this tile's stub back to it. + The vertical runs from above the tile — bridging the gap to the one + before it — down to the tile's own bottom, so the segments meet and read + as one line. The last tile stops it at the stub: a guide that carries on + past the final tile reads as a shelf with something still to come. */ + &::before, + &::after { + content: ''; + position: absolute; + left: -${theme.sizeUnit * 3}px; + background-color: ${theme.colorBorder}; + } + + &::before { + top: -${theme.sizeUnit}px; + bottom: 0; + width: 1px; + } + + &:last-child::before { + bottom: 50%; + } + + &::after { + top: 50%; + width: ${theme.sizeUnit * 3}px; + height: 1px; + } + + &:hover { + border-color: ${theme.colorPrimaryBorderHover}; + background-color: ${theme.colorFillTertiary}; + } + + &:active { + cursor: grabbing; + border-color: ${theme.colorPrimary}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimaryBorder}; + outline-offset: -2px; + } + + /* The grip is part of the tile's answer rather than a control of its own, + so it strengthens with the tile rather than on its own hover. */ + .palette-grip { + display: flex; + flex: 0 0 auto; + color: ${theme.colorTextQuaternary}; + transition: color ${theme.motionDurationMid}; + } + + &:hover .palette-grip, + &:focus-visible .palette-grip { + color: ${theme.colorTextTertiary}; + } + `} +`; + +/** + * A disclosure the palette drives rather than the browser, so a search can + * reveal through a shelf the author collapsed and give it back on clearing. + */ +const Disclosure = ({ + name, + open, + onToggle, + children, +}: { + name: string; + open: boolean; + onToggle: () => void; + children: ReactNode; +}): ReactElement => ( + <div data-test={`palette-shelf-${name.toLowerCase()}`}> + <ShelfButton + type="button" + aria-expanded={open} + // The toggle carries an `aria-label` of its own, which would otherwise + // join the shelf's name and announce the shape of the glyph first. + aria-label={name} + onClick={onToggle} + > + <span className="palette-toggle" aria-hidden> + {open ? ( + <Icons.MinusSquareOutlined iconSize="s" /> + ) : ( + <Icons.PlusSquareOutlined iconSize="s" /> + )} + </span> + {name} + </ShelfButton> + {open && <Branch>{children}</Branch>} + </div> +); + +/** + * The building blocks, as things to place. + * + * The list is the registry's — `views.getViews('dashboard.buildingBlocks')`, + * the same call `BuildingBlockView` resolves a renderer through. Registering + * a block makes it placeable and unregistering one removes it, with no list + * here to keep in agreement. + */ +export default function Palette({ + onAdd, +}: { + onAdd: (type: string) => void; +}): ReactElement { + const [query, setQuery] = useState(''); + const [closed, setClosed] = useState<ReadonlySet<string>>(new Set()); + + const entries = useMemo(paletteEntries, []); + const found = entries.filter(entry => matches(entry, query)); + const searching = query.trim() !== ''; + const isOpen = (key: string): boolean => searching || !closed.has(key); + const toggle = (key: string): void => + setClosed(previous => { + const next = new Set(previous); + if (!next.delete(key)) { + next.add(key); + } + return next; + }); + + return ( + <Column data-test="palette"> + {/* At the default height rather than `small`. This is the way into the + whole palette and the only thing on the tab that is typed into, and + at the smallest step it was shorter than the tiles it filters — the + one control on the panel read as the least of them. */} + <Input + allowClear + value={query} + aria-label={t('Search components')} + placeholder={t('Search components…')} + data-test="palette-search" + prefix={<Icons.SearchOutlined iconSize="s" />} + onChange={event => setQuery(event.target.value)} + /> + {found.length === 0 ? ( + <div data-test="palette-empty"> + <EmptyState + size="small" + image="filter-results.svg" + title={t('No matching blocks')} + description={t('Nothing here is called “%s”.', query)} + /> + </div> + ) : ( + <Shelves> + {SHELVES.map(shelf => { + const onShelf = found.filter(entry => entry.shelf === shelf.key); + // An empty shelf is not a shelf: it would imply something failed + // to register rather than that nothing of that kind exists. + if (onShelf.length === 0) { + return null; + } + return ( + <Disclosure + key={shelf.key} + name={shelf.name} + open={isOpen(shelf.key)} + onToggle={() => toggle(shelf.key)} + > + {onShelf.map(entry => ( + <BlockTile + key={entry.type} + type="button" + draggable + title={entry.description} + data-test={`palette-${entry.type}`} + onClick={() => onAdd(entry.type)} + // The grip beside the label promised this and did not + // deliver it: the tiles carried the affordance of a drag + // without the drag. Clicking still appends to whatever is + // selected; dragging is how an author says *where*. + onDragStart={event => { + event.dataTransfer.setData(PALETTE_MIME, entry.type); + event.dataTransfer.effectAllowed = 'copy'; + }} + > + {/* Beside what it drags rather than at the far edge of + the tile: the panel is resizable, and a handle pinned + right drifts further from its label the wider it is + pulled. Decoration — the tile already carries the name, + so announcing the grip again would only repeat it. */} + <span className="palette-grip" aria-hidden> + <Icons.HolderOutlined iconSize="s" /> + </span> + {entry.label} + </BlockTile> + ))} + </Disclosure> + ); + })} + </Shelves> + )} + </Column> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/PropsForm.tsx b/superset-frontend/src/pages/DashboardBuilderV2/PropsForm.tsx new file mode 100644 index 000000000000..a870b0e5df66 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/PropsForm.tsx @@ -0,0 +1,272 @@ +/** + * 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 { useMemo } from 'react'; +import type { ReactElement } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled, useTheme } from '@apache-superset/core/theme'; +import { JsonForms } from '@jsonforms/react'; +import { cellRegistryEntries } from '@great-expectations/jsonforms-antd-renderers'; +import { renderers } from 'src/features/semanticLayers/jsonFormsHelpers'; +import { provider } from 'src/core/dashboard/store'; +import inferPropsSchema, { untypedKeys } from './inferPropsSchema'; + +/** + * What the generated form is made to agree with. + * + * The controls come from a third-party renderer set, and it lays a form out + * for a page of its own rather than for a rail beside a canvas. Four things + * came out of it not matching the panel around them, and none of them can be + * fixed at the call site because nothing here renders the controls: + * + * - a group's name arrives as a bare `b` with no size, weight or space of its + * own, so it read as the run-on end of the field above rather than as the + * head of the group below. It is given `Section`'s heading, which is what a + * group of fields is titled with everywhere else in this panel. + * - controls are sized by what they hold: some carry `width: 100%`, some sit + * in an auto-width column. A column of fields that steps in and out down the + * panel reads as broken before it reads as compact, so they are all told to + * fill the column. + * - what adds a row to an array sits in a list footer, which antd centres, + * while what deletes one is pushed right — so two controls doing the same + * kind of job to the same array sat at opposite ends of it. Both go left, + * where every other control in this rail starts. + * - the buttons are antd's own default, which is `tertiary` in this app's + * terms and the right style for them; what they are not is the height the + * rest of the rail is at, and a form of full-height buttons inside a panel + * of small ones is the join showing. + * + * Scoped to this element rather than fixed in the renderers, which the + * semantic-layer modal also draws from and which are not this change's to move + * — the same reason `DashboardProperties` scopes its own input fix. + */ +const FormShell = styled.div` + ${({ theme }) => css` + /* A group's name, at the weight Section titles a group with. */ + > form > b, + fieldset > b { + display: block; + margin: ${theme.sizeUnit * 4}px 0 ${theme.sizeUnit * 2}px; + font-size: ${theme.fontSize}px; + font-weight: ${theme.fontWeightStrong}; + color: ${theme.colorText}; + } + + /* One column, one width. + + An array's entries are handed to a grid meant for a page — two to a + line, so a dimension came out half the width of the field above it. In a + rail there is no second column to put anything in, so the grid is turned + down its own axis and every cell given the width. The form item's own + label/control row is left alone: it is already a column in this layout, + and it is not a grid of entries. */ + .ant-form-item-control-input-content .ant-row:not(.ant-form-item-row) { + flex-direction: column; + align-items: stretch; + } + + .ant-form-item-control-input-content > .ant-col, + .ant-form-item-control-input-content + .ant-row:not(.ant-form-item-row) + > .ant-col { + flex: 1 1 auto; + min-width: 0; + width: 100%; + max-width: 100%; + } + + .ant-input, + .ant-input-number, + .ant-picker, + .ant-select { + width: 100%; + } + + /* One entry of an array: its fields down the column, and what removes it + beneath them. + + antd lays a list item as a row and pushes its actions to the far end, so + the fields of an entry shared the width with a Delete button and came + out a hundred pixels narrower than the fields around them — the only + reason "Column Name" sat short of "Dataset Id". Stacked, the fields get + the column and the button falls under them at the start, which is where + the other thing that acts on this array already is. */ + .ant-list-item { + flex-direction: column; + align-items: stretch; + gap: ${theme.sizeUnit}px; + padding-inline: 0; + } + + /* Written at antd's own depth, and doubled. + + Two things have to be beaten here. antd indents the actions with + margin-inline-start, which a physical margin-left does not compete with; + and it says so through a selector wrapped in :where(), which counts for + nothing and leaves three classes — more than this element plus its own + class, until the rule is written out this long. The indent is meant for + a list of actions on a page-wide row; on one Delete under a field it is + a step with nothing to line up against. */ + && .ant-list .ant-list-item .ant-list-item-action { + margin-inline-start: 0; + padding-inline: 0; + text-align: left; + } + + && .ant-list .ant-list-item .ant-list-item-action > li { + padding-inline: 0; + } + + /* Whatever acts on an array, at the start of it. What adds an entry is + handed to a centred flex row and what removes one to a list action, so + the two controls doing the same kind of job to the same array sat at + opposite ends of it. */ + .ant-list-footer, + .ant-list-header { + padding-inline: 0; + text-align: left; + } + + .ant-flex-justify-center, + .ant-form-item-control-input-content > .ant-row { + justify-content: flex-start; + } + + /* At the rail's own control height, like every button beside it. */ + .ant-btn { + height: ${theme.controlHeightSM}px; + font-size: ${theme.fontSizeSM}px; + } + + /* One rhythm down the column: the renderers space their own items and + their dividers, and the two scales did not agree. */ + .ant-form-item { + margin-bottom: ${theme.sizeUnit * 2}px; + } + + .ant-divider-horizontal { + margin: ${theme.sizeUnit * 3}px 0; + } + `} +`; + +/** + * A block's properties as fields, generated from the values it holds. + * + * The other half of this panel edits the same properties as JSON, and the two + * divide cleanly: JSON is where the *shape* is decided — a key added, a key + * dropped — and this is where the values in that shape are filled in. That is + * not a limitation to work around but what a generated form is: with no schema + * shipped alongside a block's registration (see `inferPropsSchema`), a field + * can only exist where a value already does. + * + * Edits are written as they are made rather than held until focus leaves. + * JsonForms already debounces what it reports by 10ms, and that debounce is + * exactly what a commit on blur races: clicking away fires the blur first and + * commits the draft as it stood a moment before the last keystroke, which + * silently drops it. Writing from `onChange` has one ordering and no draft to + * fall behind. + */ +export default function PropsForm({ + nodeId, + props, +}: { + nodeId: string; + props: Record<string, unknown> | undefined; +}): ReactElement { + const theme = useTheme(); + // Compared by value rather than by identity: `props` is a fresh object on + // every render of the panel, so anything derived from it has to be keyed on + // what it says rather than on which object it is, or the form is rebuilt + // under the cursor on every unrelated tick of the store. + const accepted = JSON.stringify(props ?? {}); + const data = useMemo( + () => JSON.parse(accepted) as Record<string, unknown>, + [accepted], + ); + const schema = useMemo(() => inferPropsSchema(data), [data]); + const untyped = useMemo(() => untypedKeys(data), [data]); + const empty = Object.keys(schema.properties ?? {}).length === 0; + + const note = (text: string) => ( + <p + style={{ + margin: 0, + color: theme.colorTextTertiary, + fontSize: theme.fontSizeSM, + }} + > + {text} + </p> + ); + + return ( + <FormShell + data-test="inspector-props-form" + // Labels above their fields, as everywhere else in this rail — beside + // them halves the width left for the control, in the panel that most + // needs the room. + // + // Said in classes rather than by wrapping this in an antd `Form`, + // because that is what the renderers read: `useParentFormLayout` takes + // the layout off the nearest `.ant-form` ancestor's class, by its own + // account, precisely so it does not depend on antd's form context. A + // real `Form` here would set the layout and take the edits with it — + // see `PropsEditor`. + className="ant-form ant-form-vertical" + > + {empty + ? note( + t( + 'This block has no properties yet. Add them on the JSON tab, and they become fields here.', + ), + ) + : /* No `uischema`: JsonForms lays out whatever the schema describes, + which is the point of generating the schema in the first place. */ + null} + {!empty && ( + <JsonForms + schema={schema} + data={data} + renderers={renderers} + cells={cellRegistryEntries} + // Nothing here is required and nothing is constrained, because the + // schema was read off values a block already renders from — so a + // validation message could only ever be about a type this form + // itself assigned. + validationMode="NoValidation" + onChange={({ data: next }) => { + // Guarded because this fires on mount with what was passed in, + // and again with the value that has just been written — neither + // is an edit, and both would otherwise tick the store. + if (JSON.stringify(next) !== accepted) { + provider.updateProps(nodeId, next as Record<string, unknown>); + } + }} + /> + )} + {untyped.length > 0 && + note( + t( + 'Only editable as JSON, having no value to take a type from: %s', + untyped.join(', '), + ), + )} + </FormShell> + ); +} diff --git a/superset-frontend/src/pages/DashboardBuilderV2/clientTools.test.ts b/superset-frontend/src/pages/DashboardBuilderV2/clientTools.test.ts new file mode 100644 index 000000000000..f0468b7ddc36 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/clientTools.test.ts @@ -0,0 +1,145 @@ +/** + * 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 { chat } from 'src/core/chat'; +import ChatProvider from 'src/core/chat/ChatProvider'; +import DashboardProvider from 'src/core/dashboard/DashboardProvider'; +import { dashboardClientTools } from './clientTools'; + +beforeEach(() => { + ChatProvider.getInstance().reset(); + DashboardProvider.getInstance().reset(); + chat.registerClientTools(dashboardClientTools); +}); + +async function execute(name: string, args: Record<string, unknown>) { + const result = await chat.executeClientTool(name, args); + return { ...result, value: JSON.parse(result.content) }; +} + +test('reads the blank Dashboard v2 tree', async () => { + const result = await execute('dashboard_get_state', {}); + + expect(result.isError).toBeUndefined(); + expect(result.value).toEqual({ + rootId: 'root', + nodes: { + root: { + id: 'root', + type: 'canvas', + layout: { columns: 24, gap: 16 }, + children: [], + }, + }, + }); +}); + +test('adds and edits a visible dashboard block', async () => { + const added = await execute('dashboard_add_building_block', { + parent_id: 'root', + block: { + type: 'markdown', + layout: { colSpan: 12, rowSpan: 2 }, + props: { content: '# Revenue' }, + }, + }); + const id = added.value.node.id as string; + + await execute('dashboard_update_layout', { + id, + layout: { col: 3, row: 2, colSpan: 10 }, + }); + const updated = await execute('dashboard_update_props', { + id, + props: { content: '# Net revenue' }, + }); + + expect(updated.value.node).toEqual( + expect.objectContaining({ + id, + layout: expect.objectContaining({ col: 3, row: 2, colSpan: 10 }), + props: { content: '# Net revenue' }, + }), + ); +}); + +test('changes an ECharts palette without replacing the existing chart options', async () => { + const provider = DashboardProvider.getInstance(); + const id = provider.addBuildingBlock('root', 0, { + type: 'echarts', + props: { + dataBinding: { datasetId: 1, metrics: ['sum__sales'] }, + echartsOptions: { + color: ['#old'], + xAxis: { type: 'category' }, + series: [{ type: 'bar', data: [1, 2] }], + }, + }, + }); + + const updated = await execute('dashboard_update_props', { + id, + props: { echartsOptions: { color: ['#1677ff', '#52c41a'] } }, + }); + + expect(updated.isError).toBeUndefined(); + expect(updated.value.node.props.echartsOptions).toEqual({ + color: ['#1677ff', '#52c41a'], + xAxis: { type: 'category' }, + series: [{ type: 'bar', data: [1, 2] }], + }); +}); + +test('moves a block into a nested canvas and removes the subtree', async () => { + const canvas = await execute('dashboard_add_building_block', { + parent_id: 'root', + block: { type: 'canvas', layout: { columns: 12, colSpan: 24 } }, + }); + const canvasId = canvas.value.node.id as string; + const markdown = await execute('dashboard_add_building_block', { + parent_id: 'root', + block: { type: 'markdown', props: { content: 'Move me' } }, + }); + const markdownId = markdown.value.node.id as string; + + await execute('dashboard_move_building_block', { + id: markdownId, + new_parent_id: canvasId, + new_index: 0, + }); + let state = await execute('dashboard_get_state', {}); + expect(state.value.nodes[canvasId].children).toEqual([markdownId]); + + await execute('dashboard_remove_building_block', { id: canvasId }); + state = await execute('dashboard_get_state', {}); + expect(state.value.nodes[canvasId]).toBeUndefined(); + expect(state.value.nodes[markdownId]).toBeUndefined(); +}); + +test('invalid model arguments become a client-tool error', async () => { + const result = await chat.executeClientTool('dashboard_add_building_block', { + parent_id: 'missing', + block: { type: 'markdown' }, + }); + + expect(result).toEqual({ + content: + 'Client tool "dashboard_add_building_block" failed: Parent "missing" is not a canvas node.', + isError: true, + }); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/clientTools.ts b/superset-frontend/src/pages/DashboardBuilderV2/clientTools.ts new file mode 100644 index 000000000000..3795cdf0f06a --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/clientTools.ts @@ -0,0 +1,443 @@ +/** + * 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 { + chat as chatApi, + dashboard as dashboardApi, +} from '@apache-superset/core'; +import { dashboard } from 'src/core/dashboard'; + +type ClientTool = chatApi.ClientTool; +type ClientToolResult = chatApi.ClientToolResult; +type DashboardNode = dashboardApi.DashboardNode; +type BuildingBlockSpec = dashboardApi.BuildingBlockSpec; +type LayoutProps = dashboardApi.LayoutProps; +type DataBindingSpec = dashboardApi.DataBindingSpec; +type QueryDataResult = dashboardApi.QueryDataResult; + +const QUERY_BACKED_BLOCKS = new Set([ + 'echarts', + 'ag-grid-table', + 'metric-tile', +]); +const PREVIEW_ROWS = 20; + +const emptyInputSchema = { + type: 'object', + properties: {}, + additionalProperties: false, +}; + +const layoutSchema = { + type: 'object', + description: + 'Grid geometry. col/row are 1-based; omit both to auto-place the block.', + properties: { + columns: { type: 'integer', minimum: 1 }, + gap: { type: 'number', minimum: 0 }, + rowUnit: { type: 'number', exclusiveMinimum: 0 }, + colSpan: { type: 'integer', minimum: 1 }, + rowSpan: { type: 'integer', minimum: 1 }, + col: { type: 'integer', minimum: 1 }, + row: { type: 'integer', minimum: 1 }, + }, + additionalProperties: false, +}; + +const dataBindingSchema = { + type: 'object', + properties: { + datasetId: { type: 'integer', minimum: 1 }, + metrics: { + type: 'array', + description: 'Saved metric names or Superset ad-hoc metric objects.', + items: {}, + }, + dimensions: { type: 'array', items: { type: 'string' } }, + filters: { type: 'array', items: { type: 'object' } }, + rowLimit: { type: 'integer', minimum: 1 }, + }, + required: ['datasetId', 'metrics'], + additionalProperties: false, +}; + +function jsonResult(value: unknown): ClientToolResult { + return { content: JSON.stringify(value, null, 2) }; +} + +function asRecord(value: unknown, label: string): Record<string, unknown> { + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + throw new Error(`${label} must be an object.`); + } + return value as Record<string, unknown>; +} + +function requiredRecord( + args: Record<string, unknown>, + key: string, +): Record<string, unknown> { + return asRecord(args[key], `"${key}"`); +} + +function requiredString(args: Record<string, unknown>, key: string): string { + const value = args[key]; + if (typeof value !== 'string' || value.trim() === '') { + throw new Error(`"${key}" must be a non-empty string.`); + } + return value; +} + +function requiredInteger(args: Record<string, unknown>, key: string): number { + const value = args[key]; + if (typeof value !== 'number' || !Number.isInteger(value)) { + throw new Error(`"${key}" must be an integer.`); + } + return value; +} + +function optionalInteger( + args: Record<string, unknown>, + key: string, + fallback: number, +): number { + return args[key] === undefined ? fallback : requiredInteger(args, key); +} + +function positiveNumber( + value: unknown, + label: string, + integer: boolean, + allowZero = false, +): number { + const lowerBoundValid = + typeof value === 'number' && (allowZero ? value >= 0 : value > 0); + if (!lowerBoundValid || (integer && !Number.isInteger(value))) { + const kind = integer ? 'integer' : 'number'; + throw new Error( + `${label} must be a ${allowZero ? 'non-negative' : 'positive'} ${kind}.`, + ); + } + return value; +} + +function optionalLayout(value: unknown): Partial<LayoutProps> | undefined { + if (value === undefined) return undefined; + const layout = asRecord(value, '"layout"'); + const result: Partial<LayoutProps> = {}; + if (layout.columns !== undefined) { + result.columns = positiveNumber(layout.columns, 'layout.columns', true); + } + if (layout.gap !== undefined) { + result.gap = positiveNumber(layout.gap, 'layout.gap', false, true); + } + if (layout.rowUnit !== undefined) { + result.rowUnit = positiveNumber(layout.rowUnit, 'layout.rowUnit', false); + } + if (layout.colSpan !== undefined) { + result.colSpan = positiveNumber(layout.colSpan, 'layout.colSpan', true); + } + if (layout.rowSpan !== undefined) { + result.rowSpan = positiveNumber(layout.rowSpan, 'layout.rowSpan', true); + } + if (layout.col !== undefined) { + result.col = positiveNumber(layout.col, 'layout.col', true); + } + if (layout.row !== undefined) { + result.row = positiveNumber(layout.row, 'layout.row', true); + } + return result; +} + +function dataBinding(value: unknown): DataBindingSpec { + const binding = asRecord(value, '"dataBinding"'); + const datasetId = positiveNumber( + binding.datasetId, + 'dataBinding.datasetId', + true, + ); + if (!Array.isArray(binding.metrics)) { + throw new Error('dataBinding.metrics must be an array.'); + } + if ( + binding.dimensions !== undefined && + (!Array.isArray(binding.dimensions) || + !binding.dimensions.every(item => typeof item === 'string')) + ) { + throw new Error('dataBinding.dimensions must be an array of strings.'); + } + if ( + binding.filters !== undefined && + (!Array.isArray(binding.filters) || + !binding.filters.every( + item => + item !== null && typeof item === 'object' && !Array.isArray(item), + )) + ) { + throw new Error('dataBinding.filters must be an array of objects.'); + } + + const rowLimit = + binding.rowLimit === undefined + ? undefined + : positiveNumber(binding.rowLimit, 'dataBinding.rowLimit', true); + + return { + datasetId, + metrics: binding.metrics, + dimensions: binding.dimensions as string[] | undefined, + filters: binding.filters as Record<string, unknown>[] | undefined, + rowLimit, + }; +} + +function blockSpec(value: unknown): BuildingBlockSpec { + const block = asRecord(value, '"block"'); + if (typeof block.type !== 'string' || block.type.trim() === '') { + throw new Error('block.type must be a non-empty string.'); + } + return { + type: block.type, + layout: optionalLayout(block.layout), + props: + block.props === undefined + ? undefined + : asRecord(block.props, 'block.props'), + style: + block.style === undefined + ? undefined + : asRecord(block.style, 'block.style'), + }; +} + +async function validateQueryBackedBlock( + type: string, + props: Record<string, unknown> | undefined, + requireBinding: boolean, +): Promise<QueryDataResult | undefined> { + if (!QUERY_BACKED_BLOCKS.has(type)) return undefined; + if (!props || props.dataBinding === undefined) { + if (requireBinding) { + throw new Error(`${type} blocks require props.dataBinding.`); + } + return undefined; + } + return dashboard.fetchQueryData(dataBinding(props.dataBinding)); +} + +function queryPreview(result: QueryDataResult | undefined) { + return result + ? { columns: result.columns, rows: result.rows.slice(0, PREVIEW_ROWS) } + : undefined; +} + +function readDashboardState() { + const root = dashboard.getRoot(); + const nodes: Record<string, DashboardNode> = {}; + + const visit = (node: DashboardNode) => { + nodes[node.id] = node; + node.children?.forEach(id => { + const child = dashboard.getNode(id); + if (child) visit(child); + }); + }; + visit(root); + + return { rootId: root.id, nodes }; +} + +/** + * Tools offered only while Dashboard v2 is mounted. Their handlers use the + * same public dashboard API that extensions use, so model edits and direct UI + * edits share one store, renderer, collision policy, and revision stream. + */ +export const dashboardClientTools: ClientTool[] = [ + { + name: 'dashboard_get_state', + description: + 'Read the complete unsaved Dashboard v2 tree visible on screen. Call this before editing so you use real node and parent ids. Returns {rootId,nodes}, where nodes is keyed by id and each node has type, layout, props, style, and canvas child ids. Built-in types are canvas, markdown, echarts, ag-grid-table, and metric-tile.', + inputSchema: emptyInputSchema, + execute: () => jsonResult(readDashboardState()), + }, + { + name: 'dashboard_validate_data_binding', + description: + 'Run a Dashboard v2 data binding against Superset before creating or changing a live chart, table, or metric tile. Use exact dataset, metric, and dimension names obtained from server tools. Returns result column aliases and up to 20 preview rows; use those exact aliases in ECharts $bind markers.', + inputSchema: { + type: 'object', + properties: { dataBinding: dataBindingSchema }, + required: ['dataBinding'], + additionalProperties: false, + }, + execute: async args => { + const result = await dashboard.fetchQueryData( + dataBinding(args.dataBinding), + ); + return jsonResult(queryPreview(result)); + }, + }, + { + name: 'dashboard_add_building_block', + description: + 'Add a block to the unsaved Dashboard v2 canvas and show it immediately. Read the state first. parent_id must name a canvas; omit index to append. Built-ins: markdown props {content}; metric-tile props {dataBinding,label?,prefix?,suffix?,decimals?}; ag-grid-table props {dataBinding,columnDefs?}; echarts props {dataBinding,echartsOptions}. In echartsOptions bind result data with {"$bind":{"source":"metric"|"dimension","alias":"exact column alias","single":true?}}, record arrays with {"$bind":{"source":"records","fields":{"name":"dimension alias","value":"metric alias"}}}, and theme tokens with {"$bind":{"source":"theme","token":"colorPrimary"}}. A canvas has children and may set layout.columns/gap/rowUnit. Every block layout may set colSpan/rowSpan and optional 1-based col/row. Query-backed blocks are validated before insertion. Returns the created node and a data preview when applicable.', + inputSchema: { + type: 'object', + properties: { + parent_id: { type: 'string' }, + index: { type: 'integer', minimum: 0 }, + block: { + type: 'object', + properties: { + type: { type: 'string' }, + layout: layoutSchema, + props: { type: 'object' }, + style: { type: 'object' }, + }, + required: ['type'], + additionalProperties: false, + }, + }, + required: ['parent_id', 'block'], + additionalProperties: false, + }, + execute: async args => { + const parentId = requiredString(args, 'parent_id'); + const parent = dashboard.getNode(parentId); + if (!parent?.children) { + throw new Error(`Parent "${parentId}" is not a canvas node.`); + } + const index = optionalInteger(args, 'index', parent.children.length); + if (index < 0) throw new Error('"index" must be non-negative.'); + const spec = blockSpec(args.block); + const preview = await validateQueryBackedBlock( + spec.type, + spec.props, + true, + ); + const id = dashboard.addBuildingBlock(parentId, index, spec); + return jsonResult({ + node: dashboard.getNode(id), + preview: queryPreview(preview), + }); + }, + }, + { + name: 'dashboard_update_layout', + description: + 'Move or resize one existing Dashboard v2 node within its current parent grid. col/row are 1-based; colSpan/rowSpan control size. Explicit collisions push later blocks downward. Returns the updated node.', + inputSchema: { + type: 'object', + properties: { + id: { type: 'string' }, + layout: layoutSchema, + }, + required: ['id', 'layout'], + additionalProperties: false, + }, + execute: args => { + const id = requiredString(args, 'id'); + const layout = optionalLayout(args.layout); + if (!layout) throw new Error('"layout" is required.'); + dashboard.updateLayout(id, layout); + return jsonResult({ node: dashboard.getNode(id) }); + }, + }, + { + name: 'dashboard_update_props', + description: + 'Merge content properties into an existing Dashboard v2 block and show the change immediately. Read dashboard_get_state first and use its node id; no saved dashboard or chart id is needed. Use this for markdown content, ECharts options/dataBinding, table configuration, or metric-tile labels and formatting. echartsOptions is merged at its top level, so a color-only change can send props {echartsOptions: {color: ["#hex", ...]}} without replacing axes, series, or data bindings. For multiple charts, call this separately for each chart node. A changed dataBinding is validated before the update. Returns the updated node and a data preview when applicable.', + inputSchema: { + type: 'object', + properties: { + id: { type: 'string' }, + props: { type: 'object' }, + }, + required: ['id', 'props'], + additionalProperties: false, + }, + execute: async args => { + const id = requiredString(args, 'id'); + const node = dashboard.getNode(id); + if (!node) throw new Error(`Unknown dashboard node "${id}".`); + const props = requiredRecord(args, 'props'); + const nextProps = { ...props }; + if (props.echartsOptions !== undefined) { + const currentOptions = + node.props?.echartsOptions === undefined + ? {} + : asRecord(node.props.echartsOptions, 'existing echartsOptions'); + nextProps.echartsOptions = { + ...currentOptions, + ...asRecord(props.echartsOptions, 'props.echartsOptions'), + }; + } + const mergedProps = { ...node.props, ...nextProps }; + const preview = + props.dataBinding === undefined + ? undefined + : await validateQueryBackedBlock(node.type, mergedProps, false); + dashboard.updateProps(id, nextProps); + return jsonResult({ + node: dashboard.getNode(id), + preview: queryPreview(preview), + }); + }, + }, + { + name: 'dashboard_move_building_block', + description: + 'Move an existing Dashboard v2 block (including a canvas subtree) into another canvas. The old explicit grid position is cleared so it auto-places in the destination. Returns the moved node and destination canvas.', + inputSchema: { + type: 'object', + properties: { + id: { type: 'string' }, + new_parent_id: { type: 'string' }, + new_index: { type: 'integer', minimum: 0 }, + }, + required: ['id', 'new_parent_id', 'new_index'], + additionalProperties: false, + }, + execute: args => { + const id = requiredString(args, 'id'); + const newParentId = requiredString(args, 'new_parent_id'); + const newIndex = requiredInteger(args, 'new_index'); + if (newIndex < 0) throw new Error('"new_index" must be non-negative.'); + dashboard.moveBuildingBlock(id, newParentId, newIndex); + return jsonResult({ + node: dashboard.getNode(id), + destination: dashboard.getNode(newParentId), + }); + }, + }, + { + name: 'dashboard_remove_building_block', + description: + 'Remove one Dashboard v2 block from the unsaved canvas. Removing a canvas also removes its entire subtree. Never remove the root. Read the state first and use the exact node id. Returns the removed id and the resulting dashboard state.', + inputSchema: { + type: 'object', + properties: { id: { type: 'string' } }, + required: ['id'], + additionalProperties: false, + }, + execute: args => { + const id = requiredString(args, 'id'); + dashboard.removeBuildingBlock(id); + return jsonResult({ removed: id, dashboard: readDashboardState() }); + }, + }, +]; diff --git a/superset-frontend/src/pages/DashboardBuilderV2/index.test.tsx b/superset-frontend/src/pages/DashboardBuilderV2/index.test.tsx new file mode 100644 index 000000000000..8c4515a38d37 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/index.test.tsx @@ -0,0 +1,156 @@ +/** + * 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 userEvent from '@testing-library/user-event'; +import { render, screen } from 'spec/helpers/testing-library'; +import DashboardProvider from 'src/core/dashboard/DashboardProvider'; +import DashboardBuilderV2 from '.'; + +jest.mock('src/core/chat', () => ({ + chat: { registerClientTools: () => ({ dispose: () => {} }) }, +})); + +const provider = DashboardProvider.getInstance(); + +beforeEach(() => { + provider.reset(); +}); + +const renderPage = () => render(<DashboardBuilderV2 />, { useRedux: true }); + +test('a blank dashboard can still be reached, and so can the layout it arranges in', async () => { + renderPage(); + + // The canvas is no longer chat-only: a palette sits beside it, so the + // empty state names both ways in. + expect( + screen.getByText( + 'Drag a building block from the panel, or ask the assistant for one.', + ), + ).toBeInTheDocument(); + + // A `/dashboard/v2/new/` load lands on nothing, and that is exactly when + // someone reaches for the layout control: whatever is placed next lands in + // the mode already chosen, rather than being placed and then rearranged. + // Arranging is asked in the root's own properties, so the blank canvas has + // to be selectable or the mode is unreachable until something is placed. + await userEvent.click(screen.getByTestId('empty-canvas')); + + expect(provider.getSelection()).toBe(provider.getRoot().id); + expect(screen.getByTestId('layout-mode-switcher')).toBeInTheDocument(); +}); + +test('selecting the root offers the layout once blocks have been placed too', async () => { + provider.addBuildingBlock(provider.getRoot().id, 0, { type: 'markdown' }); + renderPage(); + + provider.setSelection(provider.getRoot().id); + + expect(await screen.findByTestId('layout-mode-switcher')).toBeInTheDocument(); +}); + +test('the canvas carries the route to how it is arranged', async () => { + provider.addBuildingBlock(provider.getRoot().id, 0, { type: 'markdown' }); + renderPage(); + + // On the thing it arranges rather than on the bar above it: choosing how + // blocks lay out is done while looking at the blocks, and the control it + // leads to is one selection away in the root's own properties. + await userEvent.click(screen.getByTestId('canvas-arrange')); + + expect(provider.getSelection()).toBe(provider.getRoot().id); + expect(screen.getByTestId('layout-mode-switcher')).toBeInTheDocument(); +}); + +test('the arrange shortcut is offered on a blank canvas too', () => { + renderPage(); + + // A blank dashboard is exactly when the mode is chosen — whatever is placed + // next lands in it. + expect(screen.getByTestId('canvas-arrange')).toBeInTheDocument(); +}); + +test('refreshing sits with arranging, and is honest about not working', () => { + renderPage(); + + // Both act on the canvas as a whole rather than on anything placed in it, + // so they are reached for from the same corner. Refreshing is still the + // affordance this builder cannot honour: there is no dashboard row behind + // the page and nothing to re-read. + expect(screen.getByTestId('canvas-refresh')).toBeDisabled(); +}); + +test('the page is a header, an editor panel and a canvas', () => { + renderPage(); + + expect(screen.getByTestId('dashboard-header')).toBeInTheDocument(); + expect(screen.getByTestId('editor-panel')).toBeInTheDocument(); + expect(screen.getByTestId('canvas')).toBeInTheDocument(); +}); + +test('placing a block from the palette puts it on the dashboard and selects it', async () => { + renderPage(); + + await userEvent.click(screen.getByTestId('palette-markdown')); + + const children = provider.getRoot().children ?? []; + expect(children).toHaveLength(1); + // Placing something is the moment you want to configure it, which is also + // what brings Properties forward. + expect(provider.getSelection()).toBe(children[0]); +}); + +test('a block placed while a container is selected goes inside it', async () => { + renderPage(); + await userEvent.click(screen.getByTestId('palette-canvas')); + const sectionId = provider.getSelection()!; + + await userEvent.click(screen.getByTestId('palette-markdown')); + + // An author who has just selected a section and reaches for a block means + // to put it in that section. + expect(provider.getNode(sectionId)?.children).toEqual([ + provider.getSelection(), + ]); + expect(provider.getRoot().children).toEqual([sectionId]); +}); + +test('a block placed while a leaf is selected goes beside it, not inside it', async () => { + renderPage(); + await userEvent.click(screen.getByTestId('palette-markdown')); + const firstId = provider.getSelection()!; + + await userEvent.click(screen.getByTestId('palette-echarts')); + + expect(provider.getRoot().children).toEqual([ + firstId, + provider.getSelection(), + ]); +}); + +test('clicking the canvas itself clears the selection', async () => { + renderPage(); + await userEvent.click(screen.getByTestId('palette-markdown')); + expect(provider.getSelection()).toBeDefined(); + + await userEvent.click(screen.getByTestId('canvas')); + + // A click that reached the canvas passed every block on the way, so it is + // the one gesture that unambiguously means "nothing". + expect(provider.getSelection()).toBeUndefined(); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/index.tsx b/superset-frontend/src/pages/DashboardBuilderV2/index.tsx index 287ea64b7cba..44dd0bbd61e5 100644 --- a/superset-frontend/src/pages/DashboardBuilderV2/index.tsx +++ b/superset-frontend/src/pages/DashboardBuilderV2/index.tsx @@ -16,12 +16,19 @@ * specific language governing permissions and limitations * under the License. */ +import { useEffect } from 'react'; import { t } from '@apache-superset/core/translation'; import { css, styled } from '@apache-superset/core/theme'; -import { Flex, Typography } from '@superset-ui/core/components'; -import { Icons } from '@superset-ui/core/components/Icons'; +import { EmptyState, Flex } from '@superset-ui/core/components'; import { dashboard, useDashboardRevision } from 'src/core/dashboard'; +import { provider } from 'src/core/dashboard/store'; +import { placeBlock } from 'src/core/dashboard/placement'; +import { chat } from 'src/core/chat'; import BuildingBlockView from 'src/core/dashboard/BuildingBlockView'; +import { dashboardClientTools } from './clientTools'; +import CanvasControls from './CanvasControls'; +import DashboardHeader from './DashboardHeader'; +import EditorPanel from './EditorPanel'; const PageContainer = styled(Flex)` ${({ theme }) => css` @@ -37,10 +44,22 @@ const Canvas = styled.div` flex: 1; min-height: 0; overflow: auto; - padding: ${theme.paddingLG}px; + /* What the canvas's own corner controls are positioned against. */ + position: relative; + /* Two past the token, so the corner controls clear the frame the root + draws inside this padding rather than sitting hard against it. Written + as an offset from the token rather than as a literal, so it still moves + with the scale the rest of the app is built on. */ + padding: ${theme.paddingLG + 2}px; `} `; +const Workspace = styled.div` + display: flex; + flex: 1 1 auto; + min-height: 0; +`; + const EmptyCanvasWrapper = styled.div` ${({ theme }) => css` height: 100%; @@ -51,13 +70,36 @@ const EmptyCanvasWrapper = styled.div` `} `; +/** + * The dashboard with nothing on it, as something to aim at. + * + * A dashed frame is what the rest of the app draws around a place a thing can + * be dropped, and this is one — the palette drags land here. It is also the + * only way to select the root on a blank canvas, so it answers a pointer and a + * Tab the way anything clickable does: the frame firms up and the surface + * lifts, rather than a dashed box that never reacts to being pressed. + */ const CanvasPlaceholder = styled(Flex)` ${({ theme }) => css` width: 100%; height: 100%; - border: 2px dashed ${theme.colorBorderSecondary}; + border: 2px dashed ${theme.colorBorder}; border-radius: ${theme.borderRadiusLG}px; color: ${theme.colorTextTertiary}; + cursor: pointer; + transition: + border-color ${theme.motionDurationMid}, + background-color ${theme.motionDurationMid}; + + &:hover { + border-color: ${theme.colorPrimaryBorderHover}; + background-color: ${theme.colorFillQuaternary}; + } + + &:focus-visible { + outline: 2px solid ${theme.colorPrimaryBorder}; + outline-offset: 2px; + } `} `; @@ -84,30 +126,98 @@ export default function DashboardBuilderV2() { // Ticks on every dashboard.* mutation so this tree re-renders to reflect // whatever the chat agent (or any other caller of the dashboard API) did. useDashboardRevision(); + useEffect(() => { + const registration = chat.registerClientTools(dashboardClientTools); + return () => registration.dispose(); + }, []); const root = dashboard.getRoot(); const isEmpty = !root.children || root.children.length === 0; + /** + * Places a block from the palette. + * + * Into whatever is selected when that can hold children, and into the root + * otherwise. An author who has just selected a section and reaches for a + * chart means to put it in that section; one who has selected a chart means + * to put the next thing beside it, not inside it. + * + * A drag from the palette says where for itself — the container it was + * dropped on takes it — so only the click needs a target chosen for it. + * Both then go through the same `placeBlock`, because two copies of what a + * freshly placed block looks like is how the two paths quietly diverge. + */ + const addBlock = (type: string): void => { + const selected = provider.getSelection(); + const selectedNode = + selected === undefined ? undefined : provider.getNode(selected); + placeBlock( + selectedNode?.children !== undefined ? selectedNode.id : root.id, + type, + ); + }; + return ( <PageContainer vertical> - <Canvas> - {isEmpty ? ( - <EmptyCanvasWrapper> - <CanvasPlaceholder - vertical - align="center" - justify="center" - gap="small" - > - <Icons.AppstoreOutlined iconSize="xl" /> - <Typography.Text type="secondary"> - {t('Blank dashboard — ask the assistant to start building')} - </Typography.Text> - </CanvasPlaceholder> - </EmptyCanvasWrapper> - ) : ( - <BuildingBlockView nodeId={root.id} /> - )} - </Canvas> + <DashboardHeader /> + <Workspace> + <EditorPanel onAdd={addBlock} /> + <Canvas + data-test="canvas" + onClick={event => { + // A click that reached the canvas itself passed every block on + // the way, so it is the one gesture that unambiguously means + // "nothing". A click on a block stops before here. + if (event.target === event.currentTarget) { + provider.setSelection(undefined); + } + }} + > + {/* The canvas's own corner: what acts on the whole of it rather + than on anything placed in it, and so belongs to it rather than + to the bar above. First in the tree, raised over the root by its + own z-index — see CanvasControls. */} + <CanvasControls /> + {isEmpty ? ( + <EmptyCanvasWrapper> + {/* The dashboard itself, standing in for a canvas that has + nothing on it yet. It selects the root because that is the + only thing there is to select here, and because how the + canvas is arranged is asked in the root's properties — a + blank dashboard is exactly when that is asked, since + whatever is placed next lands in the mode already chosen. + Without this the mode would be unreachable until something + had already been placed and then rearranged. */} + <CanvasPlaceholder + vertical + align="center" + justify="center" + // eslint-disable-next-line jsx-a11y/prefer-tag-over-role + role="button" + tabIndex={0} + aria-label={t('Dashboard')} + data-test="empty-canvas" + onClick={() => provider.setSelection(root.id)} + onKeyDown={event => { + if (event.key === 'Enter' || event.key === ' ') { + event.preventDefault(); + provider.setSelection(root.id); + } + }} + > + <EmptyState + image="empty-dashboard.svg" + title={t('Start building')} + description={t( + 'Drag a building block from the panel, or ask the assistant for one.', + )} + /> + </CanvasPlaceholder> + </EmptyCanvasWrapper> + ) : ( + <BuildingBlockView nodeId={root.id} /> + )} + </Canvas> + </Workspace> </PageContainer> ); } diff --git a/superset-frontend/src/pages/DashboardBuilderV2/inferPropsSchema.test.ts b/superset-frontend/src/pages/DashboardBuilderV2/inferPropsSchema.test.ts new file mode 100644 index 000000000000..16c57db39ea4 --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/inferPropsSchema.test.ts @@ -0,0 +1,102 @@ +/** + * 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 inferPropsSchema, { untypedKeys } from './inferPropsSchema'; + +test('each property is typed by the value the block is holding', () => { + const schema = inferPropsSchema({ + title: 'Revenue', + limit: 10, + showLegend: true, + }); + + expect(schema).toEqual({ + type: 'object', + properties: { + title: { type: 'string' }, + limit: { type: 'number' }, + showLegend: { type: 'boolean' }, + }, + }); +}); + +test('a nested object is described all the way down', () => { + // `dataBinding` is the shape most worth reaching in a form rather than in + // a string of JSON, and it is two levels deep before it says anything. + const schema = inferPropsSchema({ + dataBinding: { datasetId: 3, filters: { region: 'EMEA' } }, + }); + + expect(schema.properties?.dataBinding).toEqual({ + type: 'object', + properties: { + datasetId: { type: 'number' }, + filters: { + type: 'object', + properties: { region: { type: 'string' } }, + }, + }, + }); +}); + +test('a list is described by what is in it', () => { + const schema = inferPropsSchema({ + metrics: ['count', 'sum__value'], + columnDefs: [{ field: 'name', width: 120 }], + }); + + expect(schema.properties?.metrics).toEqual({ + type: 'array', + items: { type: 'string' }, + }); + expect(schema.properties?.columnDefs).toEqual({ + type: 'array', + items: { + type: 'object', + properties: { field: { type: 'string' }, width: { type: 'number' } }, + }, + }); +}); + +test('an empty list is still a list, of nothing in particular', () => { + // There is no element to read a type off, and guessing one would make the + // first thing added to it the wrong type. + const schema = inferPropsSchema({ metrics: [] }); + + expect(schema.properties?.metrics).toEqual({ type: 'array', items: {} }); +}); + +test('a property holding nothing is left out rather than given a type it has not got', () => { + // `null` says only that the key exists. Typing it as a string would turn + // the first edit into a silent change of type, and typing it as an object + // would render a group with no fields — so the form declines it and says + // where it can still be edited. + const schema = inferPropsSchema({ kept: 'yes', cleared: null }); + + expect(Object.keys(schema.properties ?? {})).toEqual(['kept']); + expect(untypedKeys({ kept: 'yes', cleared: null })).toEqual(['cleared']); +}); + +test('a block with no properties has an empty schema rather than no schema', () => { + // JsonForms is handed this either way; an absent `properties` throws where + // an empty one renders nothing. + expect(inferPropsSchema(undefined)).toEqual({ + type: 'object', + properties: {}, + }); +}); diff --git a/superset-frontend/src/pages/DashboardBuilderV2/inferPropsSchema.ts b/superset-frontend/src/pages/DashboardBuilderV2/inferPropsSchema.ts new file mode 100644 index 000000000000..6efe1fc07ffd --- /dev/null +++ b/superset-frontend/src/pages/DashboardBuilderV2/inferPropsSchema.ts @@ -0,0 +1,88 @@ +/** + * 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 { JsonSchema7 } from '@jsonforms/core'; + +/** + * One value's type, or `undefined` where the value does not carry one. + * + * `null` is the case that matters: it says a key exists and nothing else. + * Calling it a string would make the first edit a silent change of type, and + * calling it an object would render a group with no fields in it. + */ +function describe(value: unknown): JsonSchema7 | undefined { + if (typeof value === 'string') return { type: 'string' }; + if (typeof value === 'boolean') return { type: 'boolean' }; + if (typeof value === 'number' && Number.isFinite(value)) { + return { type: 'number' }; + } + if (Array.isArray(value)) { + // Typed by its first element, which is the only element there is to read. + // A list holding more than one shape renders as the first one — a JSON + // question that the JSON half of the panel is the place to answer. + return { type: 'array', items: describe(value[0]) ?? {} }; + } + if (typeof value === 'object' && value !== null) { + const properties: Record<string, JsonSchema7> = {}; + for (const [key, held] of Object.entries(value)) { + const described = describe(held); + if (described !== undefined) { + properties[key] = described; + } + } + return { type: 'object', properties }; + } + return undefined; +} + +/** + * A block's properties, described as a JSON Schema so they can be edited in a + * form instead of in a string of JSON. + * + * Read off the values rather than declared per block type, and deliberately + * so: `BuildingBlockView` resolves a renderer through a registry an extension + * writes into, and a schema per type would make this panel the one place that + * has to learn every type there is — the exact knowledge the render path is + * built not to have. A schema shipped alongside each registration would be + * better still, and this is what stands in until there is one: it describes + * whatever the block is holding, built-in or contributed, with no list to + * keep current. + * + * What it cannot do is invent a key that is not there. A property nothing has + * written yet has no value to read a type off, so it does not appear — which + * is the JSON editor's half of the same panel: that one edits the shape, this + * one edits the values in it. + */ +export default function inferPropsSchema( + props: Record<string, unknown> | undefined, +): JsonSchema7 { + // A record is always an object, so this branch of `describe` always answers. + return describe(props ?? {}) as JsonSchema7; +} + +/** + * The keys `inferPropsSchema` declined, so the form can say what it is not + * showing rather than quietly dropping it. + */ +export function untypedKeys( + props: Record<string, unknown> | undefined, +): string[] { + return Object.entries(props ?? {}) + .filter(([, value]) => describe(value) === undefined) + .map(([key]) => key); +}